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

fetchgit: remove only .git folder #15469

Merged
merged 1 commit into from
May 16, 2016
Merged

fetchgit: remove only .git folder #15469

merged 1 commit into from
May 16, 2016

Conversation

domenkozar
Copy link
Member

See commit for explanation

cc @rbvermaa

Source of this change goes back to 2009 and original version of
fetchgit at 205fb0c.

The nondeterminism is really caused by changing .git so leave other
files alone as they might be interesting.

Note: this causes a hash mismatch with Hydra's version of Git Plugin
which we should fix to comply.
@mention-bot
Copy link

By analyzing the blame information on this pull request, we identified @timbertson, @bjornfor and @nbp to be potential reviewers

@dezgeg
Copy link
Contributor

dezgeg commented May 16, 2016

Doesn't this potentially change some hashes?

@domenkozar
Copy link
Member Author

Yes, but there is no way around really.

On Mon, May 16, 2016, 16:06 Tuomas Tynkkynen notifications@github.com
wrote:

Doesn't this potentially change some hashes?


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#15469 (comment)

@dezgeg
Copy link
Contributor

dezgeg commented May 16, 2016

Well, shouldn't they be changed via some scripts at the same time or builds will start failing left and right?

@fpletz fpletz deleted the fetchgit branch May 16, 2016 21:45
@domenkozar
Copy link
Member Author

domenkozar commented May 17, 2016

There are 1200 invocations, we should probably change those that fail now indeed.

@dezgeg
Copy link
Contributor

dezgeg commented May 17, 2016

Here's something hacky to try to detect them (I get only ~800 hits, though): https://gist.github.com/dezgeg/94b7895fb79bbe3c32cc9903956690cc

Run like: nix-instantiate --eval --strict --json ./find-fetchgits.nix --show-trace

@domenkozar
Copy link
Member Author

@dezgeg nice! I'll give it a run.

@domenkozar
Copy link
Member Author

@dezgeg this doesn't build the derivations. Just evaluating won't be enough

@dezgeg
Copy link
Contributor

dezgeg commented May 17, 2016

Yes, but you get the attr path for each thing you need to call nix-build --hash to calculate the new hash.

@domenkozar
Copy link
Member Author

domenkozar commented May 17, 2016

It's not useful because --hash just uses the substitute (which uses fixed hash which might be broken):

$ nix-build -A w3m.src --hash 
/nix/store/ih4fpapyy982jig4kj24x0rd677jmd24-w3m-e0b6e02

@domenkozar
Copy link
Member Author

We could use that information to run nix-prefetch-git before/after the merge.

@dezgeg
Copy link
Contributor

dezgeg commented May 17, 2016

Well, that's not very useful indeed... But nix-build --check does work.

@dezgeg
Copy link
Contributor

dezgeg commented May 17, 2016

I wouldn't use nix-prefetch-git for this, it's a mess to get all the keepDotGit, includeSubmodules etc. in sync.

@domenkozar
Copy link
Member Author

gnulib should fail right?

$ nix-build -A gnulib.src --hash 
/nix/store/7m5gabs2qqzal8f08klrqqbrcp00qm19-gnulib-92b60e6

@dezgeg
Copy link
Contributor

dezgeg commented May 17, 2016

It does:

» nix-build -A gnulib.src --hash --check
checking path(s) ‘/nix/store/7m5gabs2qqzal8f08klrqqbrcp00qm19-gnulib-92b60e6’
exporting http://git.savannah.gnu.org/r/gnulib.git (rev 92b60e61666f008385d9b7f7443da17c7a44d1b1) into /nix/store/0n0a49xpjzvjsfipzr4azwyrx8l3qwfc-gnulib-92b60e6
Initialized empty Git repository in /nix/store/0n0a49xpjzvjsfipzr4azwyrx8l3qwfc-gnulib-92b60e6/.git/
remote: Counting objects: 166900, done.        
remote: Compressing objects: 100% (24346/24346), done.        
remote: Total 166900 (delta 142542), reused 166827 (delta 142496)        
Receiving objects: 100% (166900/166900), 31.47 MiB | 2.00 MiB/s, done.
Resolving deltas: 100% (142542/142542), done.
From http://git.savannah.gnu.org/r/gnulib
 * [new branch]      EMACS_21_1_RC -> origin/EMACS_21_1_RC
