Improve pkgs/by-name code, minor fix
#287083
Merged
Conversation
This file contains 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
For some previously untested cases. In a future commit, those tests will also be adjusted slightly
infinisil
force-pushed
the
by-name-cleanup
branch
from
bedf79b
to
3510fd1
Compare
infinisil
changed the title
pkgs/by-name checking codepkgs/by-name code, move sbcl out of pkgs/by-name
infinisil
changed the title
pkgs/by-name code, move sbcl out of pkgs/by-namepkgs/by-name code, minor fix
infinisil
force-pushed
the
by-name-cleanup
branch
from
3510fd1
to
fd06940
Compare
|
We can't avoid |
infinisil
force-pushed
the
by-name-cleanup
branch
from
fd06940
to
4699849
Compare
|
I'm not exactly clear on the details but I believe it :) thanks |
philiptaron
approved these changes
Feb 8, 2024
|
@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
infinisil
force-pushed
the
by-name-cleanup
branch
from
4699849
to
6fc063c
Compare
|
@philiptaron Thanks for the quick review! ❤️ Typos now fixed :D |
philiptaron
approved these changes
Feb 8, 2024
infinisil
added a commit
to tweag/nixpkgs
that referenced
this pull request
Feb 12, 2024
Emantor
pushed a commit
to Emantor/nixpkgs
that referenced
this pull request
Feb 13, 2024
infinisil
added a commit
to NixOS/nixpkgs-check-by-name
that referenced
this pull request
Mar 18, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
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.