lib/modules: Allow an "anonymous" module with key in disabledModules #211855

roberth · 2023-01-21T00:51:31Z

Description of changes

This makes the following work

disabledModules = [ foo.nixosModules.bar ];

even if bar is not a path, but rather a module such as

{ key = "/path/to/foo#nixosModules.bar"; config = ...; }

By supporting this, the user will often be able to use the same syntax for both importing and disabling a module. This is becoming more relevant because flakes promote the use of attributes to reference modules. Not all of these modules in flake attributes will be identifiable, but with the help of a framework such as flake-parts, these attributes can be guaranteed to be identifiable (by outPath + attribute path).

Things done

infinisil · 2023-01-31T18:52:25Z

lib/modules.nix

+            else if isFunction m
+            then
+              let mAttrs = m fakeArgs;
+                  fakeArgs =
+                    mapAttrs
+                      (k: v: throw "If a module in 'disabledModules' is not referenced by its path, it must be identified by a 'key' attribute. Retrieving this attribute failed for ${theModule} in the 'disabledModules' of a module in '${file}', because its 'key' attribute requires the module argument '${lib.strings.escapeNixIdentifier k}' to be computed first. This is not possible, because the set of applicable modules must be determined before any configuration can be computed.")


I don't think keys behind functions should be allowed. Flake-aware code can map over nixosModules.*, wrapping each using module: { key = "<flake>#<attr>"; imports = [ module ]; }. Providing fake arguments won't be necessary like that.

Also by not allowing functions, this change can be seen as just allowing modules to be coerced to a string in a different way using .key, similar to .outPath and .__toString, which sounds more reasonable to me, because evidently from this code here, disabledModules and imports are fundamentally different (disabledModules only needs a reference to a module, while imports needs the module itself).

However an alternative idea I'd be more in favor of is to require all imports to have a key. So you couldn't just do imports = [ {} ] anymore, it would have to be imports = [ { key = "something"; } ], the key can't be behind the fixed point though. This then ensures that all modules have a stable reference that can be used to disable them. And it would also allow using all modules in both imports and disabledModules.

I've removed the possibility to specify a key in a function. It's good to be careful like that, and I don't think it's a problem for users to remove this functionality.

However an alternative idea I'd be more in favor of is to require all imports to have a key.

I don't see the benefit. The benefit comes from having more keys. Requiring those is a rather harsh incentive for such a small improvement.

Anyway, that's just future work if anything.

infinisil

It would be good to have some docs for this as well, otherwise this looks good to me.

lib/tests/modules/disable-module-with-key.nix

infinisil · 2023-02-04T00:27:35Z

lib/modules.nix

+            else if isPath m || isString m
+            then toString m


I'm just noticing that the previous code allowed any string-coercible value to be used. So we should arguably keep that behavior.

Suggested change

else if isPath m || isString m

then toString m

else if isCoercibleToString m

then toString m

I guess the previous string-coercing behavior wouldn't have worked for your use-case though, because neither outPath nor __toString are allowed attribute names in modules.

I've added a test for this, refactoring the code a bit and rejecting ambiguities between toString and .key.

Also fyi that function will enter deprecation in summer.

isCoercibleToString = lib.warnIf (lib.isInOldestRelease 2305) "lib.strings.isCoercibleToString is deprecated in favor of either isStringLike or isConvertibleWithToString. Only use the latter if it needs to return true for null, numbers, booleans and list of similarly coercibles." isConvertibleWithToString;

lib/modules.nix

infinisil · 2023-02-09T19:52:59Z

lib/modules.nix

+            else if isConvertibleWithToString m
+            then
+              if m?key
+              then
+                throw "Module `${file}` contains a disabledModules item that is an attribute set that can be converted to a string (${toString m}) but also has a `.key` attribute (${m.key}) with a different value. This makes it ambigous which module should be disabled."


Suggested change

else if isConvertibleWithToString m

then

if m?key

then

throw "Module `${file}` contains a disabledModules item that is an attribute set that can be converted to a string (${toString m}) but also has a `.key` attribute (${m.key}) with a different value. This makes it ambigous which module should be disabled."

else if isConvertibleWithToString m

then

if m ? key && m.key != toString m

then

throw "Module `${file}` contains a disabledModules item that is an attribute set that can be converted to a string (${toString m}) but also has a `.key` attribute (${m.key}) with a different value. This makes it ambigous which module should be disabled."

The other messages should technically also be adjusted to include the possibility of using any string-coercible values.

Done.
The other error messages are already ok

the one for non-string attrsets without key should keep recommending only the most explicit option: .key. It is also technically correct, even if it ignores the possibility of fixing the problem by making it toString-able. I'd like to keep the message simple and not explain what I consider irrelevant legacy behavior. If you disagree please leave a suggestion.