<snip>
Switched to a new branch 'fetchgit'
removing `.git'...
warning: rewriting hashes in ‘/nix/store/7m5gabs2qqzal8f08klrqqbrcp00qm19-gnulib-92b60e6’; cross fingers
output path ‘/nix/store/7m5gabs2qqzal8f08klrqqbrcp00qm19-gnulib-92b60e6’ has r:sha256 hash ‘0sa1dndvaxhw0zyc19al5cmpgzlwnnznjz89lw1b4vj3xn7avjnr’ when ‘0xpxq8vqdl0niib961dnsrgjq6kbpyap6nnydzp15dvzfhzgz189’ was expected
error: build of ‘/nix/store/wdn8nk25l0dczjnwlr67f2yslzvij719-gnulib-92b60e6.drv’ failed

@domenkozar
Copy link
Member Author

Ah!

@dezgeg
Copy link
Contributor

dezgeg commented May 17, 2016

BTW, the nix file I posted doesn't locate packages that aren't included in the channel. I suppose temporarily sprinkling recurseIntoAttrs a bit in all-packages.nix would help. I think in addition to the ones in ccd1029, melpa/emacs packages are not included.

vcunat added a commit that referenced this pull request May 19, 2016
and also whitespace. This is probably due to #15469.
@@ -286,7 +286,7 @@ _clone_user_rev() {
eval "$NIX_PREFETCH_GIT_CHECKOUT_HOOK"
if test -z "$leaveDotGit"; then
echo "removing \`.git'..." >&2
find "$dir" -name .git\* -print0 | xargs -0 rm -rf
Copy link
Member

Choose a reason for hiding this comment

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

maybe add -type d too ?

Copy link

@googl googl Jun 3, 2016

Choose a reason for hiding this comment

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

-type d -delete would be more beneficial than piping to xargs and rm -rf, yes?

find "$dir" -name .git -type d -delete

@groxxda
Copy link
Contributor

groxxda commented May 19, 2016

Building upon @dezgeg s expression I produced a list of the hashes
https://gist.github.com/groxxda/0f5ff6e5f905534acd9690c42a0d9b17

I added to pkgs/build-support/fetchgit/default.nix:
meta.passthru._debugGitHashes = { inherit url rev md5 sha256 leaveDotGit fetchSubmodules deepClone branchName name; };

and altered find-fetchgits.nix to check for this variable and output a nix-prefetch-git command based upon them: https://gist.github.com/groxxda/c205a41f5e70edac3c1bd731ab943d36

then some shell hacking to fetch everything and generate the output.

If it's of use, I'll post the shell snippets so someone else can run everything and we can compare the hashes. A simple sed s/old/new/ on the nixpkgs repo should then be enough to update the hashes.

@domenkozar
Copy link
Member Author

@groxxda let's do it :)

@groxxda
Copy link
Contributor

groxxda commented May 19, 2016

https://gist.github.com/groxxda/669e350b5494a94e093d0262221bf3e3 ugh that's some ugly code ☺️ hope it works for you @domenkozar

We still miss a good amount of invocations as dezgeg pointed out.. Also no vim plugins picked up 😞

I opened an issue for the packages that have a src that can't be fetched: #15558

@copumpkin
Copy link
Member

@groxxda that's a nice script. Also seems like it could be nice to figure out which fetchgit invocations could be replaced by fetchFromGitHub (I expect many of them)

@groxxda
Copy link
Contributor

groxxda commented May 19, 2016

👍 replacing fetchgit with fetchFromGitHub - now would be the perfect time
Getting the replace right would be a bit harder though and we still lack a nix-prefetch-fromgithub so I don't know an easy way to automatically get the hashes (maybe generate the fetchFromGitHub derivation with a fake hash, build it and parse the error to get the real hash?)

@vcunat
Copy link
Member

vcunat commented May 19, 2016

@groxxda: there's this new command nix-prefetch-url -A hello.src that works on anything.

@groxxda
Copy link
Contributor

groxxda commented May 19, 2016

Thanks @vcunat
I took a look at replacing the github fetchgits but theres another problem: fetchgit defaults to fetchSubmodules = true, and only 3 fetchgits set this to false. Since git-archive (and thus the github archive api) only pack the main repository without submodules we may run into problems with a mass-replace. We would have to check the repository if it contains submodules and only if it doesn't we can replace with fetchFromGitHub..

Edit: just noticed #15559 addresses this issue 👏

@domenkozar
Copy link
Member Author

There might be another change needed to fetchgit (remove the current naming of produce .drv so it matches new builtins.fetchgit).

Should we revert this and do both changes at the same time together with hash updates?

@groxxda
Copy link
Contributor

groxxda commented May 19, 2016

Is the builtin meant to replace pkgs.fetchgit some day? Then it will have to support submodules as well I guess.
In my opinion naming after url & rev is good, even if it triggers unneeded building when repositories are renamed. So the primop should be adjusted accordingly.

@domenkozar
Copy link
Member Author

No, builtins.fetchgit (in current Nix master branch) fetches from git without checking hash. Useful for CI/Hydra and such. pkgs.fetchgit requires a hash, but it would be really useful if those those functions would yield the same hash for same git revision

@vcunat
Copy link
Member

vcunat commented May 19, 2016

Could we not use the builtin to fetch without checking into $out... and finishing the derivation makes nix check the recursive hash? (I'm just guessing.)

@garbas
Copy link
Member

garbas commented May 20, 2016

vimplugins hashes fixed

@dezgeg
Copy link
Contributor

dezgeg commented Jun 3, 2016

I pushed https://git.io/voeqV which is a superset of https://gist.githubusercontent.com/groxxda/0f5ff6e5f905534acd9690c42a0d9b17/raw/0e289d419c133542ceca9ca0ed97300b0dbb77fa/broken-git-hashes.json. I don't think that still catched them all, though.

tomjridge added a commit to sibylfs/sibylfs_src that referenced this pull request Jul 14, 2016
domenkozar pushed a commit that referenced this pull request Aug 16, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants