Add ratchet checks for __structuredAttrs and strictDeps#203
Conversation
|
At the risk of scope creep: would this infrastructure change make it possible to also introduce a ratchet for environment variables outside of |
|
So far this infra is specific for booleans. I understand your desire, but I'm worried that finding newly added environment variables would be quite difficult. |
|
I don't expect it to happen very often (although anecdotally I did catch one instance in review already today) and I agree it's difficult to find out the right way to find them. I just mentioned it on the off chance that you would have a clever idea to add this without too much hassle. When |
|
@mdaniels5757, @philiptaron sorry for the ping, but I'd like to get some input on this and would greatly appreciate if somebody could help push this forward. |
|
I hope to give this some attention this coming week. I want to get to zero on the PRs in this repository one way or the other. |
There was a problem hiding this comment.
Thanks for the poke! This looks great!
A couple of things I noticed:
- I think the wording for the new package problems didn't convey that unset is equivalent to false here. I've included two patches (suitable for autosquashing, if you like them) below, and threw in some extra tests for good measure.
- The one thing I commented on in eval.rs.
Details
From 3f2e031f7246e29cd0a6144d56b80cd7d3690aae Mon Sep 17 00:00:00 2001
From: Michael Daniels <mdaniels5757@gmail.com>
Date: Sun, 22 Mar 2026 15:32:51 -0400
Subject: [PATCH 1/2] fixup! Add __structuredAttrs ratchet and tests
---
src/problem/npv_166.rs | 2 +-
.../expected | 2 +-
.../main/default.nix | 0
.../main/pkgs/by-name/README.md | 0
.../main/pkgs/by-name/fo/foo/package.nix | 0
tests/structured-attrs-new-true/expected | 1 +
tests/structured-attrs-new-true/main/default.nix | 1 +
tests/structured-attrs-new-true/main/pkgs/by-name/README.md | 1 +
.../main/pkgs/by-name/fo/foo/package.nix | 1 +
tests/structured-attrs-new-unset/expected | 4 ++++
tests/structured-attrs-new-unset/main/default.nix | 1 +
tests/structured-attrs-new-unset/main/pkgs/by-name/README.md | 1 +
.../main/pkgs/by-name/fo/foo/package.nix | 1 +
13 files changed, 13 insertions(+), 2 deletions(-)
rename tests/{structured-attrs-new => structured-attrs-new-false}/expected (73%)
rename tests/{structured-attrs-new => structured-attrs-new-false}/main/default.nix (100%)
rename tests/{structured-attrs-new => structured-attrs-new-false}/main/pkgs/by-name/README.md (100%)
rename tests/{structured-attrs-new => structured-attrs-new-false}/main/pkgs/by-name/fo/foo/package.nix (100%)
create mode 100644 tests/structured-attrs-new-true/expected
create mode 100644 tests/structured-attrs-new-true/main/default.nix
create mode 100644 tests/structured-attrs-new-true/main/pkgs/by-name/README.md
create mode 100644 tests/structured-attrs-new-true/main/pkgs/by-name/fo/foo/package.nix
create mode 100644 tests/structured-attrs-new-unset/expected
create mode 100644 tests/structured-attrs-new-unset/main/default.nix
create mode 100644 tests/structured-attrs-new-unset/main/pkgs/by-name/README.md
create mode 100644 tests/structured-attrs-new-unset/main/pkgs/by-name/fo/foo/package.nix
diff --git a/src/problem/npv_166.rs b/src/problem/npv_166.rs
index 4fa1aab..2e00303 100644
--- a/src/problem/npv_166.rs
+++ b/src/problem/npv_166.rs
@@ -18,7 +18,7 @@ impl fmt::Display for NewTopLevelPackageMustEnableStructuredAttrs {
writedoc!(
f,
"
- - Attribute `pkgs.{package_name}` is a new package that evaluates with `__structuredAttrs = false`.
+ - Attribute `pkgs.{package_name}` is a new package with `__structuredAttrs` unset or set to `false`.
Please enable `__structuredAttrs = true;` in {file}.
",
)
diff --git a/tests/structured-attrs-new/expected b/tests/structured-attrs-new-false/expected
similarity index 73%
rename from tests/structured-attrs-new/expected
rename to tests/structured-attrs-new-false/expected
index 7734d2e..05a33c7 100644
--- a/tests/structured-attrs-new/expected
+++ b/tests/structured-attrs-new-false/expected
@@ -1,4 +1,4 @@
-- Attribute `pkgs.foo` is a new package that evaluates with `__structuredAttrs = false`.
+- Attribute `pkgs.foo` is a new package with `__structuredAttrs` unset or set to `false`.
Please enable `__structuredAttrs = true;` in pkgs/by-name/fo/foo/package.nix.
(https://github.com/NixOS/nixpkgs-vet/wiki/NPV-166)
This PR introduces additional instances of discouraged patterns as listed above. Please fix them before merging.
diff --git a/tests/structured-attrs-new/main/default.nix b/tests/structured-attrs-new-false/main/default.nix
similarity index 100%
rename from tests/structured-attrs-new/main/default.nix
rename to tests/structured-attrs-new-false/main/default.nix
diff --git a/tests/structured-attrs-new/main/pkgs/by-name/README.md b/tests/structured-attrs-new-false/main/pkgs/by-name/README.md
similarity index 100%
rename from tests/structured-attrs-new/main/pkgs/by-name/README.md
rename to tests/structured-attrs-new-false/main/pkgs/by-name/README.md
diff --git a/tests/structured-attrs-new/main/pkgs/by-name/fo/foo/package.nix b/tests/structured-attrs-new-false/main/pkgs/by-name/fo/foo/package.nix
similarity index 100%
rename from tests/structured-attrs-new/main/pkgs/by-name/fo/foo/package.nix
rename to tests/structured-attrs-new-false/main/pkgs/by-name/fo/foo/package.nix
diff --git a/tests/structured-attrs-new-true/expected b/tests/structured-attrs-new-true/expected
new file mode 100644
index 0000000..defae26
--- /dev/null
+++ b/tests/structured-attrs-new-true/expected
@@ -0,0 +1 @@
+Validated successfully
diff --git a/tests/structured-attrs-new-true/main/default.nix b/tests/structured-attrs-new-true/main/default.nix
new file mode 100644
index 0000000..861260c
--- /dev/null
+++ b/tests/structured-attrs-new-true/main/default.nix
@@ -0,0 +1 @@
+import <test-nixpkgs> { root = ./.; }
diff --git a/tests/structured-attrs-new-true/main/pkgs/by-name/README.md b/tests/structured-attrs-new-true/main/pkgs/by-name/README.md
new file mode 100644
index 0000000..232fdec
--- /dev/null
+++ b/tests/structured-attrs-new-true/main/pkgs/by-name/README.md
@@ -0,0 +1 @@
+# Test fixture
diff --git a/tests/structured-attrs-new-true/main/pkgs/by-name/fo/foo/package.nix b/tests/structured-attrs-new-true/main/pkgs/by-name/fo/foo/package.nix
new file mode 100644
index 0000000..c0b68f9
--- /dev/null
+++ b/tests/structured-attrs-new-true/main/pkgs/by-name/fo/foo/package.nix
@@ -0,0 +1 @@
+{ someDrv }: someDrv // { __structuredAttrs = true; }
diff --git a/tests/structured-attrs-new-unset/expected b/tests/structured-attrs-new-unset/expected
new file mode 100644
index 0000000..05a33c7
--- /dev/null
+++ b/tests/structured-attrs-new-unset/expected
@@ -0,0 +1,4 @@
+- Attribute `pkgs.foo` is a new package with `__structuredAttrs` unset or set to `false`.
+ Please enable `__structuredAttrs = true;` in pkgs/by-name/fo/foo/package.nix.
+ (https://github.com/NixOS/nixpkgs-vet/wiki/NPV-166)
+This PR introduces additional instances of discouraged patterns as listed above. Please fix them before merging.
diff --git a/tests/structured-attrs-new-unset/main/default.nix b/tests/structured-attrs-new-unset/main/default.nix
new file mode 100644
index 0000000..861260c
--- /dev/null
+++ b/tests/structured-attrs-new-unset/main/default.nix
@@ -0,0 +1 @@
+import <test-nixpkgs> { root = ./.; }
diff --git a/tests/structured-attrs-new-unset/main/pkgs/by-name/README.md b/tests/structured-attrs-new-unset/main/pkgs/by-name/README.md
new file mode 100644
index 0000000..232fdec
--- /dev/null
+++ b/tests/structured-attrs-new-unset/main/pkgs/by-name/README.md
@@ -0,0 +1 @@
+# Test fixture
diff --git a/tests/structured-attrs-new-unset/main/pkgs/by-name/fo/foo/package.nix b/tests/structured-attrs-new-unset/main/pkgs/by-name/fo/foo/package.nix
new file mode 100644
index 0000000..3b184b7
--- /dev/null
+++ b/tests/structured-attrs-new-unset/main/pkgs/by-name/fo/foo/package.nix
@@ -0,0 +1 @@
+{ someDrv }: removeAttrs someDrv [ "__structuredAttrs" ]
--
2.51.2
Details
From c5589aca898029c900cc32f1ad5399a853166a83 Mon Sep 17 00:00:00 2001
From: Michael Daniels <mdaniels5757@gmail.com>
Date: Sun, 22 Mar 2026 15:39:07 -0400
Subject: [PATCH 2/2] fixup! Add strictDeps ratchet and tests
---
src/problem/npv_164.rs | 2 +-
tests/{strict-deps-new => strict-deps-new-false}/expected | 2 +-
.../main/default.nix | 0
.../main/pkgs/by-name/README.md | 0
.../main/pkgs/by-name/fo/foo/package.nix | 0
tests/strict-deps-new-true/expected | 1 +
tests/strict-deps-new-true/main/default.nix | 1 +
tests/strict-deps-new-true/main/pkgs/by-name/README.md | 1 +
.../strict-deps-new-true/main/pkgs/by-name/fo/foo/package.nix | 1 +
tests/strict-deps-new-unset/expected | 4 ++++
tests/strict-deps-new-unset/main/default.nix | 1 +
tests/strict-deps-new-unset/main/pkgs/by-name/README.md | 1 +
.../main/pkgs/by-name/fo/foo/package.nix | 1 +
13 files changed, 13 insertions(+), 2 deletions(-)
rename tests/{strict-deps-new => strict-deps-new-false}/expected (74%)
rename tests/{strict-deps-new => strict-deps-new-false}/main/default.nix (100%)
rename tests/{strict-deps-new => strict-deps-new-false}/main/pkgs/by-name/README.md (100%)
rename tests/{strict-deps-new => strict-deps-new-false}/main/pkgs/by-name/fo/foo/package.nix (100%)
create mode 100644 tests/strict-deps-new-true/expected
create mode 100644 tests/strict-deps-new-true/main/default.nix
create mode 100644 tests/strict-deps-new-true/main/pkgs/by-name/README.md
create mode 100644 tests/strict-deps-new-true/main/pkgs/by-name/fo/foo/package.nix
create mode 100644 tests/strict-deps-new-unset/expected
create mode 100644 tests/strict-deps-new-unset/main/default.nix
create mode 100644 tests/strict-deps-new-unset/main/pkgs/by-name/README.md
create mode 100644 tests/strict-deps-new-unset/main/pkgs/by-name/fo/foo/package.nix
diff --git a/src/problem/npv_164.rs b/src/problem/npv_164.rs
index 807d049..e9feecd 100644
--- a/src/problem/npv_164.rs
+++ b/src/problem/npv_164.rs
@@ -18,7 +18,7 @@ impl fmt::Display for NewTopLevelPackageMustEnableStrictDeps {
writedoc!(
f,
"
- - Attribute `pkgs.{package_name}` is a new package that evaluates with `strictDeps = false`.
+ - Attribute `pkgs.{package_name}` is a new package with `strictDeps` unset or set to `false`.
Please enable `strictDeps = true;` in {file}.
",
)
diff --git a/tests/strict-deps-new/expected b/tests/strict-deps-new-false/expected
similarity index 74%
rename from tests/strict-deps-new/expected
rename to tests/strict-deps-new-false/expected
index b5a0502..1ddf6c2 100644
--- a/tests/strict-deps-new/expected
+++ b/tests/strict-deps-new-false/expected
@@ -1,4 +1,4 @@
-- Attribute `pkgs.foo` is a new package that evaluates with `strictDeps = false`.
+- Attribute `pkgs.foo` is a new package with `strictDeps` unset or set to `false`.
Please enable `strictDeps = true;` in pkgs/by-name/fo/foo/package.nix.
(https://github.com/NixOS/nixpkgs-vet/wiki/NPV-164)
This PR introduces additional instances of discouraged patterns as listed above. Please fix them before merging.
diff --git a/tests/strict-deps-new/main/default.nix b/tests/strict-deps-new-false/main/default.nix
similarity index 100%
rename from tests/strict-deps-new/main/default.nix
rename to tests/strict-deps-new-false/main/default.nix
diff --git a/tests/strict-deps-new/main/pkgs/by-name/README.md b/tests/strict-deps-new-false/main/pkgs/by-name/README.md
similarity index 100%
rename from tests/strict-deps-new/main/pkgs/by-name/README.md
rename to tests/strict-deps-new-false/main/pkgs/by-name/README.md
diff --git a/tests/strict-deps-new/main/pkgs/by-name/fo/foo/package.nix b/tests/strict-deps-new-false/main/pkgs/by-name/fo/foo/package.nix
similarity index 100%
rename from tests/strict-deps-new/main/pkgs/by-name/fo/foo/package.nix
rename to tests/strict-deps-new-false/main/pkgs/by-name/fo/foo/package.nix
diff --git a/tests/strict-deps-new-true/expected b/tests/strict-deps-new-true/expected
new file mode 100644
index 0000000..defae26
--- /dev/null
+++ b/tests/strict-deps-new-true/expected
@@ -0,0 +1 @@
+Validated successfully
diff --git a/tests/strict-deps-new-true/main/default.nix b/tests/strict-deps-new-true/main/default.nix
new file mode 100644
index 0000000..861260c
--- /dev/null
+++ b/tests/strict-deps-new-true/main/default.nix
@@ -0,0 +1 @@
+import <test-nixpkgs> { root = ./.; }
diff --git a/tests/strict-deps-new-true/main/pkgs/by-name/README.md b/tests/strict-deps-new-true/main/pkgs/by-name/README.md
new file mode 100644
index 0000000..232fdec
--- /dev/null
+++ b/tests/strict-deps-new-true/main/pkgs/by-name/README.md
@@ -0,0 +1 @@
+# Test fixture
diff --git a/tests/strict-deps-new-true/main/pkgs/by-name/fo/foo/package.nix b/tests/strict-deps-new-true/main/pkgs/by-name/fo/foo/package.nix
new file mode 100644
index 0000000..62c3689
--- /dev/null
+++ b/tests/strict-deps-new-true/main/pkgs/by-name/fo/foo/package.nix
@@ -0,0 +1 @@
+{ someDrv }: someDrv // { strictDeps = true; }
diff --git a/tests/strict-deps-new-unset/expected b/tests/strict-deps-new-unset/expected
new file mode 100644
index 0000000..1ddf6c2
--- /dev/null
+++ b/tests/strict-deps-new-unset/expected
@@ -0,0 +1,4 @@
+- Attribute `pkgs.foo` is a new package with `strictDeps` unset or set to `false`.
+ Please enable `strictDeps = true;` in pkgs/by-name/fo/foo/package.nix.
+ (https://github.com/NixOS/nixpkgs-vet/wiki/NPV-164)
+This PR introduces additional instances of discouraged patterns as listed above. Please fix them before merging.
diff --git a/tests/strict-deps-new-unset/main/default.nix b/tests/strict-deps-new-unset/main/default.nix
new file mode 100644
index 0000000..861260c
--- /dev/null
+++ b/tests/strict-deps-new-unset/main/default.nix
@@ -0,0 +1 @@
+import <test-nixpkgs> { root = ./.; }
diff --git a/tests/strict-deps-new-unset/main/pkgs/by-name/README.md b/tests/strict-deps-new-unset/main/pkgs/by-name/README.md
new file mode 100644
index 0000000..232fdec
--- /dev/null
+++ b/tests/strict-deps-new-unset/main/pkgs/by-name/README.md
@@ -0,0 +1 @@
+# Test fixture
diff --git a/tests/strict-deps-new-unset/main/pkgs/by-name/fo/foo/package.nix b/tests/strict-deps-new-unset/main/pkgs/by-name/fo/foo/package.nix
new file mode 100644
index 0000000..40d48fb
--- /dev/null
+++ b/tests/strict-deps-new-unset/main/pkgs/by-name/fo/foo/package.nix
@@ -0,0 +1 @@
+{ someDrv }: removeAttrs someDrv [ "strictDeps" ]
--
2.51.2
5606eff to
7764aa6
Compare
Thanks, I applied these. |
5faf509 to
7ea08aa
Compare
The next ratchets need more context than the existing by-name check alone: they must be able to report a stable source location for attributes defined outside pkgs/by-name as well. Isolate that control-flow cleanup first so the later strictDeps and __structuredAttrs commits can stay focused on behavior rather than being mixed with a larger evaluator reshaping. Co-authored-by: Michael Daniels <mdaniels5757@gmail.com>
Co-authored-by: Michael Daniels <mdaniels5757@gmail.com>
Co-authored-by: Michael Daniels <mdaniels5757@gmail.com>
7ea08aa to
4d45bca
Compare
|
Just squashed some commits, and moved around a comment. |
| - Attribute `pkgs.{package_name}` is a new package with `__structuredAttrs` unset or set to `false`. | ||
| Please enable `__structuredAttrs = true;` in {file}. |
There was a problem hiding this comment.
I just encountered this CI message here:
https://github.com/NixOS/nixpkgs/actions/runs/24303117878/job/70959851952
The PR adds something similar to the following:
{
python3Packages,
}:
python3Packages.toPythonApplication (
python3Packages.optiland.override {
enableGui = true;
}
)to pkgs/by-name/op/optiland/package.nix.
I am not sure how to set __structuredAttrs explicitly there. Could anyone here give a hand please? Maybe the check should be modified a bit to exclude such expressions?
There was a problem hiding this comment.
Does toPythonApplication implement __structuredAttrs? I would start there, if it doesn't, it would be best to make a pr.
There was a problem hiding this comment.
In this case wouldn't it be enough to set __structuredAttrs = true; on the base python3Packages.optiland package?
There was a problem hiding this comment.
In this case wouldn't it be enough to set
__structuredAttrs = true;on the basepython3Packages.optilandpackage?
Well indeed it worked. I'm surprised though - I remember I noticed a while ago that Python packages are built with structuredAttrs enabled by default, as I always saw:
structuredAttrs is enabled
In the build logs. However I can't reproduce it now.
There was a problem hiding this comment.
Now that you mention it... yes, that is indeed strange. I don't have time right now but I'll have to look into that.
|
I encountered today another scenario where this check is bugging me: I added a derivation to Now I need to add support for |
Enable adding new packages to nixpkgs that are defined using buildEnv, without triggering new nixpkgs-vet CI failures. See: NixOS/nixpkgs-vet#203 In the future, enable `__structuredAttrs` and `strictDeps` by default.
This adds ratchet checks for two evaluated derivation flags:
__structuredAttrsmust not be newly introduced in a disabled statestrictDepsmust not be newly introduced in a disabled stateThe checks are based on evaluated attribute state rather than textual matching, so they also work in package sets where
these flags are already enabled by default.
Approach
The series first adds shared infrastructure for ratchets over evaluated boolean attributes.
It then refactors non-pkgs/by-name evaluation so later ratchets can reuse resolved definition and location information. That
keeps the feature commits focused on the new policy, instead of mixing them with evaluator reshaping.
Finally, the two concrete ratchets are added:
__structuredAttrsstrictDepsBoth apply to top-level package attributes, including attributes outside
pkgs/by-namewhen evaluation yields enough information to make a reliable call.Notes
The test mock is adjusted so the default mock derivation already enables these flags. That keeps the fixture diff smaller and leaves explicit overrides only in the focused regression/new-package tests.
Benchmark
Measured on NixOS/nixpkgs@b110319 with
hyperfine --warmup 1 --runs 5:Baseline b62136c:
Current branch:
Observed difference: about +0.03s / +0.16%.