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-namepkgs/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 |
philiptaron
left a comment
There was a problem hiding this comment.
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 thoughsbclisn't usingpkgs.callPackage ...directly.callPackagedetection and allow new package variants #285089, though that wasn't implemented fully, only triggering for new packages, but at that pointsbclwas 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.testssuccessfully onx86_64-linuxAdd a 👍 reaction to pull requests you find important.