Skip to content

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

Merged
taku0 merged 1 commit intoNixOS:masterfrom
codedownio:tree-hashes
Feb 20, 2021
Merged

Extend nix-prefetch-git to support passing tree hashes as "rev"#104714
taku0 merged 1 commit intoNixOS:masterfrom
codedownio:tree-hashes

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.

@ofborg ofborg bot added 6.topic: fetch Fetchers (e.g. fetchgit, fetchsvn, ...) 10.rebuild-darwin: 11-100 This PR causes between 11 and 100 packages to rebuild on Darwin. 10.rebuild-linux: 11-100 This PR causes between 11 and 100 packages to rebuild on Linux. labels Nov 24, 2020
@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.

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
raboof added a commit to raboof/nixpkgs that referenced this pull request Mar 14, 2021
Since NixOS#104714 using a tag as
the `rev` parameter to `fetchgit` is no longer reliable, so spell
out the rev.

Fixes NixOS#114439
primeos pushed a commit that referenced this pull request Mar 14, 2021
Since #104714 using a tag as
the `rev` parameter to `fetchgit` is no longer reliable, so spell
out the rev.

Fixes #114439
@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

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.

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

Labels

6.topic: fetch Fetchers (e.g. fetchgit, fetchsvn, ...) 10.rebuild-darwin: 11-100 This PR causes between 11 and 100 packages to rebuild on Darwin. 10.rebuild-linux: 11-100 This PR causes between 11 and 100 packages to rebuild on Linux.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants