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

Apply `disabledModules` recursively #76857

Merged
merged 4 commits into from Jan 9, 2020

Conversation

@Infinisil
Copy link
Member

Infinisil commented Jan 3, 2020

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
Copy link
Contributor

roberth commented Jan 3, 2020

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 Outdated Show resolved Hide resolved
lib/modules.nix Outdated Show resolved Hide resolved
@Infinisil Infinisil force-pushed the Infinisil:recursive-disableModules branch from 4e47c4b to 47b9fb0 Jan 3, 2020
@Infinisil Infinisil mentioned this pull request Jan 3, 2020
0 of 10 tasks complete
@Infinisil
Copy link
Member Author

Infinisil commented Jan 4, 2020

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

Infinisil commented Jan 4, 2020

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

@Infinisil
Copy link
Member Author

Infinisil commented Jan 4, 2020

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.

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

This comment has been minimized.

Copy link
@roberth

roberth Jan 5, 2020

Contributor

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

This comment has been minimized.

Copy link
@Infinisil

Infinisil Jan 5, 2020

Author Member

Ah nice, didn't even occur to me

@Infinisil
Copy link
Member Author

Infinisil commented Jan 5, 2020

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.

@Infinisil Infinisil force-pushed the Infinisil:recursive-disableModules branch 2 times, most recently from 36542bd to fda9bf8 Jan 5, 2020
lib/modules.nix Show resolved Hide resolved
@Infinisil Infinisil force-pushed the Infinisil:recursive-disableModules branch from fda9bf8 to 5d1cccf Jan 7, 2020
@Infinisil
Copy link
Member Author

Infinisil commented Jan 7, 2020

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

@Infinisil Infinisil force-pushed the Infinisil:recursive-disableModules branch from 5d1cccf to 8db41d4 Jan 8, 2020
@Infinisil
Copy link
Member Author

Infinisil commented Jan 9, 2020

Using the faster with genericClosure now

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 comment has been minimized.

Copy link
@roberth

roberth Jan 9, 2020

Contributor

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?

This comment has been minimized.

Copy link
@Infinisil

Infinisil Jan 9, 2020

Author Member

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.

This comment has been minimized.

Copy link
@roberth

roberth Jan 9, 2020

Contributor

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 Infinisil force-pushed the Infinisil:recursive-disableModules branch from 8db41d4 to 41e44f6 Jan 9, 2020
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});

This comment has been minimized.

Copy link
@Infinisil

Infinisil Jan 9, 2020

Author Member

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

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 comment has been minimized.

Copy link
@roberth

roberth Jan 9, 2020

Contributor

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.

This comment has been minimized.

Copy link
@Infinisil

Infinisil Jan 9, 2020

Author Member

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

Infinisil added 3 commits Jan 2, 2020
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 #61570 allows
modules to be fully disabled even when they have some `mkRenamedOption`
imports.
Previously disabledModules would not be disabled for the manual
@Infinisil Infinisil force-pushed the Infinisil:recursive-disableModules branch from 41e44f6 to a6462a4 Jan 9, 2020
@roberth
roberth approved these changes Jan 9, 2020
@Infinisil Infinisil merged commit e9c16ec into NixOS:master Jan 9, 2020
12 checks passed
12 checks passed
Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A darwin-tested
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release-combined.nix -A tested
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A unstable
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
@Infinisil
Copy link
Member Author

Infinisil commented Jan 9, 2020

Thanks for the review :D

@Infinisil Infinisil deleted the Infinisil:recursive-disableModules branch Jan 9, 2020
@utdemir
Copy link
Member

utdemir commented Jan 10, 2020

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

Infinisil commented Jan 10, 2020

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

Infinisil added a commit to Infinisil/nixpkgs that referenced this pull request Jan 10, 2020
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 Infinisil mentioned this pull request Jan 10, 2020
2 of 2 tasks complete
@Infinisil
Copy link
Member Author

Infinisil commented Jan 10, 2020

See above PR which fixes this

eadwu added a commit to eadwu/nixpkgs that referenced this pull request Jan 10, 2020
…leModules"

This reverts commit e9c16ec, reversing
changes made to 4052aa3.
@Infinisil
Copy link
Member Author

Infinisil commented Jan 16, 2020

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

evanjs commented Jan 16, 2020

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!

dtzWill added a commit to dtzWill/nixpkgs that referenced this pull request Jan 19, 2020
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)
@Infinisil Infinisil mentioned this pull request Jan 25, 2020
1 of 10 tasks complete
Infinisil added a commit to Infinisil/system that referenced this pull request Mar 28, 2020
@Infinisil Infinisil mentioned this pull request May 4, 2020
3 of 7 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.