top-level/stage: assert no by-name overwrites#454525
Closed
wolfgangwalther wants to merge 1 commit intoNixOS:masterfrom
Closed
top-level/stage: assert no by-name overwrites#454525wolfgangwalther wants to merge 1 commit intoNixOS:masterfrom
wolfgangwalther wants to merge 1 commit intoNixOS:masterfrom
Conversation
This asserts that no by-name packages are overwritten by definitions in all-packages.nix.
3 tasks
13 tasks
Contributor
|
We got into more trouble with this than I expected. I tried to make this show the |
Contributor
The issue here is that the assertion is thown part-way through fixing the various overlays that'll construct Therefore it is safe to evaluate the attrnames, but evaluating a package value will require One way to work-around this is to move the assert/throw outside of the overlay chain, so that diff --git a/pkgs/top-level/stage.nix b/pkgs/top-level/stage.nix
index 12d3b9315e9a..be952b18aaad 100644
--- a/pkgs/top-level/stage.nix
+++ b/pkgs/top-level/stage.nix
@@ -189,9 +189,19 @@ let
conflictingAttrs = lib.intersectAttrs res super;
in
- assert lib.assertMsg (conflictingAttrs == { })
- "The following attributes were defined both in `pkgs/top-level/all-packages.nix` and elsewhere, most likely in `pkgs/by-name/`: ${lib.concatStringsSep ", " (lib.attrNames conflictingAttrs)}";
- res;
+ res
+ // {
+ __assertions = res.__assertions or [ ] ++ [
+ {
+ assertion = conflictingAttrs == { };
+ message = "The following attributes were defined both in `pkgs/top-level/all-packages.nix` and elsewhere, most likely in `pkgs/by-name/`: ${
+ lib.concatMapAttrsStringSep "\n" (
+ name: pkg: "- ${name}" + lib.optionalString (pkg ? meta.position) ": ${pkg.meta.position}"
+ ) conflictingAttrs
+ }";
+ }
+ ];
+ };
aliases = self: super: lib.optionalAttrs config.allowAliases (import ./aliases.nix lib self super);
@@ -357,6 +367,8 @@ let
]
);
+ handleAssertions =
+ result: lib.asserts.checkAssertWarn (result.__assertions or [ ]) (result.__warnings or [ ]) result;
in
-# Return the complete set of packages.
-lib.fix toFix
+# Check assertions and return the complete set of packages.
+handleAssertions (lib.fix toFix)This solves the inf-rec, but I'm seeing another weird error about packages being missing from Full trace
This makes me think my current patch is still throwing too early. EDIT: I tried moving the assertion furthher out to diff --git a/pkgs/top-level/default.nix b/pkgs/top-level/default.nix
index 12329d0af82f..3b829e896c0a 100644
--- a/pkgs/top-level/default.nix
+++ b/pkgs/top-level/default.nix
@@ -89,7 +89,8 @@ let
lib.foldr
(x: throwIfNot (lib.isFunction x) "All crossOverlays passed to nixpkgs must be functions.")
(r: r)
- crossOverlays;
+ crossOverlays
+ (lib.asserts.checkAssertWarn (pkgs.__assertions or [ ]) (pkgs.__warnings or [ ]));
localSystem = lib.systems.elaborate args.localSystem;
diff --git a/pkgs/top-level/stage.nix b/pkgs/top-level/stage.nix
index 12d3b9315e9a..439c5e409c11 100644
--- a/pkgs/top-level/stage.nix
+++ b/pkgs/top-level/stage.nix
@@ -189,9 +189,19 @@ let
conflictingAttrs = lib.intersectAttrs res super;
in
- assert lib.assertMsg (conflictingAttrs == { })
- "The following attributes were defined both in `pkgs/top-level/all-packages.nix` and elsewhere, most likely in `pkgs/by-name/`: ${lib.concatStringsSep ", " (lib.attrNames conflictingAttrs)}";
- res;
+ res
+ // {
+ __assertions = res.__assertions or [ ] ++ [
+ {
+ assertion = conflictingAttrs == { };
+ message = "The following attributes were defined both in `pkgs/top-level/all-packages.nix` and elsewhere, most likely in `pkgs/by-name/`: ${
+ lib.concatMapAttrsStringSep "\n" (
+ name: pkg: "- ${name}" + lib.optionalString (pkg ? meta.position) ": ${pkg.meta.position}"
+ ) conflictingAttrs
+ }";
+ }
+ ];
+ };
aliases = self: super: lib.optionalAttrs config.allowAliases (import ./aliases.nix lib self super);
|
Contributor
Author
This is probably not even what would be useful. If we're dealing with by-name overrides in all-packages.nix, then both of these will point to the same package, that means their We might want to show the I don't think it's worth spending cycles on trying to improve this more than what we have right now, tbh. |
Contributor
Showing the position of the definition in diff --git a/pkgs/top-level/stage.nix b/pkgs/top-level/stage.nix
index 12d3b9315e9a..f9daeb4202f0 100644
--- a/pkgs/top-level/stage.nix
+++ b/pkgs/top-level/stage.nix
@@ -190,7 +190,18 @@ let
conflictingAttrs = lib.intersectAttrs res super;
in
assert lib.assertMsg (conflictingAttrs == { })
- "The following attributes were defined both in `pkgs/top-level/all-packages.nix` and elsewhere, most likely in `pkgs/by-name/`: ${lib.concatStringsSep ", " (lib.attrNames conflictingAttrs)}";
+ "The following attributes were defined both in `pkgs/top-level/all-packages.nix` and elsewhere, most likely in `pkgs/by-name/`: ${
+ lib.concatMapStrings (
+ name:
+ let
+ pos = builtins.unsafeGetAttrPos name res;
+ in
+ "\n- ${name}"
+ + lib.optionalString (
+ pos.file or null == toString ./all-packages.nix
+ ) " (line ${toString pos.line})"
+ ) (lib.attrNames conflictingAttrs)
+ }";
res;
aliases = self: super: lib.optionalAttrs config.allowAliases (import ./aliases.nix lib self super);(The We can also manually look for an attr definition elsewhere, but you're right that in the typical by-name we won't find one: diff --git a/pkgs/top-level/stage.nix b/pkgs/top-level/stage.nix
index f9daeb4202f0..a220ed14d997 100644
--- a/pkgs/top-level/stage.nix
+++ b/pkgs/top-level/stage.nix
@@ -194,12 +194,16 @@ let
lib.concatMapStrings (
name:
let
+ prevPos = builtins.unsafeGetAttrPos name super;
pos = builtins.unsafeGetAttrPos name res;
in
"\n- ${name}"
+ lib.optionalString (
pos.file or null == toString ./all-packages.nix
) " (line ${toString pos.line})"
+ +
+ lib.optionalString (prevPos != null)
+ " also defined at ${lib.removePrefix "${toString ../../.}/" prevPos.file}:${toString prevPos.line}"
) (lib.attrNames conflictingAttrs)
}";
res;Currently, this doesn't match anything. One thing worth considering is the formatting. Should we put the line number at the start so that the list is more readable? vs And then, should we consider sorting by line-number instead of by attr name? |
Contributor
Author
|
Do we really need to print the line number at all? It should be trivial to look for |
Contributor
|
I don't think we need to, no. It may be slightly helpful if someone has an editor that struggles searching through long files but is ok at jumping to a specific line, however I agree that's not really a strong justification for adding the additional code. The only other scenario I can see it helping with, is a package with a very generic name that is hard to grep for. Again, kinda contrived. Hopefully, once we've done the initial cleanup in #453948, people will only see the error when they add a new package to That said, I usually default to putting as much info as practical into errors and warnings, so that they can be as useful as possible. |
13 tasks
Contributor
Author
|
Not going to pursue this anymore myself (#469692). |
This was referenced Dec 11, 2025
This was referenced Jan 25, 2026
46 tasks
This was referenced Jan 26, 2026
Merged
This was referenced Jan 28, 2026
This was referenced Jan 31, 2026
Contributor
|
This is continued in #483820. |
98 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This asserts that no by-name packages are overwritten by definitions in all-packages.nix.
The relevant code from
nixpkgs/pkgs/top-level/by-name-overlay.nix
Lines 49 to 57 in 804ab2e
(replacement of #454147)
Things done
Add a 👍 reaction to pull requests you find important.