nix-prefetch-git: don't fetch everything when given a hash #130040
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
It's hugely inefficient as we can't use shallow cloning (--depth=1). This has been tested and adapted for quite a few hosts fetchgit is used on in Nixpkgs. For those where fetching the hash directly doesn't work (most notably git.savannah.gnu.org), we simply fall back to the old method.
|
Result of 2 packages failed to build:12 packages built successfully:
Note that build failures may predate this PR, and could be nondeterministic or hardware dependent. Result of 2 packages failed to build:13 packages built successfully:
Note that build failures may predate this PR, and could be nondeterministic or hardware dependent. |
|
cc @Mic92 @infinisil |
|
@Atemu I can also confirm this would significantly decrease the download requirements for various repos in the Thanks! |
|
Result of 2 packages failed to build:
13 packages built:
|
|
Thanks! |
10 tasks
13 tasks
|
11df411 changes the hash of some fetches, causing hash-mismatch failures. Example affected package: Before this change: After this change: Diff between Seems like this is intentional and the thing to do is to go update the other packages' fetch hashes like #158043 ? |
|
No, I think the problem is that the .git dir is included in the output. I'd consider that a bug though, the hash should not depend on some internal data format. I'm not even sure it stays stable anyways? I guess we only fetch a specific rev but I'm not sure git will always pack it in the exact same way there. We can disable this optimisation when the git dir is included in the output though. |
also this optimization ignores |
|
@jbgi could you open a more detailed bug report on that? |
|
yes. ☝️ |
|
This should maybe have updated the comment in Maybe something like: --- a/pkgs/build-support/fetchgit/default.nix
+++ b/pkgs/build-support/fetchgit/default.nix
@@ -32,17 +32,20 @@ in
}:
/* NOTE:
- fetchgit has one problem: git fetch only works for refs.
- This is because fetching arbitrary (maybe dangling) commits may be a security risk
- and checking whether a commit belongs to a ref is expensive. This may
- change in the future when some caching is added to git (?)
+ fetchgit had one problem: git fetch only worked for refs.
+ This was because fetching arbitrary (maybe dangling) commits might have been a security risk
+ and checking whether a commit belongs to a ref was expensive. This changed in 2015
+ with the release of git 2.5.0 https://github.com/git/git/blob/master/Documentation/RelNotes/2.5.0.txt#L106
Usually refs are either tags (refs/tags/*) or branches (refs/heads/*)
Cloning branches will make the hash check fail when there is an update.
But not all patches we want can be accessed by tags.
- The workaround is getting the last n commits so that it's likely that they
+ The workaround was getting the last n commits so that it it's likely that they
still contain the hash we want.
+ As the new behaviour is optional, we try the new one, but fall back.
+ This change was introduced in 11df41199ba8a1497d8530dbf478ef10147a93bb.
+
for now : increase depth iteratively (TODO)
real fix: ask git folks to add a |
10 tasks
|
This should either have been fixed or reverted when such regressions were discovered.. (Edit: Oh, it was discovered rather late.. No tests..?) Edit: #252865 |
12 tasks
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/deepclone-in-fetchgit-does-not-actually-yield-a-deep-clone/15028/5 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation for this change
It's hugely inefficient as we can't use shallow cloning (--depth=1).
This has been tested and adapted for quite a few hosts fetchgit is used on in
Nixpkgs. For those where fetching the hash directly doesn't work (most notably
git.savannah.gnu.org), we simply fall back to the old method.
@danielfullmer this might be especially interesting for robotnix since all its sources are
fetchgits with hashes asrevand most have huge histories we don't need, right? I haven't done in-depth benchmarking but fetching vendor/oneplus went from 5GiB to 1.26GiB and ~4min to 2min.Things done
sandboxinnix.confon non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"./result/bin/)