Merge pull request #211855 from hercules-ci/lib-modules-disabledModules-module-with-key

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

6 files changed, 174 insertions, 5 deletions

diff --git a/lib/modules.nix b/lib/modules.nix
index 5e6bee6aabe38..051dbe2ef896a 100644
--- a/lib/modules.nix
+++ b/lib/modules.nix
@@ -21,6 +21,7 @@ let
     isBool
     isFunction
     isList
+    isPath
     isString
     length
     mapAttrs
@@ -45,6 +46,9 @@ let
     showOption
     unknownModule
     ;
+  inherit (lib.strings)
+    isConvertibleWithToString
+    ;
 
   showDeclPrefix = loc: decl: prefix:
     " - option(s) with prefix `${showOption (loc ++ [prefix])}' in module `${decl._file}'";
@@ -403,7 +407,7 @@ rec {
             key = module.key;
             module = module;
             modules = collectedImports.modules;
-            disabled = module.disabledModules ++ collectedImports.disabled;
+            disabled = (if module.disabledModules != [] then [{ file = module._file; disabled = module.disabledModules; }] else []) ++ collectedImports.disabled;
           }) initialModules);
 
       # filterModules :: String -> { disabled, modules } -> [ Module ]
@@ -412,10 +416,30 @@ rec {
       # modules recursively. It returns the final list of unique-by-key modules
       filterModules = modulesPath: { disabled, modules }:
         let
-          moduleKey = m: if isString m && (builtins.substring 0 1 m != "/")
-            then toString modulesPath + "/" + m
-            else toString m;
-          disabledKeys = map moduleKey disabled;
+          moduleKey = file: m:
+            if isString m
+            then
+              if builtins.substring 0 1 m == "/"
+              then m
+              else toString modulesPath + "/" + m
+
+            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 ambiguous which module should be disabled."
+              else
+                toString m
+
+            else if m?key
+            then
+              m.key
+
+            else if isAttrs m
+            then throw "Module `${file}` contains a disabledModules item that is an attribute set, presumably a module, that does not have a `key` attribute. This means that the module system doesn't have any means to identify the module that should be disabled. Make sure that you've put the correct value in disabledModules: a string path relative to modulesPath, a path value, or an attribute set with a `key` attribute."
+            else throw "Each disabledModules item must be a path, string, or a attribute set with a key attribute, or a value supported by toString. However, one of the disabledModules items in `${toString file}` is none of that, but is of type ${builtins.typeOf m}.";
+
+          disabledKeys = concatMap ({ file, disabled }: map (moduleKey file) disabled) disabled;
           keyFilter = filter (attrs: ! elem attrs.key disabledKeys);
         in map (attrs: attrs.module) (builtins.genericClosure {
           startSet = keyFilter modules;
diff --git a/lib/tests/modules.sh b/lib/tests/modules.sh
index cde4da6439372..8081b186a2f9f 100755
--- a/lib/tests/modules.sh
+++ b/lib/tests/modules.sh
@@ -141,6 +141,14 @@ checkConfigError "The option .*enable.* does not exist. Definition values:\n\s*-
 checkConfigError "attribute .*enable.* in selection path .*config.enable.* not found" "$@" ./disable-define-enable.nix ./disable-declare-enable.nix
 checkConfigError "attribute .*enable.* in selection path .*config.enable.* not found" "$@" ./disable-enable-modules.nix
 
+checkConfigOutput '^true$' 'config.positive.enable' ./disable-module-with-key.nix
+checkConfigOutput '^false$' 'config.negative.enable' ./disable-module-with-key.nix
+checkConfigError 'Module ..*disable-module-bad-key.nix. contains a disabledModules item that is an attribute set, presumably a module, that does not have a .key. attribute. .*' 'config.enable' ./disable-module-bad-key.nix
+
+# Not sure if we want to keep supporting module keys that aren't strings, paths or v?key, but we shouldn't remove support accidentally.
+checkConfigOutput '^true$' 'config.positive.enable' ./disable-module-with-toString-key.nix
+checkConfigOutput '^false$' 'config.negative.enable' ./disable-module-with-toString-key.nix
+
 # Check _module.args.
 set -- config.enable ./declare-enable.nix ./define-enable-with-custom-arg.nix
 checkConfigError 'while evaluating the module argument .*custom.* in .*define-enable-with-custom-arg.nix.*:' "$@"
@@ -358,6 +366,10 @@ checkConfigOutput '^"The option `a\.b. defined in `.*/doRename-warnings\.nix. ha
   config.result \
   ./doRename-warnings.nix
 
+# Anonymous modules get deduplicated by key
+checkConfigOutput '^"pear"$' config.once.raw ./merge-module-with-key.nix
+checkConfigOutput '^"pear\\npear"$' config.twice.raw ./merge-module-with-key.nix
+
 cat <<EOF
 ====== module tests ======
 $pass Pass
diff --git a/lib/tests/modules/disable-module-bad-key.nix b/lib/tests/modules/disable-module-bad-key.nix
new file mode 100644
index 0000000000000..f50d06f2f03cd
--- /dev/null
+++ b/lib/tests/modules/disable-module-bad-key.nix
@@ -0,0 +1,16 @@
+{ lib, ... }:
+let
+  inherit (lib) mkOption types;
+
+  moduleWithKey = { config, ... }: {
+    config = {
+      enable = true;
+    };
+  };
+in
+{
+  imports = [
+    ./declare-enable.nix
+  ];
+  disabledModules = [ { } ];
+}
diff --git a/lib/tests/modules/disable-module-with-key.nix b/lib/tests/modules/disable-module-with-key.nix
new file mode 100644
index 0000000000000..ea2a60aa832d0
--- /dev/null
+++ b/lib/tests/modules/disable-module-with-key.nix
@@ -0,0 +1,34 @@
+{ lib, ... }:
+let
+  inherit (lib) mkOption types;
+
+  moduleWithKey = {
+    key = "disable-module-with-key.nix#moduleWithKey";
+    config = {
+      enable = true;
+    };
+  };
+in
+{
+  options = {
+    positive = mkOption {
+      type = types.submodule {
+        imports = [
+          ./declare-enable.nix
+          moduleWithKey
+        ];
+      };
+      default = {};
+    };
+    negative = mkOption {
+      type = types.submodule {
+        imports = [
+          ./declare-enable.nix
+          moduleWithKey
+        ];
+        disabledModules = [ moduleWithKey ];
+      };
+      default = {};
+    };
+  };
+}
diff --git a/lib/tests/modules/disable-module-with-toString-key.nix b/lib/tests/modules/disable-module-with-toString-key.nix
new file mode 100644
index 0000000000000..3f8c81904ce67
--- /dev/null
+++ b/lib/tests/modules/disable-module-with-toString-key.nix
@@ -0,0 +1,34 @@
+{ lib, ... }:
+let
+  inherit (lib) mkOption types;
+
+  moduleWithKey = {
+    key = 123;
+    config = {
+      enable = true;
+    };
+  };
+in
+{
+  options = {
+    positive = mkOption {
+      type = types.submodule {
+        imports = [
+          ./declare-enable.nix
+          moduleWithKey
+        ];
+      };
+      default = {};
+    };
+    negative = mkOption {
+      type = types.submodule {
+        imports = [
+          ./declare-enable.nix
+          moduleWithKey
+        ];
+        disabledModules = [ 123 ];
+      };
+      default = {};
+    };
+  };
+}
diff --git a/lib/tests/modules/merge-module-with-key.nix b/lib/tests/modules/merge-module-with-key.nix
new file mode 100644
index 0000000000000..21f00e6ef9766
--- /dev/null
+++ b/lib/tests/modules/merge-module-with-key.nix
@@ -0,0 +1,49 @@
+{ lib, ... }:
+let
+  inherit (lib) mkOption types;
+
+  moduleWithoutKey = {
+    config = {
+      raw = "pear";
+    };
+  };
+
+  moduleWithKey = {
+    key = __curPos.file + "#moduleWithKey";
+    config = {
+      raw = "pear";
+    };
+  };
+
+  decl = {
+    options = {
+      raw = mkOption {
+        type = types.lines;
+      };
+    };
+  };
+in
+{
+  options = {
+    once = mkOption {
+      type = types.submodule {
+        imports = [
+          decl
+          moduleWithKey
+          moduleWithKey
+        ];
+      };
+      default = {};
+    };
+    twice = mkOption {
+      type = types.submodule {
+        imports = [
+          decl
+          moduleWithoutKey
+          moduleWithoutKey
+        ];
+      };
+      default = {};
+    };
+  };
+}