Disable checkMeta by default again

[Profpatsch]

This caused too many downstream projects to break, so we are reverting this change for now, until further transition fixes are in place.

See discussion in #191171

This reverts part of 6762de9

Description of changes

Things done

• Built on platform(s)
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
• 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/)
• 22.11 Release Notes (or backporting 22.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
  • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
• Fits CONTRIBUTING.md.

[piegamesde]

(retracted)

[Profpatsch]

This is a classic false dilemma fallacy.

nixpkgs uses the PR workflow for exactly this reason, that we can have checks that we only need to do before merging a change, not every time we run the code.

[Profpatsch]

Especially seeing that the checks in master are not recursive, I see this as an argument in bad faith.

[sternenseemann]

A type check that is only performed in some glitchy CI¹ – and doing a shallow type check at that anyways – is a farce

ofborg evaluates every package in the release.nix jobset with checkMeta set to true. I'm not sure what you mean by shallow, but there is no meaningful difference between what ofborg does and a normal user with checkMeta = true would do.

Not sure if “our CI doesn't work and we need to use downstream users as CI” is really the right argument against this change?

[mweinelt]

Wouldn't it be sufficient to allow extra arguments in meta and enforce checks only on well-defined keys used throughout nixpkgs?

[Profpatsch]

@mweinelt yes, that could be a good step after setting the flag to "warn" instead of true: #191171 (comment)

[Profpatsch]

For now I’d revert the change to unbreak downstream.

[alyssais]

Wouldn't it be sufficient to allow extra arguments in meta and enforce checks only on well-defined keys used throughout nixpkgs?

Upon reflection I don't think this is a good idea. The main use I've had of the meta checks in the past has actually been catching misspelled keys. So I think we really need a config option with an allow list of unrecognized meta checks.

[alyssais]

Since it was brought up in the original PR a couple of times, and it was very easy to do, I ran a benchmark eval of nixosTests.simple (since evaluating a NixOS system felt like a good realistic test) on 198b38f (master from a few hours ago) with and without checkMeta enabled.

Benchmark results

% hyperfine -w 1 -m 10 'nix-instantiate -E "with import ./. { config.checkMeta = true; }; nixosTests.simple"' 'nix-instantiate -E "with import ./. { config.checkMeta = false; }; nixosTests.simple"'
Benchmark 1: nix-instantiate -E "with import ./. { config.checkMeta = true; }; nixosTests.simple"
  Time (mean ± σ):      5.367 s ±  0.255 s    [User: 3.855 s, System: 0.776 s]
  Range (min … max):    4.872 s …  5.703 s    10 runs

Benchmark 2: nix-instantiate -E "with import ./. { config.checkMeta = false; }; nixosTests.simple"
  Time (mean ± σ):      5.396 s ±  0.381 s    [User: 3.900 s, System: 0.767 s]
  Range (min … max):    4.875 s …  6.084 s    10 runs

Summary
  'nix-instantiate -E "with import ./. { config.checkMeta = true; }; nixosTests.simple"' ran
    1.01 ± 0.09 times faster than 'nix-instantiate -E "with import ./. { config.checkMeta = false; }; nixosTests.simple"'
hyperfine -w 1 -m 10    85,95s user 17,11s system 86% cpu 1:59,02 total

It was slightly faster with meta checks enabled, but that effect was dwarfed by uncertainty. Anyway, this suggests eval time is not meaningfully affected by this change.

[Profpatsch]

fwiw:

> bench --output bench.html 'nix-instantiate -E "with import ./. { config.checkMeta = false; }; nixosTests.simple"' 'nix-instantiate -E "with im
port ./. { config.checkMeta = true; }; nixosTests.simple"'
benchmarking bench/nix-instantiate -E "with import ./. { config.checkMeta = false; }; nixosTests.simple"
time                 3.814 s    (3.763 s .. 3.868 s)
                     1.000 R²   (1.000 R² .. 1.000 R²)
mean                 3.693 s    (3.617 s .. 3.733 s)
std dev              72.52 ms   (31.42 ms .. 91.44 ms)
variance introduced by outliers: 19% (moderately inflated)

benchmarking bench/nix-instantiate -E "with import ./. { config.checkMeta = true; }; nixosTests.simple"
time                 3.562 s    (3.377 s .. 3.946 s)
                     0.999 R²   (0.997 R² .. 1.000 R²)
mean                 3.742 s    (3.666 s .. 3.821 s)
std dev              99.40 ms   (42.03 ms .. 133.2 ms)
variance introduced by outliers: 19% (moderately inflated)

I get a small median increase by 0.05s per evaluation.

[mweinelt]

For me it's also slightly faster with checkMeta enabled. About 0.02s rounded up. Same method used as @Profpatsch.

output

❯ bench --output bench.html 'nix-instantiate -E "with import ./. { config.checkMeta = false; }; nixosTests.simple"' 'nix-instantiate -E "with import ./. { config.checkMeta = true; }; nixosTests.simple"'
benchmarking bench/nix-instantiate -E "with import ./. { config.checkMeta = false; }; nixosTests.simple"
time                 2.274 s    (2.231 s .. 2.340 s)
                     1.000 R²   (1.000 R² .. 1.000 R²)
mean                 2.316 s    (2.295 s .. 2.327 s)
std dev              22.30 ms   (17.96 ms .. 25.46 ms)
variance introduced by outliers: 19% (moderately inflated)
benchmarking bench/nix-instantiate -E "with import ./. { config.checkMeta = true; }; nixosTests.simple"

time                 2.283 s    (2.267 s .. 2.314 s)

1.000 R²   (1.000 R² .. 1.000 R²)

mean                 2.297 s    (2.289 s .. 2.312 s)

std dev              14.35 ms   (113.2 μs .. 18.86 ms)

variance introduced by outliers: 19% (moderately inflated)

[Profpatsch]

Here’s my results for the deep check via evalModules from commit 26328a694200e2eb370d27b219b2c96f470d5ac6

[Profpatsch]

Err, nevermind, I think my benchmarks on 26328a694200e2eb370d27b219b2c96f470d5ac6 failed due to the fact that it does not actually check the meta attributes, at least I could not get it to fail with unknown extra meta attributes.

So please disregard.

[Profpatsch]

Ah, I missed the totally obvious fact that 26328a694200e2eb370d27b219b2c96f470d5ac6 pushes eval time for nixosTests.simple from about 3 seconds to 7 seconds, lol.

[piegamesde]

This discussion has totally gotten out of hand, both in content and in tone. I am going to merge this now, so that we may start afresh discussing more proper solutions for this problem.

@Profpatsch I'd appreciate if you abstained from participating in any further discussions of this topic for a while.

[Profpatsch]

This discussion has totally gotten out of hand, both in content and in tone. I am going to merge this now, so that we may start afresh discussing more proper solutions for this problem.

@Profpatsch I'd appreciate if you abstained from participating in any further discussions of this topic for a while.

I don’t think your reaction to all this was at all appropriate.

I have to admit that I am a little annoyed that you are insinuating I am the source for this “discussion getting out of hand” when

a) you ignored my -2 on the original PR and merged anyway
b) the discussion never got out of hand, there was objectively phrased critique which was ignored.

You cannot assume drive-by commits on central nixpkgs features can just be merged with very little work, and ignoring requests for benchmarking & verifying downstream breakage is certainly not the way to go.

[Profpatsch]

@piegamesde I’m a little confused by your laugh-emoji reaction. What is it you want to say?