-
-
Notifications
You must be signed in to change notification settings - Fork 15k
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
check-meta: Expose checkValidity and add to all-packages #339597
Conversation
Bump |
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/prs-ready-for-review/3032/4709 |
@@ -226,6 +226,8 @@ with pkgs; | |||
} | |||
''); | |||
|
|||
checkMeta = callPackage ../stdenv/generic/check-meta.nix { }; |
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.
Why this does not trigger by-name checks?
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.
No derivations added I guess?
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.
The checks are for adding a package (more precisely, a function that returns a derivation) to pkgs/top-level/all-packages.nix
; this is a function that returns an attrset.
Bump! |
@@ -226,6 +226,8 @@ with pkgs; | |||
} | |||
''); | |||
|
|||
checkMeta = callPackage ../stdenv/generic/check-meta.nix { }; |
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.
The checks are for adding a package (more precisely, a function that returns a derivation) to pkgs/top-level/all-packages.nix
; this is a function that returns an attrset.
This broke eval, because checkMeta expects https://github.com/NixOS/nixpkgs/actions/runs/12929570543/job/36059219164?pr=376113 |
Dang. I saw the ✅ and hit merge. Thanks for the revert @mweinelt. |
I would generally advise caution with older PRs, given that they were evaled either against an older nixpkgs or not at all. |
Yeah, I can see that. Adding "consider rebasing the PR before merging if >1mo old" to my notes. |
Might be worth having an action that rebases and checks evaluation upon PR approval? Also: are these notes public? I'd like a copy for my own reviewing. |
The best solution would be to use GitHub merge queues to ensure that each PR gets eval tested on its actual merged parent before landing. Multiple PRs in the queue at once can have their evaluation tested in parallel to not slow down merging too much. |
I imagine this would require a repository admin to set that up, right? |
It's a physical notebook on my desk. 😞 |
Would still appreciate a copy! |
Description of changes
Cherry-picked from a commit that used to be in #326199 (see discussion at #326199 (comment))
This is useful when a package has an optional dependency on another package that is nonfree or not available for every platform (e.g. #326199, #310138).
CC @natsukium
Things done
nix.conf
? (See Nix manual)sandbox = relaxed
sandbox = true
nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)Add a 👍 reaction to pull requests you find important.