Minor module improvements by infinisil · Pull Request #82751 · NixOS/nixpkgs

infinisil · 2020-03-16T20:47:00Z

Motivation for this change

Some tiny improvements to the module system:

Removing _module from the evaluation result, this will be very useful for Freeform modules #82743, as it means that people won't have to filter out that key manually from every submodule that's used.
Improving one error message
Fixing another error message

Previous discussion regarding the removal of _module is in #82461 (comment), where @roberth mentioned that he uses it for debugging sometimes. This could still be exposed via an explicit and non-module-internal option specifically for NixOS modules though.

Things done

Implemented tests for the error message bits
Successfully evaluated my system configs with the _module change

teto

glad we won't have to filter _module anymore

lib/modules.nix

danbst · 2020-03-17T16:23:10Z

lib/modules.nix

Because foldl' is the strict version, it would evaluate res (which is type.merge loc defsFinal) before checking all the types, so a merging of the types would be attempted even before they are all checked to be of the correct type, which would lead to an error Conflicting definitions rather than is not of type. See the test I added for this, which would not succeed without this change.

wow, thanks

res didn't need to go through the fold. I've contributed a fix for the potential stack overflow resulting from foldl.

res

I mean the merge result. That didn't have to be the fold "accumulator".

@roberth your commit was force-pushed, it seems. I've noticed it existed but now it is gone.

@roberth (I accidentally force pushed over your commit, but I squashed it back together with my fix for this now)

The _module option is added as an internal option set, and it messes up the results of module evaluations, requiring people to manually filter _modules out. If people depend on this, they can still use config._module from inside the modules, exposing _module as an explicitly declared user option. Or alternatively with the _module attribute now returned by evalModules.

infinisil · 2020-03-17T18:29:00Z

lib/modules.nix

@roberth I don't think this needs to be exposed though, it can be a let in within the mergedValue instead

Didn't realize it was exposed. Yes, that is ok.

Found an imo better way to do this instead, which I'm now using for both changes here

Co-Authored-By: Robert Hensing <robert@roberthensing.nl>

infinisil · 2020-03-18T03:46:29Z

lib/modules.nix

-            mergeModules' loc decls defns
+            if all (def: isAttrs def.value) defns' then mergeModules' loc decls defns
+            else let firstInvalid = findFirst (def: ! isAttrs def.value) null defns';
+            in throw "The option path `${showOption loc}' is an attribute set of options, but it is defined to not be an attribute set in `${firstInvalid.file}'. Did you define its value at the correct and complete path?"


Only in the case of a failure does this code actually try to figure out which element is problematic. For the successful case only the all builtin is used.

samueldr · 2020-03-19T19:55:40Z

_module still is used by nixpkgs/eval-config.nix.

nixpkgs/nixos/lib/eval-config.nix

Line 50 in 2719f56

_module.args.pkgs = lib.mkIf (pkgs_ != null) (lib.mkForce pkgs_);
nixpkgs/nixos/lib/eval-config.nix

Line 72 in 2719f56

inherit (config._module.args) pkgs;

This ends up breaking the Mobile NixOS eval.

The current implementation has a copy of that file, but I just tried locally with an import re-using the one from Nixpkgs and it fails the same way:

trace: **********************************
trace: * Evaluating device: qemu-x86_64 *
trace: **********************************
trace: WARNING: evaluation includes ./local.nix
error: attribute '_module' missing, at /Users/samuel/Projects/mobile-nixos/nixpkgs/nixos/lib/eval-config.nix:72:12
(use '--show-trace' to show detailed location information)

I'll be coming soon with better repro info and a deeper investigation.

infinisil · 2020-03-19T20:22:55Z

@samueldr Thanks for the report! Fixed in #82960

Though with *our* modules. With NixOS/nixpkgs#82960 this fixes the regression from NixOS/nixpkgs#82751.

And adapt for change in NixOS/nixpkgs#82751

nh2 · 2021-11-04T00:59:03Z

@infinisil I used the following as an evaluation-time check against accidentally mis-spelling systemd service names, like so:

# Example:
#   serviceUnitOf cfg.systemd.services.myservice == "myservice.service"

serviceUnitOf = service: "${service._module.args.name}.service";

