Skip to content
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

programs.screen's assertion doesn't run, possibly due to confusing semantics around mkIf? #312194

Closed
tomfitzhenry opened this issue May 16, 2024 · 2 comments · Fixed by #312210
Closed

Comments

@tomfitzhenry
Copy link
Contributor

tomfitzhenry commented May 16, 2024

Steps To Reproduce

Steps to reproduce the behavior:

  1. Build a machine with:
porgrams.screen.enable = false;
programs.screen.screenrc = "foobar";

Expected behavior

This should fail to build, due to this assertion:

assertions = [
{
assertion = cfg.screenrc != null -> cfg.enable;
message = "`programs.screen.screenrc` has been configured, but `programs.screen.enable` is not true";
}
];

But the assertion doesn't assert.

As a means of diagnosis, if I remove "lib.mkIf cfg.enable" from line 33, the assertion asserts.

Very confusing!

Additional context

I noticed this oddity between lib.mkIf, //, and assertions, while addressing a PR review comment in an unrelated PR: #312187 (comment)

Notify maintainers

@vrthra


Add a 👍 reaction to issues you find important.

@tomfitzhenry
Copy link
Contributor Author

tomfitzhenry commented May 16, 2024

Oh, I think I see what's going on.

{ foo = 1; } // mkIf true { bar = 1; }; is mixing a plain attrset (LHS) with a NixOS option if attrset (RHS), which is an invalid operation.

This assertion just won't work.

There's presumably a way to do this in https://github.com/NixOS/nixpkgs/blob/master/lib/modules.nix that does the right thing.

@tomfitzhenry
Copy link
Contributor Author

There's presumably a way to do this in https://github.com/NixOS/nixpkgs/blob/master/lib/modules.nix that does the right thing.

I found a way, but it's gross.

Disclaimer: I've never explored the innards of options/module merging, so my understanding/terminology is going to be way off.

The idea is to lift the plain attrset into the NixOS module value space (?) via lib.mkIf true, and then use lib.mkMerge as an alternative to //.

config = lib.mkMerge [
  (lib.mkIf true { assertions = foo; })
  (lib.mkIf cfg.enable { blah = 2; })
];

It works!

tomfitzhenry added a commit to tomfitzhenry/nixpkgs that referenced this issue May 16, 2024
See NixOS#312194 (comment) for explanation why the assertion currently fails to run.
tomfitzhenry added a commit to tomfitzhenry/nixpkgs that referenced this issue May 17, 2024
See NixOS#312194 (comment) for explanation why the assertion currently fails to run.
tomfitzhenry added a commit that referenced this issue May 20, 2024
See #312194 (comment) for explanation why the assertion currently fails to run.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant