Skip to content

Commit

Permalink
nixos: Fix eval error for documentation.nixos
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@aszlig
aszlig committed Sep 25, 2018
1 parent 6cd90cb commit c5bb431
Show file tree
Hide file tree
Showing 2 changed files with 25 additions and 23 deletions.
1 change: 1 addition & 0 deletions nixos/modules/profiles/installation-device.nix
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ with lib;
documentation.enable = mkForce true;

# Show the manual.
documentation.nixos.enable = mkForce true;
services.nixosManual.showManual = true;

# Let the user play Rogue on TTY 8 during the installation.
Expand Down
47 changes: 24 additions & 23 deletions nixos/modules/services/misc/nixos-manual.nix
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,29 +44,30 @@ in
};


config = mkIf cfg.showManual {

assertions = [{
assertion = cfgd.enable && cfgd.nixos.enable;
message = "Can't enable `service.nixosManual.showManual` without `documentation.nixos.enable`";
}];

boot.extraTTYs = [ "tty${toString cfg.ttyNumber}" ];

systemd.services."nixos-manual" = {
description = "NixOS Manual";
wantedBy = [ "multi-user.target" ];
serviceConfig = {
ExecStart = "${cfg.browser} ${config.system.build.manual.manualHTMLIndex}";
StandardInput = "tty";
StandardOutput = "tty";
TTYPath = "/dev/tty${toString cfg.ttyNumber}";
TTYReset = true;
TTYVTDisallocate = true;
Restart = "always";
config = mkMerge [
(mkIf cfg.showManual {
assertions = singleton {
assertion = cfgd.enable && cfgd.nixos.enable;
message = "Can't enable `services.nixosManual.showManual` without `documentation.nixos.enable`";
};
};

};
})
(mkIf (cfg.showManual && cfgd.enable && cfgd.nixos.enable) {
boot.extraTTYs = [ "tty${toString cfg.ttyNumber}" ];

systemd.services."nixos-manual" = {
description = "NixOS Manual";
wantedBy = [ "multi-user.target" ];
serviceConfig = {
ExecStart = "${cfg.browser} ${config.system.build.manual.manualHTMLIndex}";
StandardInput = "tty";
StandardOutput = "tty";
TTYPath = "/dev/tty${toString cfg.ttyNumber}";
TTYReset = true;
TTYVTDisallocate = true;
Restart = "always";
};
};
})
];

}

9 comments on commit c5bb431

@oxij
Copy link
Member

@oxij oxij commented on c5bb431 Sep 25, 2018 via email

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@oxij
Copy link
Member

@oxij oxij commented on c5bb431 Sep 25, 2018 via email

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aszlig
Copy link
Member Author

@aszlig aszlig commented on c5bb431 Sep 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@oxij: Fair enough, that seems to be a better fix, I'll do a partial revert + merge after reviewing #47293.

@aszlig
Copy link
Member Author

@aszlig aszlig commented on c5bb431 Sep 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@oxij: Okay, merged the pull request in 9bfd864, however as noted there, I don't think that just reordering asserts/warnings is the proper fix, because in any case even when not building the full system, the eval error stays there when you don't access config.system.build but some other config value, like how it's done in nixos-option.

@aszlig
Copy link
Member Author

@aszlig aszlig commented on c5bb431 Sep 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For example, when changing the warnings in the systemd module to asserts, you'll get the same evaluation error but this time the other way around. Swapping the evaluation order for every corner case like this clearly is something I'd consider a workaround. And yes, I acknowledge that mkMerge here makes the code a bit more convoluted, but also prevents a minefield of random evaluation errors whenever the implementation of another module changes in the way described.

@aszlig
Copy link
Member Author

@aszlig aszlig commented on c5bb431 Sep 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So in summary: While I do find some advantages in ordering assertions before warnings (as stated in the mentioned merge commit), I'm not going to revert this until there is a proper fix rather than just shuffling around evaluation order.

@oxij
Copy link
Member

@oxij oxij commented on c5bb431 Sep 26, 2018 via email

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aszlig
Copy link
Member Author

@aszlig aszlig commented on c5bb431 Sep 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem here is that nixos-manual.nix now requires that you only evaluate via config.system.build and any other way will potentially trigger an evaluation error, for example (eg. when on 563d5b1):

nix-instantiate nixos --arg configuration '{ services.nixosManual.enable = false; services.nixosManual.showManual = true; }' -A config.system.build.etc

... and really anything that triggers unboxing config.systemd.services.nixos-manual.

In any case, I think the change to nixos/modules/profiles/installation-device.nix here is superfluous

It is not, because it defaults to false for the tests:

nixpkgs/nixos/lib/build-vms.nix

Line 31 in e0f4610

{ key = "no-manual"; documentation.nixos.enable = false; }

While setting it to true in the installation-device profile is a no-op for eg. building an ISO image it doesn't hurt to set it to false here instead of adding another special case in say test-instrumentation.nix and giving a comment referring to installation-device.nix.

@oxij
Copy link
Member

@oxij oxij commented on c5bb431 Sep 30, 2018 via email

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please sign in to comment.