It errored nicely when you tried to refer to a service that wasn't actually available.

This now no longer works in the places I use it (e.g. top-level nixops deployments).

Do you happen to know what I can use instead?

roberth · 2021-11-04T12:59:57Z

@nh2 The systemd module could be improved to provide this. Something like:

$ cd nixpkgs
$ nix repl .
nix-repl> n = nixos {}

nix-repl> n.config.systemd.services.nix-daemon.unitName
"nix-daemon.service"

Proof of concept:

diff --git a/nixos/modules/system/boot/systemd-unit-options.nix b/nixos/modules/system/boot/systemd-unit-options.nix
index 4154389b2ce..69c764df719 100644
--- a/nixos/modules/system/boot/systemd-unit-options.nix
+++ b/nixos/modules/system/boot/systemd-unit-options.nix
@@ -43,6 +43,16 @@ in rec {
       '';
     };
 
+    unitName = mkOption {
+      type = types.str;
+      description = ''
+        The name of the unit, such as <literal>foo.service</literal>, <literal>bar.socket</literal>, etc.
+
+        This value is derived from the name of the attribute that contains the unit. It can not be set. If you need to base the name on a variable, use a dynamic attribute. See <link xlink:href="https://nixos.org/manual/nix/stable/expressions/language-values.html#sets">https://nixos.org/manual/nix/stable/expressions/language-values.html#sets</link>
+      '';
+      readOnly = true;
+    };
+
     requiredBy = mkOption {
       default = [];
       type = types.listOf types.str;
diff --git a/nixos/modules/system/boot/systemd.nix b/nixos/modules/system/boot/systemd.nix
index 8fcf62d7fbf..9427f9988b6 100644
--- a/nixos/modules/system/boot/systemd.nix
+++ b/nixos/modules/system/boot/systemd.nix
@@ -262,7 +262,9 @@ let
 
   serviceConfig = { name, config, ... }: {
     config = mkMerge
-      [ { # Default path for systemd services.  Should be quite minimal.
+      [ {
+          unitName = "${name}.service";
+          # Default path for systemd services.  Should be quite minimal.
           path = mkAfter
             [ pkgs.coreutils
               pkgs.findutils
@@ -345,7 +347,7 @@ let
                 "Environment=${builtins.toJSON "${n}=${env.${n}}"}\n";
               # systemd max line length is now 1MiB
               # https://github.com/systemd/systemd/commit/e6dde451a51dc5aaa7f4d98d39b8fe735f73d2af
-              in if stringLength s >= 1048576 then throw "The value of the environment variable ‘${n}’ in systemd service ‘${name}.service’ is too long." else s) (attrNames env)}
+              in if stringLength s >= 1048576 then throw "The value of the environment variable ‘${n}’ in systemd service ‘${def.unitName}’ is too long." else s) (attrNames env)}
           ${if def.reloadIfChanged then ''
             X-ReloadIfChanged=true
           '' else if !def.restartIfChanged then ''
@@ -1114,7 +1116,7 @@ in
 
     systemd.units =
          mapAttrs' (n: v: nameValuePair "${n}.path"    (pathToUnit    n v)) cfg.paths
-      // mapAttrs' (n: v: nameValuePair "${n}.service" (serviceToUnit n v)) cfg.services
+      // mapAttrs' (n: v: nameValuePair v.unitName     (serviceToUnit n v)) cfg.services
       // mapAttrs' (n: v: nameValuePair "${n}.slice"   (sliceToUnit   n v)) cfg.slices
       // mapAttrs' (n: v: nameValuePair "${n}.socket"  (socketToUnit  n v)) cfg.sockets
       // mapAttrs' (n: v: nameValuePair "${n}.target"  (targetToUnit  n v)) cfg.targets
@@ -1128,7 +1130,7 @@ in
 
     systemd.user.units =
          mapAttrs' (n: v: nameValuePair "${n}.path"    (pathToUnit    n v)) cfg.user.paths
-      // mapAttrs' (n: v: nameValuePair "${n}.service" (serviceToUnit n v)) cfg.user.services
+      // mapAttrs' (n: v: nameValuePair v.unitName     (serviceToUnit n v)) cfg.user.services
       // mapAttrs' (n: v: nameValuePair "${n}.slice"   (sliceToUnit   n v)) cfg.user.slices
       // mapAttrs' (n: v: nameValuePair "${n}.socket"  (socketToUnit  n v)) cfg.user.sockets
       // mapAttrs' (n: v: nameValuePair "${n}.target"  (targetToUnit  n v)) cfg.user.targets

roberth · 2021-11-04T13:02:08Z

Actually the systemd module can be cleaned up a lot and be more useful, but I have my hands full with improvements in other areas.

And adapt for change in NixOS/nixpkgs#82751

nh2 · 2022-01-03T18:51:38Z

I have extracted the above discussion on evaluation-time checking of systemd units into its own issue for discussion: #153369

infinisil requested review from edolstra and nbp as code owners March 16, 2020 20:47

infinisil mentioned this pull request Mar 16, 2020

Freeform modules #82743

Merged

4 tasks

ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux. labels Mar 16, 2020

teto approved these changes Mar 16, 2020

View reviewed changes

roberth requested changes Mar 17, 2020

View reviewed changes

lib/modules.nix Outdated Show resolved Hide resolved

lib/modules.nix Outdated Show resolved Hide resolved

danbst reviewed Mar 17, 2020

View reviewed changes

infinisil force-pushed the minor-module-improvements branch 2 times, most recently from b498b1d to 62ad9c1 Compare March 17, 2020 18:26

infinisil commented Mar 17, 2020

View reviewed changes

infinisil and others added 2 commits March 18, 2020 04:38

lib/modules: Throw better error when definitions assign to an option set

15c873b

lib/modules: Fix type checks not being done before merging

e931de5

Co-Authored-By: Robert Hensing <robert@roberthensing.nl>

infinisil force-pushed the minor-module-improvements branch from 62ad9c1 to e931de5 Compare March 18, 2020 03:39

infinisil commented Mar 18, 2020

View reviewed changes

danbst approved these changes Mar 18, 2020

View reviewed changes

roberth merged commit 5f357b2 into NixOS:master Mar 18, 2020

infinisil mentioned this pull request Mar 19, 2020

Fix dependent option sets #82915

Merged

1 task

infinisil added the 6.topic: module system About "NixOS" module system internals label Mar 19, 2020

infinisil mentioned this pull request Mar 19, 2020

nixos: Fix pkgs exporting #82960

Merged

1 task

infinisil deleted the minor-module-improvements branch March 19, 2020 20:23

samueldr added a commit to samueldr-wip/mobile-nixos-wip that referenced this pull request Mar 19, 2020

lib/eval-config.nix: Directly use the upstream one

ddfd04c

Though with *our* modules. With NixOS/nixpkgs#82960 this fixes the regression from NixOS/nixpkgs#82751.

samueldr mentioned this pull request Mar 19, 2020

lib/eval-config.nix: Directly use the upstream one mobile-nixos/mobile-nixos#101

Merged

infinisil added a commit to infinisil/nixus that referenced this pull request Mar 28, 2020

Update nixpkgs

432a624

And adapt for change in NixOS/nixpkgs#82751

infinisil mentioned this pull request Sep 29, 2020

_module.args.name gets merged on assignment #53458

Closed

roberth mentioned this pull request Nov 21, 2021

Move systemd-lib.nix and systemd-unit-options.nix into utils #146815

Merged

J4NV5 pushed a commit to J4NV5/DarkOps that referenced this pull request Dec 2, 2021

Update nixpkgs

9e3505b

And adapt for change in NixOS/nixpkgs#82751

nh2 mentioned this pull request Jan 3, 2022

Evaluation-time checking of systemd units #153369

Open

Uh oh!

Conversation

infinisil commented Mar 16, 2020

Motivation for this change

Things done

Uh oh!

teto left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

samueldr commented Mar 19, 2020

Uh oh!

infinisil commented Mar 19, 2020

Uh oh!

nh2 commented Nov 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

roberth commented Nov 4, 2021

Uh oh!

roberth commented Nov 4, 2021

Uh oh!

nh2 commented Jan 3, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

nh2 commented Nov 4, 2021 •

edited

Loading