This moves the diff of outpaths into the outpaths job, mainly as a
preparation to allow future improvements. For example, this will allow
running the purity release checks only on changed outpaths instead of
the whole eval.
This also removes the inefficiency introduced in the last commit about
uploading the intermediate paths twice. Now, only the diff is passed on.
Also, technically, the diff is now run in parallel across 4 jobs. This
should be *slightly* faster than before, where outpaths from all systems
were combined first and then diffed. It's probably only a few seconds,
though.
Everything is a result, especially when nix-build uses "result" as its
default output. This becomes confusing, when re-wiring the different
parts later.
Thus, consistently name those things after some of their properties and
avoid the term result.
Instead of parsing a plain text file with jq, we can make nix-env output
JSON directly, which is significantly faster.
This saves about 8 out of 10 seconds for the combine step.
compare/maintainers.nix needs to access the current checkout to check
attrpaths, but makes the mistake of using lib from that checkout as
well. All other code in ci/ uses the pinned nixpkgs instance, so
maintainers.nix should do so as well.
Previously, `eval.full` organized the results for the supported systems
in a specific layout, i.e. with a folder with one subfolder per system.
Then, `eval.combine` relied on that.
When using `eval.singleSystem` and `eval.combine` directly, the caller
was responsible to recreate the same layout. This is annoying and
error-prone to do, when downloading artifacts from CI to recreate some
steps locally.
With this change, all the artifacts can be downloaded and extracted into
the same folder - because the result from `eval.singleSystem` already
contains the <system-name>/ subfolder.
We really can't expect packages that are marked as broken to evaluate,
and *especially* not on unsupported platforms.
For context, we were attempting to eval them *past* the broken throw
previously, which caused fun side effects like [0].
When we set `includeBroken = true` before, this also included unfree
packages. Those would now be excluded, which is not what we want. Thus,
we explicitly enable them separately.
Commit by winterqt, message slightly reworded by wolfgangwalther.
[0]:
https://github.com/NixOS/nixpkgs/issues/355847#issuecomment-2878873137
Right now, there are some paths that don't even get exposed to certain
systems (notably Darwin, but some outliers exist for Linux such as the
Darwin-specific Hackage overlay) for one reason or another, usually
because of assertions like `stdenv.isLinux`. To catch these scenarios,
this change implements a way to specify the system to evaluate attrpaths
on, and makes it default to the system that we're evaluating outpaths
for.
Previously, the attrs step consisted of:
- 7s queue time
- 1m 15s run time
Only 25s of this were spent preparing the attr paths. A bit more than a
minute was just spent for queuing, checking out the repo, downloading
nix, downloading dependencies, uploading the artifacts - and then
downloading them again in the next step. All of that can be avoided if
we collect the attrs as part of the outpaths job.
By running the attrs step as part of each outpaths step the attrpaths
will be collected 4x, but:
- We save a minute for each eval run to complete.
- We save a full job, giving us more free runners and *possibly* less
queue times for other jobs in the repo.
- We reduce complexity in the workflow file.
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 75c8548d81
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 cfed9a19cb
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.
It's currently annoying to see the actual failure in the attrs step,
because `time -v` displays like 20 lines, which get repeated, therefore
requiring you to scroll up most of the time:
3429721834 (step):5:794
This commit fixes that by only displaying the most important stats, the
same ones as the chunked system-specific evals.