the final catch-all one already lists value supported by toString.

infinisil · 2023-02-13T13:40:20Z

Do you want to keep the commits as-is or squash a bit? Squashing a bit would be my preference

This makes the following work disabledModules = [ foo.nixosModules.bar ]; even if `bar` is not a path, but rather a module such as { key = "/path/to/foo#nixosModules.bar"; config = ...; } By supporting this, the user will often be able to use the same syntax for both importing and disabling a module. This is becoming more relevant because flakes promote the use of attributes to reference modules. Not all of these modules in flake attributes will be identifiable, but with the help of a framework such as flake-parts, these attributes can be guaranteed to be identifiable (by outPath + attribute path).

roberth force-pushed the lib-modules-disabledModules-module-with-key branch from 14cf976 to 4d31a4e Compare January 21, 2023 20:25

ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Jan 21, 2023

roberth marked this pull request as ready for review January 21, 2023 20:50

roberth requested review from edolstra, nbp and infinisil as code owners January 21, 2023 20:50

roberth mentioned this pull request Jan 29, 2023

Support disabledModules hercules-ci/flake-parts#111

Merged

infinisil requested changes Jan 31, 2023

View reviewed changes

infinisil added the 6.topic: module system About "NixOS" module system internals label Jan 31, 2023

roberth force-pushed the lib-modules-disabledModules-module-with-key branch 2 times, most recently from 3cca12f to 6f487fe Compare February 1, 2023 13:19

roberth mentioned this pull request Feb 1, 2023

Help with disabling of modules hercules-ci/flake-parts#49

Closed

roberth requested a review from infinisil February 2, 2023 03:53

infinisil requested changes Feb 3, 2023

View reviewed changes

lib/tests/modules/disable-module-with-key.nix Outdated Show resolved Hide resolved

github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: documentation labels Feb 3, 2023

roberth force-pushed the lib-modules-disabledModules-module-with-key branch from 1a3aa2b to eeda7a1 Compare February 3, 2023 21:12

infinisil requested changes Feb 4, 2023

View reviewed changes

roberth requested a review from infinisil February 5, 2023 18:42

infinisil requested changes Feb 6, 2023

View reviewed changes

lib/modules.nix Outdated Show resolved Hide resolved

roberth force-pushed the lib-modules-disabledModules-module-with-key branch from a7730a5 to 4277605 Compare February 7, 2023 18:55

roberth requested a review from infinisil February 7, 2023 22:05

infinisil requested changes Feb 9, 2023

View reviewed changes

roberth force-pushed the lib-modules-disabledModules-module-with-key branch from ebf6729 to 49ebf87 Compare February 13, 2023 10:47

roberth requested a review from infinisil February 13, 2023 10:47

infinisil approved these changes Feb 13, 2023

View reviewed changes

roberth force-pushed the lib-modules-disabledModules-module-with-key branch from 49ebf87 to 118bdf2 Compare March 1, 2023 14:06

roberth merged commit 6b79fe8 into NixOS:master Mar 9, 2023

llakala mentioned this pull request Sep 23, 2024

Documentation: No example is provided for using disabledModules with an attribute set #343792

Open

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lib/modules: Allow an "anonymous" module with key in disabledModules #211855

lib/modules: Allow an "anonymous" module with key in disabledModules #211855

roberth commented Jan 21, 2023 •

edited

Loading

infinisil Jan 31, 2023 •

edited

Loading

roberth Jan 31, 2023

infinisil left a comment •

edited

Loading

infinisil Feb 4, 2023

roberth Feb 4, 2023

infinisil Feb 9, 2023

infinisil Feb 9, 2023

roberth Feb 13, 2023

infinisil commented Feb 13, 2023

lib/modules: Allow an "anonymous" module with key in disabledModules #211855

lib/modules: Allow an "anonymous" module with key in disabledModules #211855

Conversation

roberth commented Jan 21, 2023 • edited Loading

Description of changes

Things done

infinisil Jan 31, 2023 • edited Loading

Choose a reason for hiding this comment

roberth Jan 31, 2023

Choose a reason for hiding this comment

infinisil left a comment • edited Loading

Choose a reason for hiding this comment

infinisil Feb 4, 2023

Choose a reason for hiding this comment

roberth Feb 4, 2023

Choose a reason for hiding this comment

infinisil Feb 9, 2023

Choose a reason for hiding this comment

infinisil Feb 9, 2023

Choose a reason for hiding this comment

roberth Feb 13, 2023

Choose a reason for hiding this comment

infinisil commented Feb 13, 2023

roberth commented Jan 21, 2023 •

edited

Loading

infinisil Jan 31, 2023 •

edited

Loading

infinisil left a comment •

edited

Loading