-
-
Notifications
You must be signed in to change notification settings - Fork 13.3k
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
nixos/screen: fix assertion to actually execute, by moving it outside of mkIf. #312210
nixos/screen: fix assertion to actually execute, by moving it outside of mkIf. #312210
Conversation
I see this question came up during the PR review that introduced the assertion: https://github.com/NixOS/nixpkgs/pull/262133/files/f12eabdcbe642884b37cb5d20dbf781f998061f7#r1415492863 |
#312187 (comment) has a better suggested alternative. I'll do that. |
No, that is worse. Using throw will abort the evaluation completely and not collect all assertions. |
See NixOS#312194 (comment) for explanation why the assertion currently fails to run.
74dd80c
to
3544cfe
Compare
@SuperSandro2000 thoughts on this being merged before the 24.05 cut?
|
@Mic92 @wegank , thoughts re #312210 (comment) ? |
The code was before broken and didn't do what we expected it to do and we silently ignored peoples configs unlike what is written in the release notes. |
I just got hit by this assertion, why does it need to exist? Per convention, module settings are always guarded behind the enable option's value. We usually don't assert this. |
#262133 introduced this assertion, and has the rationale/commentary. This PR just fixed the assertion to actually work. |
Fixes #312194.
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.