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

release: access fetchGit from builtins to fix parsing w/1.11 (<1.12) #1789

Merged
merged 1 commit into from Jan 10, 2018

Conversation

dtzWill
Copy link
Member

@dtzWill dtzWill commented Jan 10, 2018

Currently nix "stable" (tested with 1.11.16) aborts when trying to process release.nix:

$ nix-instantiate --parse-only release.nix
error: undefined variable ‘fetchGit’ at /path/to/nix/src/release.nix:1:9

This PR accesses fetchGit through builtins so that release.nix parses,
and can be evaluated with 1.11 if proper nix and nixpkgs arguments are provided.

@shlevy shlevy merged commit 435ccc7 into NixOS:master Jan 10, 2018
@dtzWill dtzWill deleted the fix/eval-possible-with-1.11 branch January 10, 2018 21:46
@FRidh
Copy link
Member

FRidh commented Jan 26, 2018

Consider building the tarball, nix-build -A tarball release.nix

With this PR, the default nix ? builtins.fetchGit ./. works for 1.11, but not for 1.12 because git is missing as buildInput (see #1814). The default nixpkgs ? fetchTarball channel:nixos-17.09 does not work for 1.11, but does for 1.12.

If we can't make the default work for both cases, then I think this should be reverted.

@dtzWill
Copy link
Member Author

dtzWill commented Jan 26, 2018

Hmm. Works for me on 1.12, don't know why it wouldn't work for you.

I noticed the Nix being built in #1814 is old (cd74a55), if that's also the version you're using perhaps it's a bug that's been fixed since then?

(also I don't think release.nix is meant to actually let you build "any" nix by overriding the src argument, although it probably works it's probably best to use the release.nix corresponding to the source being built)

Anyway:

This PR was about 1.11 failing to even parse the release.nix, which is especially unfortunate. It will still fail to evaluate if you don't override src (since there's no builtins.fetchGit in 1.11) but without this fix it simply was not possible to use release.nix with 1.11--meaning the primary way to build nix 1.12 couldn't be used unless already using nix 1.12. So, let's not be too hasty about reverting please :).

Similarly 1.11 will fail unless you also override nixpkgs but that's not a change in behavior re:this PR.

@dtzWill
Copy link
Member Author

dtzWill commented Jan 26, 2018

This PR simply replaces fetchGit with builtins.fetchGit-- are these not basically equivalent, such that one requires git and the other does not?

FWIW if git needs to be an input it probably should be a nativeBuildInput.

It would be "nice" if there was a better story for use with 1.11 but since few people use release.nix directly (instead most would get it from nixpkgs which has its own 1.11-and-1.12-compatible expression) I think that's not a priority but I'm sure improvements would be welcome :).

@FRidh
Copy link
Member

FRidh commented Jan 26, 2018

Hmm. Works for me on 1.12, don't know why it wouldn't work for you.

Do you have the sandbox enabled? It fails because it cannot find git, which isn't included as a buildInput.

I noticed the Nix being built in #1814 is old (cd74a55), if that's also the version you're using perhaps it's a bug that's been fixed since then?

It has not been fixed.

This PR was about 1.11 failing to even parse the release.nix, which is especially unfortunate.

That is indeed unfortunate. Aside from adding git as input the only option then is to always having to pass in explicit arguments.

@dtzWill
Copy link
Member Author

dtzWill commented Jan 26, 2018

Do you have the sandbox enabled? It fails because it cannot find git, which isn't included as a buildInput.

yes I do. And I build Nix all the time on multiple machines all using sandbox ...

Something strange is going on--are you saying that if you revert the commit in this PR your observed behavior re:errors in #1814 is changed?

(and why is this somehow also causing you to have a missing json.h fle?? Really sounds like you're just using the wrong release.nix for your nix version?)

I'd be less surprised if you found problems before/after 6d80870 , is that what you meant?

Also looking at commit history around that point only reinforces the idea that your problems are mismatched release.nix for the nix source (changes to the json bits, adding git and mercurial to satisfy check reqs)-- all of which explain your problem and have nothing to do with this PR.

Do you see the problem if you use the release.nix from the source you're building, or only when they're mismatched?

@FRidh
Copy link
Member

FRidh commented Jan 26, 2018

I don't have access to the previous machine currently, but am now on another one, that runs nix (Nix) 1.12pre5788_e3013543

Something weird is going on, and I think it is fetchGit.

git fetch upstream
git checkout 98f3c75a0e16f5aaaecb25a46f988580efb04d19   # current master
git clean -df .
nix-collect-garbage -d
nix-build -A tarball release.nix

produces

autoreconf: Entering directory `.'
autoreconf: configure.ac: not using Gettext
autoreconf: running: aclocal --force
autoreconf: configure.ac: tracing
autoreconf: configure.ac: not using Libtool
autoreconf: running: /nix/store/p305vmh7dm30ab904k72h670178bk9hx-autoconf-2.69/bin/autoconf --force
autoreconf: running: /nix/store/p305vmh7dm30ab904k72h670178bk9hx-autoconf-2.69/bin/autoheader --force
autoreconf: configure.ac: not using Automake
autoreconf: Leaving directory `.'
configuring
autoreconf: 'configure.ac' or 'configure.in' is required
build time elapsed:  0m0.063s 0m0.043s 0m1.195s 0m0.070s
builder for '/nix/store/331my5s8g9fl0d8smbdwp531n92ac6ar-nix-tarball-1.12pre4840_ae8884b.drv' failed with exit code 1
error: build of '/nix/store/331my5s8g9fl0d8smbdwp531n92ac6ar-nix-tarball-1.12pre4840_ae8884b.drv' failed

Alright. Now, let's revert this change:

git revert 435ccc798077e6291fd34fd1720c6abcf3521557
git clean -df .
nix-collect-garbage -d
nix-build -A tarball release.nix

And I get the same error. So, it's unrelated, right?

What if we reset the change, i.e., we keep the change, but not the commit:

git reset --soft HEAD~1
git clean -df .     # the change is still staging
nix-collect-garbage -d
nix-build -A tarball release.nix

And it builds!

Note that using master at 98f3c75 and cherry-picking my change from #1814 did not make it build on this machine.

However....when I have the change in there (adding git to buildInputs) but not committed it would build. It sounds very much like a dirty tree, but, as before, if I stage the change, and clean the repo, it builds.

@FRidh
Copy link
Member

FRidh commented Jan 26, 2018

I will try (later) building with a newer Nix version.

@dtzWill
Copy link
Member Author

dtzWill commented Jan 29, 2018

That is very strange!! I'm sorry if this was caused by this PR, but regardless this is rather unexpected.

Minor observation, the failing log looks like:

builder for '/nix/store/331my5s8g9fl0d8smbdwp531n92ac6ar-nix-tarball-1.12pre4840_ae8884b.drv' failed with exit code 1

Which I'll note has the git revision of ae8884b (and "revision count" is about 1000 revisions older than master revision used 98f3c75.

Which leads me to believe you maybe are being bitten by the issue fixed by #1755 or a related one-- namely, if you used fetchGit on a clean git repository 'master' was used regardless of current checked out branch.
This is a subtle but significant breakage that leads to this sort of confusion (I say this because a very confused debugging session of my own is what led me to the PR :))...

I don't suppose your local 'master' reference points to ae8884b?

Related: what's the version of nix being used to evaluate all of this? And is daemon the same version?

Oh-- your final comment really makes this sound like the issue fixed in that PR. Fingers crossed!

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.

None yet

3 participants