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

nixos: top-level: evaluate assertions before warnings #47293

Merged
merged 2 commits into from Sep 25, 2018

Conversation

oxij
Copy link
Member

@oxij oxij commented Sep 24, 2018

Motivation for this change

A bug found while investigating #46193 (comment).

Things done
  • Noop, only changes evaluation order.

or else at least the following config will fail with an evaluation error
instead of an assert

```
{
  services.nixosManual.enable = false;
  services.nixosManual.showManual = true;
}
```
@oxij oxij mentioned this pull request Sep 24, 2018
1 task
oxij referenced this pull request Sep 25, 2018
Introduced by 0f3b89b.

If services.nixosManual.showManual is enabled and
documentation.nixos.enable is not, there is no
config.system.build.manual available, so evaluation fails. For example
this is the case for the installer tests.

There is however an assertion which should catch exactly this, but it
isn't thrown because the usage of config.system.build.manual is
evaluated earlier than the assertions.

So I split the assertion off into a separate mkIf to make sure it is
shown appropriately and also fixed the installation-device profile to
enable documentation.nixos.

Signed-off-by: aszlig <aszlig@nix.build>
Cc: @oxij
@aszlig aszlig self-assigned this Sep 25, 2018
@aszlig aszlig merged commit 563d5b1 into NixOS:master Sep 25, 2018
aszlig added a commit that referenced this pull request Sep 25, 2018
Changes the evaluation order in that it evaluates assertions before
warnings, so that eg. the following would work:

  { config, lib, ... }:

  {
    options.foo = lib.mkOption {
      type = lib.types.bool;
      default = true;
      description = "...";
    };

    options.bar = lib.mkOption {
      type = lib.types.bool;
      default = false;
      description = "...";
    };

    config = lib.mkMerge [
      (lib.mkIf config.bar {
        system.build.bar = "foobar";
      })
      (lib.mkIf config.foo {
        assertions = lib.singleton {
          assertion = config.bar;
          message = "Bar needs to be enabled";
        };
        systemd.services.foo = {
          description = "Foo";
          serviceConfig.ExecStart = config.system.build.bar;
        };
      })
    ];
  }

This is because the systemd module includes definitions for warnings
that would trigger evaluation of the config.system.build.bar definition.

The original pull request references a breakage due to the following:

  {
    services.nixosManual.enable = false;
    services.nixosManual.showManual = true;
  }

However, changing the eval order between asserts and warnings clearly is
a corner case here and it only happens because of the aforementioned
usage of warnings in the systemd module and needs more discussion.

Nevertheless, this is still useful because it lowers the evaluation time
whenever an assertion is hit, which is a hard failure anyway.
@oxij oxij deleted the nixos/asserts-before-warnings branch November 18, 2018 08:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants