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.

Sign up for GitHub

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

Jump to bottom

nixos: reusable assertions #207187

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

nixos: reusable assertions #207187

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8ea31ec
nixos: Move assertions/warnings implementation to assertions.nix
roberth Dec 22, 2022
19e3d86
nixos/testing: Add test-level assertions and warnings
roberth Dec 22, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions nixos/lib/testing/driver.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ let
lib.warnIf config.skipLint "Linting is disabled";

driver =
config._module.assertAndWarn
hostPkgs.runCommand "nixos-test-driver-${config.name}"
{
# inherit testName; TODO (roberth): need this?
Expand Down
17 changes: 4 additions & 13 deletions nixos/lib/testing/legacy.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,23 +3,14 @@ let
inherit (lib) mkIf mkOption types;
in
{
# This needs options.warnings, which we don't have (yet?).
# imports = [
# (lib.mkRenamedOptionModule [ "machine" ] [ "nodes" "machine" ])
# ];
imports = [
../../modules/misc/assertions.nix
(lib.mkRenamedOptionModule [ "machine" ] [ "nodes" "machine" ])
];

options = {
machine = mkOption {
internal = true;
type = types.raw;
};
};

config = {
nodes = mkIf options.machine.isDefined (
lib.warn
"In test `${config.name}': The `machine' attribute in NixOS tests (pkgs.nixosTest / make-test-python.nix / testing-python.nix / makeTest) is deprecated. Please set the equivalent `nodes.machine'."
{ inherit (config) machine; }
);
};
}
piegamesde marked this conversation as resolved.
Show resolved Hide resolved
31 changes: 28 additions & 3 deletions nixos/modules/misc/assertions.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,21 @@
{ lib, ... }:
{ config, lib, ... }:

with lib;
let
inherit (lib)
filter
concatStringsSep
mkOption
showWarnings
types
;

failedAssertions = map (x: x.message) (filter (x: !x.assertion) config.assertions);

assertAndWarn = x: if failedAssertions != []
then throw "\nFailed assertions:\n${concatStringsSep "\n" (map (x: "- ${x}") failedAssertions)}"
else showWarnings config.warnings x;

in
{

options = {
Expand Down Expand Up @@ -29,6 +43,17 @@ with lib;
'';
};

_module.assertAndWarn = mkOption {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
_module.assertAndWarn = mkOption {
assertAndWarn = mkOption {

The _module namespace should be reserved for module-internal options

Copy link
Member

@ncfavier ncfavier Dec 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Quoting you:

The main insight I had was that _module doesn't have a _ at the start because it's for module-internal things, but rather to avoid clashing with option names. While @roberth pointed out that we don't have a checks option at the root, these options are added to submodules as well. So introducing sugar for this could cause problems for any module or submodule which has a checks option. While there is no such option in nixpkgs, we can't be sure that users don't have one on their own.

This is module-internal in spirit anyway.

Copy link
Member

Choose a reason for hiding this comment

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

That PR was a module-internal implementation of assertions and warnings though, it added support for them to any module (including submodules) since _module.checks is added by default. This is in contrast to this PR here, which is opt-in for modules.

Copy link
Member Author

Choose a reason for hiding this comment

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

Perhaps a middle ground would be for _module.assertAndWarn to be a module-system-standardized interface rather than a full-featured implementation of the concept?

Copy link
Member

Choose a reason for hiding this comment

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

I'd think it would be better then to make it a full-featured implementation. Essentially resurrect #97023 but with opt-in triggering of checks using _module.triggerChecks (like _module.assertAndWarn).

Copy link
Member Author

Choose a reason for hiding this comment

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

Shorthand strikes again. Fair enough.

Copy link
Member Author

Choose a reason for hiding this comment

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

I suppose users such as the NixOS toplevel could alias the checks as an internal config.checks option, at which point we're basically full circle, but at least the intent around the _module.* attributes will be clear?
Seems like a lot of bookkeeping just to create an illusion of a formally approved interface, but ok.

Copy link
Member

Choose a reason for hiding this comment

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

Another idea I just had: What if we just used #97023 as is with checking done strictly by default, but additionally enable customization of when it trigger (and whether they trigger at all). This way we can just add a couple manual fixes on top of the rare breakages.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh no, I forgot to submit my huge response to this. We'll have to make do with a less detailed comment for now.

Laziness is crucial for modularity without infinite recursions. Adding strictness to a module is inherently risky, as we can not know the dependencies that user logic adds. A supposedly helpful warning must never cause evaluations to fail with an infinite recursion. Not even config would be a good enough trigger because the user may need a mutually recursive dependency with another submodule. For the main logic to keep working it must not depend on any checks, because the checks themselves must depend on the main logic. Anything that makes creating such dependencies on checks easy would be a mistake. Instead, we could standardize on specific _module attributes, and leave config itself unencumbered by checks.

This way we can just add a couple manual fixes on top of the rare breakages.

Try to explain this design choice to someone who's confronted with a 120-item eval trace that appears to have no relation to their code, except for two items that are obviously not in a mutually recursive relationship according to any sensible logic. That's if they can even make sense of the data flow in their modules, because yes, that's a plural.

  • just - no, this requires a specific skill
  • manual fixes - so that will have to be us then
  • rare - feel free to play with the crocodile; it rarely bites someone's head off.

Triggering must only happen "automatically" at the ultimate of top level. Anything lower than that is susceptible to infinite recursions when users indirectly create mutual dependencies between, for example, NixOS configurations in a deployment.

Copy link
Member Author

Choose a reason for hiding this comment

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

We're going to need something outside NixOS. It doesn't have to be perfect, and when the time comes, we deprecate the old opt-in solution.

Encountered it again today

internal = true;
type = types.functionTo types.raw;
description = ''
A function that applies the assertions and warnings.
If there are none, it behaves like the identity function, `x: x`.
'';
};
};

config = {
_module.assertAndWarn = assertAndWarn;
};
# impl of assertions is in <nixpkgs/nixos/modules/system/activation/top-level.nix>
}
13 changes: 3 additions & 10 deletions nixos/modules/system/activation/top-level.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,18 +116,11 @@ let
perl = pkgs.perl.withPackages (p: with p; [ ConfigIniFiles FileSlurp ]);
} // config.system.systemBuilderArgs);

# Handle assertions and warnings

failedAssertions = map (x: x.message) (filter (x: !x.assertion) config.assertions);

baseSystemAssertWarn = if failedAssertions != []
then throw "\nFailed assertions:\n${concatStringsSep "\n" (map (x: "- ${x}") failedAssertions)}"
else showWarnings config.warnings baseSystem;

# Replace runtime dependencies
system = foldr ({ oldDependency, newDependency }: drv:
system = config._module.assertAndWarn
foldr ({ oldDependency, newDependency }: drv:
pkgs.replaceDependency { inherit oldDependency newDependency drv; }
) baseSystemAssertWarn config.system.replaceRuntimeDependencies;
) baseSystem config.system.replaceRuntimeDependencies;

in

Expand Down