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

Include assertions and warnings in flake-parts? #230

Open
Skarlett opened this issue Jun 19, 2024 · 4 comments
Open

Include assertions and warnings in flake-parts? #230

Skarlett opened this issue Jun 19, 2024 · 4 comments

Comments

@Skarlett
Copy link

I know that flake-parts is designed to be very minimal and strive to be as close to flakes as possible.

But would you consider the inclusion of config.assertions & config.warnings ?

mkFlake = args: module:
  let
    eval = flake-parts-lib.evalFlakeModule args module;
    failedAssertions = map (x: x.message) (filter (x: !x.assertion) eval.config.assertions);
 in
    lib.warnIf (failedAssertions != []) (lib.stringConcatSep "\n" failedAssertions)  
    eval.config.flake;

I believe the impl would look something like this.

helpful links:

https://github.com/NixOS/nixpkgs/blob/ab35d52e8a1f75a2b5a1bad575e6973f3c9605c7/nixos/modules/misc/assertions.nix
https://github.com/NixOS/nixpkgs/blob/ab35d52e8a1f75a2b5a1bad575e6973f3c9605c7/nixos/modules/system/activation/top-level.nix#L64

@roberth
Copy link
Member

roberth commented Jun 19, 2024

Assertions have some conflicting requirements:

  • For performance, we want attributes, options values and definitions to be as lazy as possible.
  • For the best relevance of error messages, we want assertions to trigger before we access the broken thing the assertion is about
  • For robustness against infinite recursions, we also want everything to be as lazy as possible.
  • Assertions tend to decrease laziness.

NixOS assertions mostly avoid these conflicts by only triggering the assertions in system.build.toplevel, which works well because almost no other options use toplevel, so infinite recursions tend not to be a problem, and almost all usages of a configuration involve evaluating toplevel anyway.

Flakes are different though. We don't have a single flake output that is always evaluated, except for the root attribute set of the outputs, but that's costly in terms of performance, and it will still cause infinite recursions when using the flake self in an assertion or anything that has an assertion about it.

So I don't think assertions.nix is a good match for flake-parts, unfortunately. Something more advanced could work, and perhaps relevant context for this is the discussion after this merge NixOS/nixpkgs#97023 (comment).

I suppose one thing we could try is make the assertions hierarchical, so that you can control which attribute paths trigger which assertions.
Perhaps instead of warnIf foo eval.config.flake, we could do mirrorSeq eval.config.evalChecks eval.config.flake, where

{
  options.evalChecks = mkOption { type = fix attrsOf // { description = "attrsOf (attrsOf (attrsOf ...))"; }; };
}
let
mirrorSeq = checks: ret: builtins.seq checks (mapAttrs (k: v: mirrorSeq checks.${k} v) ret);
{
  evalChecks.packages.default = lib.warnIf (! (config.packages.default?meta)) "packages.default.meta is not defined" {};
  evalChecks.packages = lib.warnIf (config.packages?internal) "packages.internal exists. Is foobar2nix acting up?" {};
}

throwIf also works, but the error messages don't get merged, so you get to see an arbitrary one if an attribute path has multiple assertions. A different solution with more complicated syntax could fix that, but I think this might be good enough for now.

@Atry
Copy link
Contributor

Atry commented Sep 16, 2024

We now have partitions. Putting assertions in the separate test partition seems the best solution.

@roberth
Copy link
Member

roberth commented Sep 17, 2024

I suppose you could, but it does duplicate the whole module evaluation. What is the benefit over, say, triggering the checks as part of a particular attribute, such as an empty definition for flake.checks?

@Atry
Copy link
Contributor

Atry commented Sep 18, 2024

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants