hercules-ci / flake-parts

❄️ Simplify Nix Flakes with the module system
https://flake.parts
MIT License
733 stars 41 forks source link

Include `assertions` and `warnings` in flake-parts? #230

Open Skarlett opened 3 months ago

Skarlett commented 3 months ago

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 commented 3 months ago

Assertions have some conflicting requirements:

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 https://github.com/NixOS/nixpkgs/pull/97023#event-4129747018.

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 commented 2 weeks ago

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

roberth commented 2 weeks ago

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 commented 2 weeks ago

I see. Settings assertions for individual packages makes more since.

https://github.com/nix-community/dream2nix/blob/b76c529f377100516c40c5b6e239a4525fdcabe0/modules/dream2nix/core/assertions.nix#L4