Fix test evaluation in restrict-eval #50342
Conversation
The current behavior, breaking restrict-eval=true, had been introduced in 209f8b1. See also [1]. This change uses a hack to identify whether we're evaluating in restrict-eval, keeping the optimization when not and reverting to the old behavior when evaluating in restrict-eval. This should fix the issues that appeared at [2] and [3], that followed the change making us actually use this `nixpkgs` argument for tests since 83b27f6 [4]. CC: @roberth [1] NixOS@209f8b1acd4c#commitcomment-28419462 [2] NixOS#50233 (comment) [3] NixOS#50186 [4] NixOS#50233
|
Sanity-check: all four should eval & pass. @GrahamcOfBorg build opensmtpd |
|
Success on x86_64-linux (full log) Attempted: opensmtpd Partial log (click to expand)
|
Currently, when building NixOS from a git clone, Nix has to copy
the entire repo at >1GB into the store by default. That is not
necessary and causes a dumping large path message.
If you need the old behaviour for some reason, you will have to
specify it by passing the path to your repo explicitly as the
nixpkgs argument like this:
--arg nixpkgs '{outPath = ./.; revCount = 56789; shortRev = "gfedcba"; }'
GrahamcOfBorg
added
the
6.topic: nixos
|
Success on aarch64-linux (full log) Attempted: opensmtpd Partial log (click to expand)
|
|
Success on x86_64-linux (full log) Attempted: nixosTests.opensmtpd Partial log (click to expand)
|
|
Success on x86_64-linux (full log) Attempted: tests.opensmtpd Partial log (click to expand)
|
|
Success on x86_64-linux (full log) Attempted: opensmtpd.tests Partial log (click to expand)
|
GrahamcOfBorg
added
10.rebuild-darwin: 0
|
Unexpected error: command failed with exit code 1 on aarch64-linux (full log) Attempted: opensmtpd.tests Partial log (click to expand)
|
|
Unexpected error: command failed with exit code 1 on aarch64-linux (full log) Attempted: nixosTests.opensmtpd Partial log (click to expand)
|
|
Well… these unexpected errors sure are weird (esp. as they usually appear as “Failure on […]”), but it appears to eval correctly, so I'd guess it's unrelated to the problem addressed here. |
|
Unexpected error: command failed with exit code 1 on aarch64-linux (full log) Attempted: tests.opensmtpd Partial log (click to expand)
|
|
Ideally we would use |
|
BTW I don't understand the motivation for this change. In Hydra evaluations, we should not need the default value for the |
|
@edolstra The reason for this change is at least for Now, I hoped it was the reason why hydra appears to have no evaluations in the last 3 days on As for (by the way, just in case: I noticed hydra didn't have a successful evaluation in 3 days just earlier today, otherwise I'd likely have asked someone to backout #50233 if it took me 3 days to come up with this fix) |
|
I guess it kind of depends on what we consider more important for ofborg to test. If it's hydra then doing the same (eg. NixOS/ofborg#189) is the proper solution. |
|
Yeah, it's probably best if ofborg does what Hydra does (i.e. pass a |
|
Using --arg doesn't influence imports right? one thing we do want to verify is that expressions don't depend on |
|
@LnL7 It doesn't. Actually Hydra does add jobset inputs to |
|
Semi-relatedly: this change also unbreaks evaluation when manually running Also, it looks like this was not actually the cause of hydra not advancing, so for hydra the point here is moot. |
| { nixpkgs ? { outPath = cleanSource ./..; revCount = 130979; shortRev = "gfedcba"; } | ||
| # `revCount` and `shortRev` are placeholders, that should be filled | ||
| # automatically for releases by passing the `nixpkgs` argument | ||
| { nixpkgs ? { outPath = cleanSourceForImport ./..; revCount = 654321; shortRev = "gfedcba"; } |
There was a problem hiding this comment.
This revcount isn't a placeholder, it is updated when new channels are cut.
There was a problem hiding this comment.
@grahamc Are you sure about that? I ran git log -p nixos/release.nix before writing this comment, and it was set to 56789 since May 2014 before 18.03 cut-off (and before that it was set to 5678 since Oct 2013, and before that to a mix of 1234 for nixosSrc and 5678 for nixpkgs), and it hasn't been updated when cutting out 18.09. This is the reason why I thought @vcunat's updating it when 18.03 was cut was potentially a mistake, but…?
There was a problem hiding this comment.
@grahamc I'd like to try and move this forward. Could you either confirm, infirm or say you don't have time to evaluate my statement above? :)
There was a problem hiding this comment.
I don't have time to dive deeper, however, I thought:
Update versionSuffix in nixos/release.nix, use git log --format=%an|wc -l to get the commit count
was supposed to include updating this count. (https://nixos.org/nixos/manual/index.html#at-beta-release-time)
Maybe I'm wrong, and I trust you to do the digging, and I don't feel like I need to be part of making the decision if this is true.
|
Are there any updates on this pull request, please? |
|
Thank you for your contributions.
|
stale
bot
added
the
2.status: stale
|
Well… I've been unable to reproduce the problem listed in the top post on current master, with either of: As such, I guess this has been done again by someone in the meantime while I had completely forgotten about this PR, and I'll close this. If the original issue wasn't actually closed in the meantime, feel free to poke me and/or reopen this as a custom PR, it looks like it doesn't have too many merge conflicts even nowadays 2 years after the initial opening, so hopefully it'll stay easily un-merge-conflict-able! :) |
This will be more easily reviewed commit-by-commit.
For the
revCountreset, I'm not 100% sure of myself, but withshortRevbeing a placeholder and the previous value being56789I'm ~99% sure it was supposed to be a placeholder too.I have checked that the
--option restrict-eval truereproducers now appear to build correctly locally.Commit messages for easier reading:
release: reset
revCountto a dummy placeholderPlaceholder had been removed in 7e968a4.
Additionally, bumps the
revCountfromrelease-small.nixandrelease-combined.nixso that it's more obvious it's a placeholder.CC: @vcunat
release: do not cleanSource when evaluating in restrict-eval
The current behavior, breaking restrict-eval=true, had been introduced
in 209f8b1. See also [1].
This change uses a hack to identify whether we're evaluating in
restrict-eval, keeping the optimization when not and reverting to the
old behavior when evaluating in restrict-eval.
This should fix the issues that appeared at [2] and [3], that followed
the change making us actually use this
nixpkgsargument for testssince 83b27f6 [4].
CC: @roberth
[1] 209f8b1acd4c#commitcomment-28419462
[2] #50233 (comment)
[3] #50186
[4] #50233
Things done
sandboxinnix.confon non-NixOS)nix-shell -p nox --run "nox-review wip"./result/bin/)nix path-info -Sbefore and after)