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.

Sign up for GitHub

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

Jump to bottom

Extend nix-prefetch-git to support passing tree hashes as "rev" #104714

Merged
merged 1 commit into from Feb 20, 2021
Merged

Extend nix-prefetch-git to support passing tree hashes as "rev" #104714

merged 1 commit into from Feb 20, 2021

Conversation

thomasjm
Copy link
Contributor

Motivation for this change

I'm working on Julia packages in Nix. Julia's package manager organizes its dependencies a Manifest.toml file, which is almost perfect to use with Nix because it contains Git hashes we can pull out and pass to fetchgit. However, Julia uses Git tree hashes rather than commit hashes; see here.

It would be convenient if fetchgit could understand tree hashes when they're passed as the rev argument. It seems straightforward to support, as this PR shows.

I've tested this with my prototype Julia packaging tool and it works. If this could be accepted then I'd be happy to update the documentation and stuff. Thanks!

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions (Ubuntu 20.04)
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • [N/A] Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@sheeldotme
Copy link

@jonringer it looks like you were the last person to commit to this file, I hope you don't mind being tagged. Do you by any chance know who might be the best person to provide feedback on this? Interested in getting this change merged.

taku0
taku0 approved these changes Feb 13, 2021
View reviewed changes
Copy link
Contributor

@taku0 taku0 left a comment

Choose a reason for hiding this comment

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

Code-wise LGTM.

@taku0
Copy link
Contributor

taku0 commented Feb 13, 2021

Please squash commits and rename to fetchgit: support passing tree hashes as "rev".

@thomasjm thomasjm force-pushed the tree-hashes branch 2 times, most recently from babf18f to 81a58ae Compare February 13, 2021 22:43
@thomasjm
Copy link
Contributor Author

Done! Also rebased on master

@thomasjm
Copy link
Contributor Author

Just checking in, is this ready to be merged?

@taku0 taku0 merged commit a964d70 into NixOS:master Feb 20, 2021
This was referenced Mar 13, 2021
Closed
Open
raboof added a commit to raboof/nixpkgs that referenced this pull request Mar 14, 2021
@raboof
linux-firmware: fix fetching from git
077f4f8 
Since NixOS#104714 using a tag as
the `rev` parameter to `fetchgit` is no longer reliable, so spell
out the rev.

Fixes NixOS#114439
@raboof raboof mentioned this pull request Mar 14, 2021
Merged
10 tasks
primeos pushed a commit that referenced this pull request Mar 14, 2021
@raboof
firmwareLinuxNonfree: fix fetching from git (#116306)
5bf3230 
Since #104714 using a tag as
the `rev` parameter to `fetchgit` is no longer reliable, so spell
out the rev.

Fixes #114439
@primeos primeos mentioned this pull request Mar 14, 2021
Open
@primeos
Copy link
Member

primeos commented Mar 14, 2021

FYI: This caused a regression that isn't the fault of this PR but it now becomes apparent that checkout_hash() is also relevant for some Git tags (which obviously fails now): #116307

(Just to link that issue here in case anyone else runs into that and finds this PR.)

@thomasjm
Copy link
Contributor Author

thomasjm commented Mar 14, 2021 •
edited

Thanks @primeos. For completeness, there have been a couple other issues mentioning regressions:

#115145
#113926

I'm not sure what the best fix is. We could revert this if it's causing major problems.

FWIW, we could restore the old behavior by handling the "$object_type" == "tag" case like you mentioned and also catching any exceptions from git cat-file. But maybe that would just be papering over the problem.

@sternenseemann sternenseemann mentioned this pull request Jun 14, 2021
Open
@johnrichardrinehart johnrichardrinehart mentioned this pull request Sep 5, 2021
Closed
@colinxs colinxs mentioned this pull request Feb 5, 2022
Open
@chkno chkno mentioned this pull request Jan 29, 2024
Merged
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@taku0 taku0 taku0 approved these changes

Assignees
No one assigned
Labels
6.topic: fetch 10.rebuild-darwin: 11-100 10.rebuild-linux: 11-100
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@thomasjm @sheeldotme @taku0 @primeos