From 657c689842d9c57d9e3ce7a81d24cabf2e46445b Mon Sep 17 00:00:00 2001
From: emilylange <git@emilylange.de>
Date: Sun, 2 Feb 2025 21:04:08 +0100
Subject: [PATCH] ci/eval: make eval for non-native platforms less incorrect

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 75c8548d8118c31509b89ffd7df6c322b94017dd
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 cfed9a19cbc98d12c0167f69238673f4ed89f798
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.
---
 ci/eval/default.nix | 1 +
 1 file changed, 1 insertion(+)

diff --git a/ci/eval/default.nix b/ci/eval/default.nix
index 927dec5a908d..d5f4419598b4 100644
--- a/ci/eval/default.nix
+++ b/ci/eval/default.nix
@@ -89,6 +89,7 @@ let
         set +e
         command time -f "Chunk $myChunk on $system done [%MKB max resident, %Es elapsed] %C" \
           nix-env -f "${nixpkgs}/pkgs/top-level/release-attrpaths-parallel.nix" \
+          --eval-system "$system" \
           --option restrict-eval true \
           --option allow-import-from-derivation false \
           --query --available \