Apply `disabledModules` recursively #76857

infinisil · 2020-01-03T03:35:33Z

Motivation for this change

If you have a module foo.nix as follows:

{
  imports = [ ./bar.nix ];
}

And then set

{
  disabledModules = [ ./foo.nix ];
}

The module bar.nix would still be included, even though foo.nix which imports it isn't anymore. This is problematic for e.g. mkRenamedOptionModule's, because none of them would be disabled if the module defining them was disabled, meaning all the renamed options would still apply. See #61570 for more info on this. This fixes it.

To be able to implement this correctly, I had to completely restructure how the module system collects modules. While a simpler implementation would be possible, it then shows undefined behavior when e.g. two modules foo.nix and bar.nix both import qux.nix, but only one of foo.nix/bar.nix gets disabled. In the simpler implementation, qux.nix would only be imported for one of them, whereas with this more correct implementation it will get imported in both cases.

Ping @roberth @danbst @nbp @rycee @Profpatsch @aanderse

Things done

Evaluate how much worse performance is and try to improve it
Write a test for this
Extended documentation with information on this change

roberth · 2020-01-03T10:49:08Z

it then shows undefined behavior

Nix doesn't have undefined behavior afaik. I think what you mean is that the output depends on details in the input that should not matter much.

lib/default.nix

lib/modules.nix

infinisil · 2020-01-04T02:28:11Z

Evaluating nix-instantiate nixos/release-combined.nix -A tested three times I get:

Before this change: 220.0s (stddev 2.0s)
After this change: 226.0s (stddev 1.3s)

So this change makes it about 2.7% slower, not too bad. For more stats, see ofborg's result here: https://github.com/NixOS/nixpkgs/pull/76857/checks?check_run_id=373281930

infinisil · 2020-01-04T02:31:54Z

I'll try to optimize it a bit further though, I think there's some potential

infinisil · 2020-01-04T03:27:18Z

Nope, the potential I saw turned out to be an anti-optimization. Apart from perhaps adding some more comments to the code itself, I think this is pretty much ready.

roberth · 2020-01-05T10:46:35Z

lib/modules.nix

+          # it out to a direct `key -> module` mapping while also filtering disabled
+          # keys
+          flattenKeys = keys:
+            let included = filterAttrs (key: value: ! elem key disabledKeys) keys;


Did you try builtins.removeAttrs? Unlike filterAttrs, it's a primop.

Ah nice, didn't even occur to me

infinisil · 2020-01-05T17:09:57Z

With that change I get:

Before: 205.2s (stddev 0.7s, 3 samples)
After: 211.2s (stddev 3.1s, 6 samples)

Not sure why this time the times are a bit faster than last time, the raw values are 210.574, 212.35, 205.356, 210.043, 214.301, 214.593. Not sure what's up with that 205.356 there either. With those means we get a decrease of 2.9%, so it's about the same. But it certainly can't hurt to use removeAttrs instead, will apply that change.

lib/modules.nix

infinisil · 2020-01-07T03:38:56Z

Moved loadModule to the let in scope, no need to expose this in lib

infinisil · 2020-01-09T02:45:10Z

Using the faster with genericClosure now

roberth · 2020-01-09T09:13:08Z

lib/modules.nix

+        let
+          moduleKey = m: if isString m then toString modulesPath + "/" + m else toString m;
+          disabledKeys = map moduleKey disabled;
+          keyFilter = filter (attrs: ! elem attrs.key disabledKeys);


This will become slow when more than very few modules are disabled. Can we change this to disabledKeys ? attrs.key by making disabledKeys an attrset?

I believe it shouldn't get slower, because disabledKeys ? ${attrs.key} seems to be linear in disabledKeys.. See https://github.com/NixOS/nix/blob/04bbfa692f4ac506a74ae372eca486bd9b56de77/src/libexpr/attr-set.hh#L67-L73

So the elem (also a primop) essentially implements ? but for a list.

std::lower_bound is in fact a binary search. See the docs and impl references on https://en.cppreference.com/w/cpp/algorithm/lower_bound

infinisil · 2020-01-09T15:40:21Z

lib/modules.nix

+        let
+          moduleKey = m: if isString m then toString modulesPath + "/" + m else toString m;
+          disabledKeys = listToAttrs (map (k: nameValuePair (moduleKey k) null) disabled);
+          keyFilter = filter (attrs: ! disabledKeys ? ${attrs.key});


@roberth Now doing this. Was able to confirm slight memory decrease with this approach for ~25 disabled modules

roberth · 2020-01-09T16:16:40Z

lib/modules.nix

+      loadModule = args: fallbackFile: fallbackKey: m:
+        if isFunction m || isAttrs m then
+          unifyModuleSyntax fallbackFile fallbackKey (applyIfFunction fallbackKey m args)
+        else loadModule args (toString m) (toString m) (import m);


This allows a module file to consist of a path to be imported. That doesn't seem very useful, yet introduces infinite recursion as a failure mode here. Calling unifyModuleSyntax directly doesn't seem so bad.

