From 155d248729b88e70f0bf1e2af33bb8003b0eab2d Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Fri, 10 Jun 2022 11:31:49 +0200 Subject: [PATCH 1/3] lib/modules: Make unifyModuleSyntax key optional, remove extensionOffset Tried to make it work in #177080 but that still didn't solve all cases in practice (wip #176557). Found a simpler solution: don't number anonymous modules at all. Keys are assigned at a later stage anyway. We don't have to reinvent that solution in submoduleWith. --- lib/modules.nix | 14 +++++--------- lib/types.nix | 12 +++--------- 2 files changed, 8 insertions(+), 18 deletions(-) diff --git a/lib/modules.nix b/lib/modules.nix index 48ddb31508f25..d823773a2ff8e 100644 --- a/lib/modules.nix +++ b/lib/modules.nix @@ -113,10 +113,6 @@ rec { args ? {} , # This would be remove in the future, Prefer _module.check option instead. check ? true - # Internal variable to avoid `_key` collisions regardless - # of `extendModules`. Used in `submoduleWith`. - # Test case: lib/tests/modules, "168767" - , extensionOffset ? 0 }: let withWarnings = x: @@ -345,17 +341,15 @@ rec { modules ? [], specialArgs ? {}, prefix ? [], - extensionOffset ? length modules, }: evalModules (evalModulesArgs // { modules = regularModules ++ modules; specialArgs = evalModulesArgs.specialArgs or {} // specialArgs; prefix = extendArgs.prefix or evalModulesArgs.prefix or []; - inherit extensionOffset; }); type = lib.types.submoduleWith { - inherit modules specialArgs extensionOffset; + inherit modules specialArgs; }; result = withWarnings { @@ -461,19 +455,21 @@ rec { throw "Module `${key}' has an unsupported attribute `${head (attrNames badAttrs)}'. This is caused by introducing a top-level `config' or `options' attribute. Add configuration attributes immediately on the top level instead, or move all of them (namely: ${toString (attrNames badAttrs)}) into the explicit `config' attribute." else { _file = toString m._file or file; - key = toString m.key or key; disabledModules = m.disabledModules or []; imports = m.imports or []; options = m.options or {}; config = addFreeformType (addMeta (m.config or {})); + } // optionalAttrs (m?key || key != null) { + key = toString m.key or key; } else { _file = toString m._file or file; - key = toString m.key or key; disabledModules = m.disabledModules or []; imports = m.require or [] ++ m.imports or []; options = {}; config = addFreeformType (addMeta (removeAttrs m ["_file" "key" "disabledModules" "require" "imports" "freeformType"])); + } // optionalAttrs (m?key || key != null) { + key = toString m.key or key; }; applyModuleArgsIfFunction = key: f: args@{ config, options, lib, ... }: if isFunction f then diff --git a/lib/types.nix b/lib/types.nix index 147b92f784c9d..4a224099c9250 100644 --- a/lib/types.nix +++ b/lib/types.nix @@ -571,11 +571,6 @@ rec { , specialArgs ? {} , shorthandOnlyDefinesConfig ? false , description ? null - - # Internal variable to avoid `_key` collisions regardless - # of `extendModules`. Wired through by `evalModules`. - # Test case: lib/tests/modules, "168767" - , extensionOffset ? 0 }@attrs: let inherit (lib.modules) evalModules; @@ -584,14 +579,14 @@ rec { then value: value else value: { config = value; }; - allModules = defs: imap1 (n: { value, file }: + allModules = defs: map ({ value, file }: if isFunction value then setFunctionArgs - (args: lib.modules.unifyModuleSyntax file "${toString file}-${toString (n + extensionOffset)}" (value args)) + (args: lib.modules.unifyModuleSyntax file null (value args)) (functionArgs value) else if isAttrs value then - lib.modules.unifyModuleSyntax file "${toString file}-${toString (n + extensionOffset)}" (shorthandToModule value) + lib.modules.unifyModuleSyntax file null (shorthandToModule value) else value ) defs; @@ -632,7 +627,6 @@ rec { (base.extendModules { modules = [ { _module.args.name = last loc; } ] ++ allModules defs; prefix = loc; - extensionOffset = extensionOffset + length defs; }).config; emptyValue = { value = {}; }; getSubOptions = prefix: (base.extendModules From 1d47fcaee584268f38249e500903bb85aedfc025 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Fri, 10 Jun 2022 12:18:03 +0200 Subject: [PATCH 2/3] lib/modules: Make unifyModuleSyntax key required, use setDefaultModuleLocation This should be equivalent and perhaps perform better. --- lib/modules.nix | 6 ++---- lib/types.nix | 19 +++++++++---------- 2 files changed, 11 insertions(+), 14 deletions(-) diff --git a/lib/modules.nix b/lib/modules.nix index d823773a2ff8e..d8ae497fb2d84 100644 --- a/lib/modules.nix +++ b/lib/modules.nix @@ -455,21 +455,19 @@ rec { throw "Module `${key}' has an unsupported attribute `${head (attrNames badAttrs)}'. This is caused by introducing a top-level `config' or `options' attribute. Add configuration attributes immediately on the top level instead, or move all of them (namely: ${toString (attrNames badAttrs)}) into the explicit `config' attribute." else { _file = toString m._file or file; + key = toString m.key or key; disabledModules = m.disabledModules or []; imports = m.imports or []; options = m.options or {}; config = addFreeformType (addMeta (m.config or {})); - } // optionalAttrs (m?key || key != null) { - key = toString m.key or key; } else { _file = toString m._file or file; + key = toString m.key or key; disabledModules = m.disabledModules or []; imports = m.require or [] ++ m.imports or []; options = {}; config = addFreeformType (addMeta (removeAttrs m ["_file" "key" "disabledModules" "require" "imports" "freeformType"])); - } // optionalAttrs (m?key || key != null) { - key = toString m.key or key; }; applyModuleArgsIfFunction = key: f: args@{ config, options, lib, ... }: if isFunction f then diff --git a/lib/types.nix b/lib/types.nix index 4a224099c9250..99dc54518c748 100644 --- a/lib/types.nix +++ b/lib/types.nix @@ -579,16 +579,15 @@ rec { then value: value else value: { config = value; }; - allModules = defs: map ({ value, file }: - if isFunction value - then setFunctionArgs - (args: lib.modules.unifyModuleSyntax file null (value args)) - (functionArgs value) - else if isAttrs value - then - lib.modules.unifyModuleSyntax file null (shorthandToModule value) - else value - ) defs; + allModules = defs: + map + ({ value, file }: + if isAttrs value + then { _file = file; imports = [ (shorthandToModule value) ]; } + else if isFunction value + then { _file = file; imports = [ value ]; } + else value) + defs; base = evalModules { inherit specialArgs; From 5bb1e0c7720cb30961e82f7fb3b8d265ad2cf4f3 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Fri, 10 Jun 2022 13:13:48 +0200 Subject: [PATCH 3/3] Revert "lib/modules: Make unifyModuleSyntax key required, use setDefaultModuleLocation" The performance did not improve, judging by the memory statistics. Baseline: https://github.com/NixOS/nixpkgs/pull/177157/checks?check_run_id=6828895472 Without optionalAttrs: https://github.com/NixOS/nixpkgs/pull/177157/checks?check_run_id=6829274815 So the memory cost of the reverted commit was about 0.05%. This reverts commit bb8461b4cb05bd468e34f2191cdea28dcd01e70a. --- lib/modules.nix | 6 ++++-- lib/types.nix | 19 ++++++++++--------- 2 files changed, 14 insertions(+), 11 deletions(-) diff --git a/lib/modules.nix b/lib/modules.nix index d8ae497fb2d84..d823773a2ff8e 100644 --- a/lib/modules.nix +++ b/lib/modules.nix @@ -455,19 +455,21 @@ rec { throw "Module `${key}' has an unsupported attribute `${head (attrNames badAttrs)}'. This is caused by introducing a top-level `config' or `options' attribute. Add configuration attributes immediately on the top level instead, or move all of them (namely: ${toString (attrNames badAttrs)}) into the explicit `config' attribute." else { _file = toString m._file or file; - key = toString m.key or key; disabledModules = m.disabledModules or []; imports = m.imports or []; options = m.options or {}; config = addFreeformType (addMeta (m.config or {})); + } // optionalAttrs (m?key || key != null) { + key = toString m.key or key; } else { _file = toString m._file or file; - key = toString m.key or key; disabledModules = m.disabledModules or []; imports = m.require or [] ++ m.imports or []; options = {}; config = addFreeformType (addMeta (removeAttrs m ["_file" "key" "disabledModules" "require" "imports" "freeformType"])); + } // optionalAttrs (m?key || key != null) { + key = toString m.key or key; }; applyModuleArgsIfFunction = key: f: args@{ config, options, lib, ... }: if isFunction f then diff --git a/lib/types.nix b/lib/types.nix index 99dc54518c748..4a224099c9250 100644 --- a/lib/types.nix +++ b/lib/types.nix @@ -579,15 +579,16 @@ rec { then value: value else value: { config = value; }; - allModules = defs: - map - ({ value, file }: - if isAttrs value - then { _file = file; imports = [ (shorthandToModule value) ]; } - else if isFunction value - then { _file = file; imports = [ value ]; } - else value) - defs; + allModules = defs: map ({ value, file }: + if isFunction value + then setFunctionArgs + (args: lib.modules.unifyModuleSyntax file null (value args)) + (functionArgs value) + else if isAttrs value + then + lib.modules.unifyModuleSyntax file null (shorthandToModule value) + else value + ) defs; base = evalModules { inherit specialArgs;