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

Minor module improvements #82751

Merged
merged 3 commits into from Mar 18, 2020
Merged

Conversation

infinisil
Copy link
Member

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

Ping @roberth

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

Copy link
Member

@teto teto left a comment

Choose a reason for hiding this comment

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

glad we won't have to filter _module anymore

lib/modules.nix Outdated Show resolved Hide resolved
lib/modules.nix Show resolved Hide resolved
lib/modules.nix Outdated
@@ -410,7 +415,7 @@ rec {
# Type-check the remaining definitions, and merge them. Or throw if no definitions.
mergedValue =
if isDefined then
foldl' (res: def:
foldl (res: def:
Copy link
Contributor

Choose a reason for hiding this comment

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

why?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

wow, thanks

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

res

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

@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 infinisil force-pushed the minor-module-improvements branch 2 times, most recently from b498b1d to 62ad9c1 Compare March 17, 2020 18:26
lib/modules.nix Outdated
@@ -407,13 +413,16 @@ rec {
};
defsFinal = defsFinal'.values;

invalidDefinitionExceptionOrNull =
Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

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?"
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

@roberth roberth merged commit 5f357b2 into NixOS:master Mar 18, 2020
@infinisil infinisil mentioned this pull request Mar 19, 2020
1 task
@infinisil infinisil added the 6.topic: module system About NixOS module system internals label Mar 19, 2020
@samueldr
Copy link
Member

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

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 infinisil mentioned this pull request Mar 19, 2020
1 task
@infinisil
Copy link
Member Author

@samueldr Thanks for the report! Fixed in #82960

@infinisil 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
Though with *our* modules.

With NixOS/nixpkgs#82960 this fixes the
regression from NixOS/nixpkgs#82751.
infinisil added a commit to infinisil/nixus that referenced this pull request Mar 28, 2020
And adapt for change in NixOS/nixpkgs#82751
@nh2
Copy link
Contributor

nh2 commented Nov 4, 2021

@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
Copy link
Member

roberth commented Nov 4, 2021

@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
Copy link
Member

roberth commented Nov 4, 2021

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.

J4NV5 pushed a commit to J4NV5/DarkOps that referenced this pull request Dec 2, 2021
And adapt for change in NixOS/nixpkgs#82751
@nh2
Copy link
Contributor

nh2 commented Jan 3, 2022

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

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

Successfully merging this pull request may close these issues.

None yet

6 participants