[Backport release-24.11] ci/eval: make eval for non-native platforms less incorrect #382095
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
![nixpkgs-ci[bot]](https://avatars.githubusercontent.com/u/487568?s=60&v=4){kind=small}
We commonly use platform-dependent conditional patterns like
`lib.meta.availableOn stdenv.hostPlatform` and `stdenv.hostPlatform.isLinux`
to enable different features in a given derivation or to evaluate
completely different derivations based on the platform.
For example, source builds of a given derivation may only be available
on linux but not on darwin. The use of such conditionals allow us to
fall back to patched binaries on darwin instead.
In `chromedriver` (pkgs/development/tools/selenium/chromedriver/default.nix), we use
~~~nix
if lib.meta.availableOn stdenv.hostPlatform chromium then
callPackage ./source.nix { }
else
callPackage ./binary.nix { }
~~~
To provide some context, `chromedriver` source builds are based on `chromium.mkDerivation`
and `chromium` is limited to `lib.platforms.linux`.
Based on the same `chromium.mkDerivation`, we also do source builds for
`electron` (pkgs/top-level/all-packages.nix):
~~~nix
electron_33 = if lib.meta.availableOn stdenv.hostPlatform electron-source.electron_33 then electron-source.electron_33 else electron_33-bin;
electron_34 = electron_34-bin;
electron = electron_34;
~~~
And finally, the top-level `jdk` (Java) attribute has a lot of
indirection, but eventually also boils down to `stdenv.hostPlatform.isLinux`
for source builds and binaries for x86_64-darwin and aarch64-darwin.
A surprising amount of electron and jdk consumers use variations of
`meta.platforms = electron.meta.platforms` in their own meta block.
Due to internal implementation details, the conditionals in those
top-level attributes like `chromedriver`, `electron` and `jdk` are
evaluated based on the value from `builtins.currentSystem` and not the
system passed to `import <nixpkgs> { }`.
This then causes `chromedriver`, `electron`, `jdk` and all dependents
that inherit those `meta.platforms` to appear only available on linux
despite also being available on darwin. Hydra is affected similarly, but
it's a lot more nuanced and in practice not actually *that* bad.
The addition of `--eval-system` ensures that `builtins.currentSystem`
matches the requested platform.
As a bonus, this also fixes the store paths of an impure test that
should probably be made pure:
~~~diff
@@ -885069,13 +886119,13 @@
"out": "/nix/store/lb2500hc69czy4sfga9mbh2k679cr1rp-test-compressDrv"
},
"tests.config.allowPkgsInPermittedInsecurePackages.aarch64-darwin": {
- "out": "/nix/store/0l5h8svrpzwymq35mnpvx82gyc7nf8s4-hello-2.12.1"
+ "out": "/nix/store/v1zjb688mp4y2132b6chii43d5kkxnpa-hello-2.12.1"
},
"tests.config.allowPkgsInPermittedInsecurePackages.aarch64-linux": {
- "out": "/nix/store/0l5h8svrpzwymq35mnpvx82gyc7nf8s4-hello-2.12.1"
+ "out": "/nix/store/hb21z2zdk03dwygsw5lvpa8zc3fbr500-hello-2.12.1"
},
"tests.config.allowPkgsInPermittedInsecurePackages.x86_64-darwin": {
- "out": "/nix/store/0l5h8svrpzwymq35mnpvx82gyc7nf8s4-hello-2.12.1"
+ "out": "/nix/store/gljdqsf0mxv1j8zb04phx9ws09pp7z3l-hello-2.12.1"
},
"tests.config.allowPkgsInPermittedInsecurePackages.x86_64-linux": {
"out": "/nix/store/0l5h8svrpzwymq35mnpvx82gyc7nf8s4-hello-2.12.1"
~~~
Diff stats between two full evals based on 75c8548
with and without this fix on x86_64-linux:
~~~bash
# git diff --no-index --stat /nix/store/659l3xp78255wx7abbahggsnrlj3a1la-combined-result/outpaths.json /nix/store/4fhlq4g5qa65cxbibskq9pma40zigrx7-combined-result/outpaths.json
/nix/store/{659l3xp78255wx7abbahggsnrlj3a1la-combined-result => 4fhlq4g5qa65cxbibskq9pma40zigrx7-combined-result}/outpaths.json | 1416 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 1405 insertions(+), 11 deletions(-)
~~~
The full diff is available as a gist at <https://gist.github.com/emilylange/d40c50031fc332bbcca133ad56d224f6>.
When we added `electron_34` only as binary instead of the usual source
on linux with binary fallback in cfed9a1
and made the unversioned `electron` top-level point to the newly added
`electron_34` instead of `electron_33`, the GitHub workflow suddenly
reported 20 new packages. Of those 20 reported packages, 17 where
false-positives caused by dropping the wrongly evaluated conditional.
(cherry picked from commit 657c689)
13 tasks
github-actions
bot
added
the
6.topic: continuous integration
nix-owners
bot
requested review from
philiptaron,
risicle,
LeSuisse,
mweinelt and
infinisil
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.
Bot-based backport to
release-24.11, triggered by a label in #378922.