Yeah, doesn't seem too bad with a bit of duplication. Did that

With this change, disabledModules applies recursively, meaning if you have a module "foo.nix" with imports = [ ./bar.nix ]; then setting disabledModules = [ "foo.nix" ]; will disable both "foo.nix" and "bar.nix", whereas previously only "foo.nix" would be disabled. This change along with NixOS#61570 allows modules to be fully disabled even when they have some `mkRenamedOption` imports.

Previously disabledModules would not be disabled for the manual

infinisil · 2020-01-09T17:20:21Z

Thanks for the review :D

utdemir · 2020-01-10T01:16:28Z

After this change, I am unable to use store paths in my module imports. For example, something like:

{
  imports = [ "${sources.home-manager}/nixos" ];
  ...
}

now fails to evaluate with:

error: the string '/nix/store/an1yy60yq7p6syfxwbwljb8dmxnkk1ls-source/nixos' is not allowed to refer to a store path (such as '/nix/store/an1yy60yq7p6syfxwbwljb8dmxnkk1ls-source')

Is this intentional? If so, is there a workaround?

infinisil · 2020-01-10T02:59:21Z

Oh thanks for the report. Turns out the changes in #76857 (comment) are causing this. I'll make a PR to fix it

This fixes imports from the store not being possible, which was caused by NixOS#76857 E.g. such a case: imports = [ "${home-manager}/nixos" ];

infinisil · 2020-01-10T03:17:04Z

See above PR which fixes this

infinisil · 2020-01-16T19:29:13Z

As uncovered by @evanjs on IRC, this introduces a new case of infinite recursion when imports have a cycle in them, e.g. a module foo.nix with the contents

{
  imports = [ ./foo.nix ];
}

now throws

error: stack overflow (possible infinite recursion)

While I do have a fix for this, I don't think this use-case needs to be supported as I don't know of a case where something like this is needed. To fix this, just break the cycle by not importing the same module again.

So you can say that imports now have to be a DAG instead of an arbitrary graph.

Fix is here in case this is needed in the future:

diff --git a/lib/modules.nix b/lib/modules.nix
index e2315290ff0..82981dba73b 100644
--- a/lib/modules.nix
+++ b/lib/modules.nix
@@ -139,16 +139,22 @@ rec {
             disabled = concatLists (catAttrs "disabled" modules);
             inherit modules;
           };
-        in parentFile: parentKey: initialModules: args: collectResults (imap1 (n: x:
+        in parentPath: parentFile: parentKey: initialModules: args:
           let
-            module = loadModule args parentFile "${parentKey}:anon-${toString n}" x;
-            collectedImports = collectStructuredModules module._file module.key module.imports args;
+            # All modules this module imports, loaded and without modules that
+            # are anywhere on the parent path to prevent cycles
+            includedModules = filter (module: ! elem module.key parentPath) (imap1 (n:
+              loadModule args parentFile "${parentKey}:anon-${toString n}"
+            ) initialModules);
+          in collectResults (map (module:
+          let
+            collectedImports = collectStructuredModules (parentPath ++ [ module.key ]) module._file module.key module.imports args;
           in {
             key = module.key;
             module = module;
             modules = collectedImports.modules;
             disabled = module.disabledModules ++ collectedImports.disabled;
-          }) initialModules);
+          }) includedModules);
 
       # filterModules :: String -> { disabled, modules } -> [ Module ]
       #
@@ -165,7 +171,7 @@ rec {
         });
 
     in modulesPath: initialModules: args:
-      filterModules modulesPath (collectStructuredModules unknownModule "" initialModules args);
+      filterModules modulesPath (collectStructuredModules [""] unknownModule "" initialModules args);
 
   /* Massage a module into canonical form, that is, a set consisting
      of ‘options’, ‘config’ and ‘imports’ attributes. */

evanjs · 2020-01-16T23:28:51Z

Just wanted to confirm that a dry build without @infinisil's patch does reproduce this issue (error: stack overflow (possible infinite recursion)), and with the patch, I am able to get a list of packages that would be built!

This fixes imports from the store not being possible, which was caused by NixOS#76857 E.g. such a case: imports = [ "${home-manager}/nixos" ]; (cherry picked from commit e0ea5f4)

Now possible with NixOS/nixpkgs#61570 and NixOS/nixpkgs#76857

infinisil requested review from edolstra and nbp as code owners January 3, 2020 03:35

ofborg bot added 6.topic: nixos 8.has: documentation 8.has: module (update) 10.rebuild-darwin: 0 10.rebuild-linux: 0 labels Jan 3, 2020

roberth reviewed Jan 3, 2020

View reviewed changes

lib/default.nix Outdated Show resolved Hide resolved

lib/modules.nix Outdated Show resolved Hide resolved

infinisil force-pushed the recursive-disableModules branch from 4e47c4b to 47b9fb0 Compare January 3, 2020 19:21

infinisil mentioned this pull request Jan 3, 2020

Move renames to their appropriate places #61570

Merged

10 tasks

roberth reviewed Jan 5, 2020

View reviewed changes

infinisil force-pushed the recursive-disableModules branch 2 times, most recently from 36542bd to fda9bf8 Compare January 5, 2020 17:11

roberth reviewed Jan 5, 2020

View reviewed changes

lib/modules.nix Show resolved Hide resolved

infinisil force-pushed the recursive-disableModules branch from fda9bf8 to 5d1cccf Compare January 7, 2020 03:38

infinisil force-pushed the recursive-disableModules branch from 5d1cccf to 8db41d4 Compare January 8, 2020 22:41

roberth reviewed Jan 9, 2020

View reviewed changes

infinisil force-pushed the recursive-disableModules branch from 8db41d4 to 41e44f6 Compare January 9, 2020 15:38

infinisil commented Jan 9, 2020

View reviewed changes

roberth reviewed Jan 9, 2020

View reviewed changes

infinisil added 3 commits January 9, 2020 17:26

nixos/documentation: Fix disabledModules being rendered

7167985

Previously disabledModules would not be disabled for the manual

lib/tests: Add tests for recursive disabledModules

b89b23b

nixos/doc: Extend disabledModules docs to indicate recursive disabling

a6462a4

infinisil force-pushed the recursive-disableModules branch from 41e44f6 to a6462a4 Compare January 9, 2020 16:26

roberth approved these changes Jan 9, 2020

View reviewed changes

infinisil merged commit e9c16ec into NixOS:master Jan 9, 2020

infinisil deleted the recursive-disableModules branch January 9, 2020 17:20

infinisil added a commit to infinisil/nixpkgs that referenced this pull request Jan 10, 2020

lib/modules: Fix store imports

e0ea5f4

This fixes imports from the store not being possible, which was caused by NixOS#76857 E.g. such a case: imports = [ "${home-manager}/nixos" ];

infinisil mentioned this pull request Jan 10, 2020

Fix store imports from NixOS modules #77416

Merged

2 tasks

infinisil mentioned this pull request Jan 25, 2020

dnscrypt-proxy2 service: init #72815

Closed

10 tasks

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

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

disable nixpkgs' murmur module, replacing it with my own

19a252a

Now possible with NixOS/nixpkgs#61570 and NixOS/nixpkgs#76857

infinisil mentioned this pull request May 4, 2020

Replacing modules #22764

Merged

7 tasks

infinisil mentioned this pull request Sep 1, 2023

nixos/modules: show warning and continue on self import #252806

Open

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Apply `disabledModules` recursively #76857

Apply `disabledModules` recursively #76857

infinisil commented Jan 3, 2020 •

edited

roberth commented Jan 3, 2020

infinisil commented Jan 4, 2020

infinisil commented Jan 4, 2020

infinisil commented Jan 4, 2020

roberth Jan 5, 2020

infinisil Jan 5, 2020

infinisil commented Jan 5, 2020 •

edited

infinisil commented Jan 7, 2020

infinisil commented Jan 9, 2020

roberth Jan 9, 2020

infinisil Jan 9, 2020 •

edited

roberth Jan 9, 2020

infinisil Jan 9, 2020

roberth Jan 9, 2020

infinisil Jan 9, 2020

infinisil commented Jan 9, 2020

utdemir commented Jan 10, 2020

infinisil commented Jan 10, 2020

infinisil commented Jan 10, 2020

infinisil commented Jan 16, 2020

evanjs commented Jan 16, 2020

Apply disabledModules recursively #76857

Apply disabledModules recursively #76857

Conversation

infinisil commented Jan 3, 2020 • edited

Motivation for this change

Things done

roberth commented Jan 3, 2020

infinisil commented Jan 4, 2020

infinisil commented Jan 4, 2020

infinisil commented Jan 4, 2020

roberth Jan 5, 2020

Choose a reason for hiding this comment

infinisil Jan 5, 2020

Choose a reason for hiding this comment

infinisil commented Jan 5, 2020 • edited

infinisil commented Jan 7, 2020

infinisil commented Jan 9, 2020

roberth Jan 9, 2020

Choose a reason for hiding this comment

infinisil Jan 9, 2020 • edited

Choose a reason for hiding this comment

roberth Jan 9, 2020

Choose a reason for hiding this comment

infinisil Jan 9, 2020

Choose a reason for hiding this comment

roberth Jan 9, 2020

Choose a reason for hiding this comment

infinisil Jan 9, 2020

Choose a reason for hiding this comment

infinisil commented Jan 9, 2020

utdemir commented Jan 10, 2020

infinisil commented Jan 10, 2020

infinisil commented Jan 10, 2020

infinisil commented Jan 16, 2020

evanjs commented Jan 16, 2020

Apply `disabledModules` recursively #76857

Apply `disabledModules` recursively #76857

infinisil commented Jan 3, 2020 •

edited

infinisil commented Jan 5, 2020 •

edited

infinisil Jan 9, 2020 •

edited