We already have treefmt running for nixfmt, so it's easy to just add
another formatter to it. This gives a much better UX, because all
formatting errors are reported through the same channel.
It also saves us one CI job, which takes most of the time to just set up
the machine, clone the repo and download Nix - while doing a minimum of
actual work.
Total execution time for treefmt is ~10% slower:
- 38s only nixfmt
- 43s nixfmt + editorconfig-checker
We need to pass through the maintainers and teams positions from the
original meta so pings work correctly, since check-meta clobbers the
original attribute positions in them.
Tested with `maintainers/scripts/get-maintainer-pings-between.sh` on a
handful of major packages maintained by both individuals and teams.
Sometimes it is quite useful to output names instead of GitHub IDs, e.g.
for maintainer scripts that show you who you would ping. Add this as an
option, but keep the existing default.
Changes the Nix format checking workflow to now strictly enforce
formatting of all Nix files using the treefmt setup introduced
in the pre-previous commit.
This is in [accordance with the approved RFC 166](https://github.com/NixOS/rfcs/blob/master/rfcs/0166-nix-formatting.md#reformat-nixpkgs).
Note that the "skip treewide" thing is no longer necessary, already
before, because there's nothing that would fail for treewide changes.
Previously the problem was that the GitHub API would be bombarded.
Introduces treefmt with a simple nixfmt-rfc-style configuration to
format all Nix files.
This is only practically usable with the following commit that formats
all files accordingly.
This suddenly appeared after updating Nix to v26, which then complained:
… while calling the 'fromJSON' builtin
at
/home/runner/work/nixpkgs/nixpkgs/target/ci/eval/compare/default.nix:74:19:
73|
74| getAttrs = dir: builtins.fromJSON (builtins.readFile
"${dir}/outpaths.json");
| ^
75| beforeAttrs = getAttrs beforeResultDir;
… while evaluating the first argument passed to builtins.fromJSON
error: the string '{
"AMB-plugins.aarch64-linux": {
"out":
"/nix/store/faw59ba5p6h4b177n8q2ilb3hlm7xlc2-AMB-plugins-0.8.1"
},
....
"zzuf.aarch64-linux": {
"out": "/nix/store/bqvm1h7jfd8smgnjc1v1gpmbwdgvwy5g-zzuf-0.15"
},
"zzuf.x86_64-linux": {
"out": "/nix/store/6qs4lnmzn1qlr3smqqxnmhnrcdcfiv6a-zzuf-0.15"
}
}
' is not allowed to refer to a store path (such as
'134m2q047vsr9miwh5l227j7sh9jb130-jq-1.7.1-bin')
By discard the unsafe string context, we explicitly allow loading those
store paths. It's unclear why this blew up now, especially because I was
not possible to consistently replicate this locally, so far.
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.
Various improvements such as:
1. Avoiding deduplications when there can't be any duplicates
2. Avoiding O(n^2) deduplications
3. Using builtins.any to avoid list allocations
4. Using builtins.concatMap instead of lib.flatten when it's known that there's only one level of nesting
5. Using builtins.groupBy instead of folding with an accumulator
In particular 5. should fix CI exceeding the stack size on staging: https://github.com/NixOS/nixpkgs/actions/runs/12989244871/job/36240781244?pr=377253
While 2. in particular should make CI a lot faster.
