Skip to content

turso-cli: 0.85.3 -> 0.86.3#261729

Merged
wegank merged 2 commits intoNixOS:masterfrom
Fryuni:turso
Oct 18, 2023
Merged

turso-cli: 0.85.3 -> 0.86.3#261729
wegank merged 2 commits intoNixOS:masterfrom
Fryuni:turso

Conversation

@Fryuni
Copy link
Contributor

@Fryuni Fryuni commented Oct 17, 2023

Description of changes

Changes: tursodatabase/turso-cli@v0.85.3...v0.86.3

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 23.11 Release Notes (or backporting 23.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

@ofborg ofborg bot requested a review from kashw2 October 18, 2023 00:05
@ofborg ofborg bot added 11.by: package-maintainer This PR was created by a maintainer of all the package it changes. 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux. labels Oct 18, 2023
Copy link
Contributor

@kashw2 kashw2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@delroth delroth added 12.approvals: 1 This PR was reviewed and approved by one person. 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in any of the changed packages. labels Oct 18, 2023
@Fryuni Fryuni marked this pull request as draft October 18, 2023 10:20
@Fryuni
Copy link
Contributor Author

Fryuni commented Oct 18, 2023

All the CLI commands are working, except the login.

I'm talking with the turso team to see if it is something we are doing here. I'll keep this in draft while figuring that out.

@Fryuni
Copy link
Contributor Author

Fryuni commented Oct 18, 2023

If someone else could try it out to see if the login is working then it would mean it is something on my machine, we can merge this in that case.

@frontsideair
Copy link
Contributor

I'm on the previous version on nixpkgs (v0.85.3) and login didn't work for me either, but there's turso auth login --headless option. I got an environment token as an env var back, but couldn't use anything possibly due to old version. I'll try the version in this PR next.

@frontsideair
Copy link
Contributor

I checked out this PR and run nix run .#turso-cli -- db create with the token env var set, didn't work. Here's the error:

Error: failed to get groups: Get "https://api.turso.tech/v1/groups": net/http: invalid header field value for "Tursocliversion"

List command didn't work either. Hope this is helpful in some way.

@Fryuni
Copy link
Contributor Author

Fryuni commented Oct 18, 2023

So, progress on this is on this thread on their discord

TL;DR:

  • The problem is really on our derivation.
  • I checked out a commit from two months ago and login was working fine (other things were not because old version)
  • I bisected to find out when it started breaking, it was on f0a8d61, from my PR turso-cli: 0.82.0 -> 0.85.3 #257627. At that point, everything works except logging in, which I forgot to test at the time because I was already logged in.
  • Testing each change individually, enabling the prod build tag is what makes the login break. AFAICT that build tag is used only to embed the version instead of using a hard-coded "dev" value.

I have no idea why enabling the prod build tag breaks. It does not seem to be just because of embedding since there are other filed that get embedded even on dev build, but then it works fine.

Following the suggestion @kashw2 made from the beginning (#257627 (comment)) works. I was against it because I though the build tag would be used elsewhere. It isn't, but it could be.

Also, my point here about -X ldflag not working for unexported variable is wrong, I don't know why it failed when I tested, probably I did something else incorrectly.

@Fryuni
Copy link
Contributor Author

Fryuni commented Oct 18, 2023

@frontsideair can you try again with my latest commit? I tried and it is working for me.

@Fryuni Fryuni marked this pull request as ready for review October 18, 2023 13:31
@delroth delroth removed the 12.approvals: 1 This PR was reviewed and approved by one person. label Oct 18, 2023
@frontsideair
Copy link
Contributor

@Fryuni Tried with the last commit, can confirm that login and other authenticated commands that I tried all worked. Thanks!

@ofborg ofborg bot requested a review from kashw2 October 18, 2023 14:17
@Fryuni
Copy link
Contributor Author

Fryuni commented Oct 18, 2023

I figured out why the auth it wasn't working before, it uses the embedded version on the request to the server, but echo "v${version}" > internal/cmd/version.txt adds a newline at the end of the file. So the embedded version was "v0.85.3\n".

Turso uses printf for that, which doesn't add a newline:
https://github.com/tursodatabase/turso-cli/blob/main/internal/cmd/version_prod.go#L11

@LucioFranco
Copy link

@lovesegfault you able to take a look?

@delroth delroth added the 12.approvals: 2 This PR was reviewed and approved by two persons. label Oct 18, 2023
@wegank
Copy link
Member

wegank commented Oct 18, 2023

Result of nixpkgs-review pr 261729 run on aarch64-darwin 1

1 package built:
  • turso-cli

@wegank wegank merged commit edad0cc into NixOS:master Oct 18, 2023
@Fryuni Fryuni deleted the turso branch October 19, 2023 02:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux. 11.by: package-maintainer This PR was created by a maintainer of all the package it changes. 12.approvals: 2 This PR was reviewed and approved by two persons. 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in any of the changed packages.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants