Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

nix/flake: installables exclude attribute path from URL #1819

Merged
merged 2 commits into from
Feb 19, 2024

Conversation

evsl
Copy link
Contributor

@evsl evsl commented Feb 16, 2024

Summary

I ran into an issue causing the generated .devbox/gen/flake to contain an invalid flake reference when adding some installables to devbox.json, specifically those which pointed to a remote resource. Local files were fine.

After a quick hike around the repo, the fix seemed trivial enough to open up a PR rather than open up an issue. Interestingly, I didn't seem to find any issues around this even though this bug would've been around for some months.

As it turns out, Installables returned from ParseInstallable included the attribute path when provided a value with any scheme aside from <empty string>, flake, path, or github. This was because the parseURLRef method was not separating the fragment out of the refURL. More generically, the affected branches were any of those that set parsed.URL to the result of refURL.String(), since that specific method will re-add Fragment unless its empty.

Additionally, the test for ParseInstallable with a flake was passing because the test case itself had an invalid flake reference. I corrected that, as well as adding a couple of relevant test cases for some other types as well.

Repo steps

  • Add any non-local-path flake reference with an Installable to devbox.json
    e.g. https://github.com/NixOS/patchelf/archive/master.tar.gz#patchelf
  • Execute the following
DEVBOX_DEBUG=1 devbox shell
  • Observe the following nix error:
Error: nix print-dev-env --json "path:/path/to/your/.devbox/gen/flake": exit status 1

2024/02/16 18:44:09 Command stderr: error: unexpected fragment 'patchelf' in flake reference 'tarball+https://github.com/NixOS/patchelf/archive/master.tar.gz#patchelf'

How was it tested?

  • Updating the relevant test, then making adjustments to ensure they pass.

@gcurtis
Copy link
Collaborator

gcurtis commented Feb 19, 2024

This is great! Thanks for the fix and updating the tests. I also added a check to ParseRef so that we error when parsing refs with a fragment. I'll merge as soon as tests pass.

@gcurtis gcurtis merged commit 1dbcad2 into jetify-com:main Feb 19, 2024
24 checks passed
@kadaan
Copy link
Contributor

kadaan commented Feb 22, 2024

One thing I noticed when I was just looking into this is that in the generated flake.nix file, the flake input names for flakes with fragments still include the fragment. Luckily the target does not, so they are deduplicated and it produces a valid flake file, but it would be good to fix the flake input name too.

@kadaan
Copy link
Contributor

kadaan commented Feb 22, 2024

I'm pretty sure the fix is to changedhttps://github.com/jetpack-io/devbox/blob/master/internal/devpkg/package.go#L255-L255 from result = p.installable.String() + "-" + p.Hash() to result = p.installable.Ref.URL + "-" + p.Hash()

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.

None yet

3 participants