Skip to content

Conversation

gcurtis
Copy link
Collaborator

@gcurtis gcurtis commented Sep 23, 2024

Make sure the flake ref is URL-encoded and quote the rcfile in the shellrc template.

Fixes #2290.

@savil
Copy link
Collaborator

savil commented Sep 23, 2024

Awesome!

I think this solves #1914, and #971 as well? Could you close them if so?

@Lagoja
Copy link
Contributor

Lagoja commented Sep 23, 2024

@savil last time you looked into this issue -- did we discover a compatibility risk with older versions of Nix?

@savil
Copy link
Collaborator

savil commented Sep 23, 2024

@Lagoja I think this was my revert commit #1935. Unfortunately, I forgot to document the reason I had to revert :(

But digging some more, I believe the prior approach relied on using percent-encoded paths which were only supported on nix 2.19. However, I think @gcurtis 's flake.Ref handles this internally (how?), as per his claim here (see internal discussion)

@gcurtis how does flake.Ref handle spaces internally so they work with older nix versions?

@gcurtis
Copy link
Collaborator Author

gcurtis commented Sep 23, 2024

@savil I wasn't aware of that revert. Let me try testing this with older Nix versions before merging.

@gcurtis
Copy link
Collaborator Author

gcurtis commented Sep 23, 2024

@savil flake.Ref.String uses url.PathEscape to encode the file path. I think in the reverted PR we were calling flake.ParseRef("path:" + flakeDirResolved), so it assumed that flakeDirResolved was already encoded (the path: makes it a URL).

I added a test and ran it back to Nix 2.17 and everything seemed to work. I also hardened the shell quoting using the same logic we use for environment variables.

Make sure the flake ref is URL-encoded and quote the rcfile in the
shellrc template.

Fixes #2290.
@gcurtis gcurtis force-pushed the gcurtis/path-whitespace branch from d45c44c to c400e97 Compare September 24, 2024 04:14
@gcurtis gcurtis merged commit ac07204 into main Sep 24, 2024
24 checks passed
@gcurtis gcurtis deleted the gcurtis/path-whitespace branch September 24, 2024 04:25
Copy link

sentry-io bot commented Oct 7, 2024

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

  • ‼️ **exec.ExitError: <redacted *errors.withStack>: <redacted errors.withMessage>: <redacted exec.ExitError> go.jetpack.io/devbox/internal/nix in (*Nix).Pri... View Issue

Did you find this useful? React with a 👍 or 👎

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

devbox fails to start if there is whitespace in the path
3 participants