ci/eval: make eval for non-native platforms less incorrect

[emilylange]

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

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):

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:

@@ -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:

# 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 #376770 (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.

Things done

• Built on platform(s)
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
  • sandbox = relaxed
  • sandbox = true
• Tested, as applicable:
  • NixOS test(s) (look inside nixos/tests)
  • and/or package tests
  • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
  • made sure NixOS tests are linked to the relevant packages
• Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
• Tested basic functionality of all binary files (usually in ./result/bin/)
• 25.05 Release Notes (or backporting 24.11 and 25.05 Release notes)
  • (Package updates) Added a release notes entry if the change is major or breaking
  • (Module updates) Added a release notes entry if the change is significant
  • (Module addition) Added a release notes entry if adding a new NixOS module
• Fits CONTRIBUTING.md.

[wolfgangwalther]

Seems reasonable to me.

[wolfgangwalther]

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> { }.

Could you point me to where that happens? I can't find it.

[emilylange]

Could you point me to where that happens? I can't find it.

Hm sure, I can try. If you want me to go into more detail, let me know.

nixpkgs/ci/eval/default.nix

Lines 91 to 102 in 05397ad

	nix-env -f "${nixpkgs}/pkgs/top-level/release-attrpaths-parallel.nix" \
	--option restrict-eval true \
	--option allow-import-from-derivation false \
	--query --available \
	--no-name --attr-path --out-path \
	--show-trace \
	--arg chunkSize "$chunkSize" \
	--arg myChunk "$myChunk" \
	--arg attrpathFile "${attrpathFile}" \
	--arg systems "[ \"$system\" ]" \
	--arg checkMeta ${lib.boolToString checkMeta} \
	--arg includeBroken ${lib.boolToString includeBroken} \

calls

nixpkgs/pkgs/top-level/release-attrpaths-parallel.nix

Lines 19 to 22 in 66aea4c

	unfiltered = import ./release-outpaths.nix {
	inherit path;
	inherit checkMeta includeBroken systems;
	};

which in turn calls

nixpkgs/pkgs/top-level/release-outpaths.nix

Lines 21 to 25 in 05397ad

	import (path + "/pkgs/top-level/release.nix")
	# Compromise: accuracy vs. resources needed for evaluation.
	{
	inherit attrNamesOnly;
	supportedSystems = if systems == null then [ builtins.currentSystem ] else systems;

--arg systems "[ \"$system\" ]" becomes supportedSystems passed to pkgs/top-level/release.nix.

nixpkgs/pkgs/top-level/release.nix

Lines 12 to 15 in 66aea4c

	, system ? builtins.currentSystem
	, officialRelease ? false
	# The platform doubles for which we build Nixpkgs.
	, supportedSystems ? import ../../ci/supportedSystems.nix

and there system defaults to builtins.currentSystem.

Even if multiple supportedSystems are passed to pkgs/top-level/release.nix, only a single pkgs is instantiated for system because of performance reasons.

nixpkgs/pkgs/top-level/release.nix

Lines 61 to 65 in 66aea4c

	release-lib = import ./release-lib.nix {
	inherit supportedSystems scrubJobs nixpkgsArgs system;
	};
	inherit (release-lib) mapTestOn pkgs;

nixpkgs/pkgs/top-level/release-lib.nix

Line 40 in 66aea4c

pkgs = packageSet (recursiveUpdate { inherit system; config.allowUnsupportedSystem = true; } nixpkgsArgs);

pkgs/top-level/release.nix itself solves the pkgs.hostPlatform conditionals based on the passed system and then queries the available platforms for the resulting attribute path.

Changing system ? builtins.currentSystem to system ? lib.head supportedSystems may seem like an easy alternative fix to this PR here, but is brittle in its own ways.

We could also modify the chain of imports to pass system from start to end and add that, instead of systems.

But then that doesn't fix the impure test I mentioned in my commit and opening comment.

[wolfgangwalther]

Ah, thanks. So the builtins.currentSystem part happens in the top-level/release-... code. From what you wrote in the PR description, I understood this to be somewhere in the expressions for chromedriver, electron etc. - but couldn't find it at all there.

This whole thing seems to be equivalent to "Should we run eval for the different platforms on those platforms directly?". Like, right now we run all of the 4 eval jobs on x86_64-linux. We could run them on the other 3 available GitHub runners, though - aarch64-linux, and both as -darwin as well. Would we expect eval to be any worse in that case? Surely not. Clearly, we could only benefit from having builtins.currentSystem have the right value.

If we can achieve the same with this setting and we don't need to switch to other runners (which not be available in the same number as the x86_64 runners) - even better.

So I don't see any reason why we should not to this.

[wolfgangwalther]

I made the same change locally and ran eval for all 4 systems. Nothing broken.

[nixpkgs-ci]

Successfully created backport PR for release-24.11:

• [Backport release-24.11] ci/eval: make eval for non-native platforms less incorrect #382095