-
-
Notifications
You must be signed in to change notification settings - Fork 13.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve pkgs/by-name
code, minor fix
#287083
Conversation
For some previously untested cases. In a future commit, those tests will also be adjusted slightly
bedf79b
to
3510fd1
Compare
pkgs/by-name
checking codepkgs/by-name
code, move sbcl
out of pkgs/by-name
pkgs/by-name
code, move sbcl
out of pkgs/by-name
pkgs/by-name
code, minor fix
3510fd1
to
fd06940
Compare
We can't avoid |
fd06940
to
4699849
Compare
I'm not exactly clear on the details but I believe it :) thanks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I particularly like the changes in eval.rs
@hraban The intuitive explanation is that for any
In reality we still need to allow customising the |
- Detect manual use of _internalCallByNamePackageFile for packages in `pkgs/by-name` (can't be done for others though) - Separate error message for when attribute locations can't be determined for `pkgs/by-name` attributes - Much better structure of the code in eval.rs, representing more closely what is being checked - Much more extensive comments
This adds a test to check that a commit like 0a3dab4 would fail CI After doing some improvements to the `pkgs/by-name` check I discovered that sbcl shouldn't have been allowed in `pkgs/by-name` after all as is. Specifically, the requirement is that if `pkgs/by-name/sb/sbcl` exists, the definition of the `sbcl` attribute must look like sbcl = callPackage ../by-name/sb/sbcl/package.nix { ... }; However it currently is an alias like sbcl = sbcl_2_4_1; This wasn't detected before because `sbcl_2_4_1` was semantically defined using `callPackage`: sbcl_2_4_1 = wrapLisp { pkg = callPackage ../development/compilers/sbcl { version = "2.4.1"; }; faslExt = "fasl"; flags = [ "--dynamic-space-size" "3000" ]; }; However this doesn't syntactically match what is required. In NixOS#285089 I introduced syntactic checks for exactly this, but they were only used for packages not already in `pkgs/by-name`. Only now that I'm doing the refactoring to also use this check for `pkgs/by-name` packages this problem is noticed. While introducing this new check is technically an increase in strictness, and therefore would justify adding a new ratchet, I consider this case to be rare enough that we don't need to do that. This commit introduces a test to prevent such regressions in the future Moving sbcl back out of `pkgs/by-name` will be done when the pinned CI is updated
This reverts commit 0a3dab4 See the parent commit as to why this is necessary
4699849
to
6fc063c
Compare
@philiptaron Thanks for the quick review! ❤️ Typos now fixed :D |
Description of changes
Mainly code cleanup, but that also discovered a problem, which is fixed with the cleanup.
pkgs/by-name
: Enforce for new packages #275539 had false negative bug that would've made sbcl: 2.4.0 -> 2.4.1 #284591 succeed CI, even thoughsbcl
isn't usingpkgs.callPackage ...
directly.callPackage
detection and allow new package variants #285089, though that wasn't implemented fully, only triggering for new packages, but at that pointsbcl
was already merged as is, since I discounted the CI failure as a false positiveSee the commit messages for more details.
I'll merge this myself soon after CI is successful.
This work is sponsored by Antithesis and Tweag ✨
Things done
nix-build -A tests.nixpkgs-check-by-name.tests
successfully onx86_64-linux
Add a 👍 reaction to pull requests you find important.