Skip to content

lib/modules: pass specialArgs to modules#121870

Merged
roberth merged 2 commits intoNixOS:masterfrom
Pacman99:pass-specialargs
May 6, 2021
Merged

lib/modules: pass specialArgs to modules#121870
roberth merged 2 commits intoNixOS:masterfrom
Pacman99:pass-specialargs

Conversation

@Pacman99
Copy link
Contributor

@Pacman99 Pacman99 commented May 6, 2021

Motivation for this change

To do evaluations within the module system you also need to know the specialArgs. So here specialArgs itself is passed to nixos modules as a module argument.

The alternative to this change is to get all arguments with args@{ config, ... }:, but that is really hacky. You have to manually remove lots of arguments from args to ensure you don't mess with the module system arguments.

fixes #97255

And should help with mobile-nixos/mobile-nixos#353

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@Pacman99 Pacman99 requested review from infinisil and nbp as code owners May 6, 2021 01:48
@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels May 6, 2021
@Pacman99 Pacman99 changed the title Pass specialargs Pass specialArgs to nixos modules May 6, 2021
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux. labels May 6, 2021
Copy link
Member

@infinisil infinisil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've thought a bit about this, and I'm now convinced that this is the right approach, assuming below suggestion is used.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doing it like this won't include the defaulted modulesPath in the specialArgs argument. Instead I suggest doing this in the evalModules definition instead:

diff --git a/lib/modules.nix b/lib/modules.nix
index 6b3faa447be..3ad1da4dd44 100644
--- a/lib/modules.nix
+++ b/lib/modules.nix
@@ -128,7 +128,7 @@ rec {
         let collected = collectModules
           (specialArgs.modulesPath or "")
           (modules ++ [ internalModule ])
-          ({ inherit lib options config; } // specialArgs);
+          ({ inherit lib options config specialArgs; } // specialArgs);
         in mergeModules prefix (reverseList collected);
 
       options = merged.matchedOptions;

This also has the advantage that no modules need to be evaluated to access specialArgs this way, which works the same for individual specialArgs values.

Copy link
Contributor Author

@Pacman99 Pacman99 May 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My original reasoning was to actually not include modulesPath because eval-config sets a default for modulesPath. But I don't really have a reason for why thats a good thing, so I switched to use your patch.

But with this is there any way to access specialArgs from an evaluated configuration? Right now you can do nixosConfigurations.NixOS._module.args to get all module args. And I would like to be able to access specialArgs that way too. This is useful for re-evaluating modules.

Copy link
Contributor Author

@Pacman99 Pacman99 May 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we do something like this instead?

diff --git a/lib/modules.nix b/lib/modules.nix
index cc38e4cba87..97e3081c4b3 100644
--- a/lib/modules.nix
+++ b/lib/modules.nix
@@ -119,7 +119,7 @@ rec {
         };
 
         config = {
-          _module.args = args;
+          _module.args = args // { inherit specialArgs; };
         };
       };

I have no idea if that could break something. just tested this seems to work ok too

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I switched to that method, let me know if its okay to do it like that

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is a good idea, because it can cause specialArgs to be changed by modules, which then wouldn't match what they are used for. E.g. it would be possible to do _module.args.specialArgs.foo = mkForce "something else", but the overridden value wouldn't apply to the actual module arguments.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want to access specialArgs you could expose it from eval-config.nix instead

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it, yeah I see how that would be inconsistent. I switched back to your method.

For my case I could just expose it inside a module: { specialArgs, ... }: { lib = { inherit specialArgs; }; }. Then specialArgs can be accessed after. But thats a pretty rare use case, so I don't think it needs to be done by default. Its better than what I used to have to do: wrap eval-config to add specialArgs to config.lib.

@Pacman99 Pacman99 force-pushed the pass-specialargs branch from 9a94f1f to 9aa0b33 Compare May 6, 2021 03:27
@Pacman99 Pacman99 requested a review from edolstra as a code owner May 6, 2021 03:27
@Pacman99 Pacman99 changed the title Pass specialArgs to nixos modules lib/modules: pass specialArgs to modules May 6, 2021
@Pacman99 Pacman99 force-pushed the pass-specialargs branch from 9aa0b33 to 86d5680 Compare May 6, 2021 15:59
@Pacman99 Pacman99 force-pushed the pass-specialargs branch from 86d5680 to 87c659a Compare May 6, 2021 23:04
@infinisil infinisil requested a review from roberth May 6, 2021 23:06
Copy link
Member

@infinisil infinisil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this looks good from my side. I'd also like to get @roberth's input though

Copy link
Member

@roberth roberth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks pretty harmless.

It's sad that this is eval-config instead of a submodule. Didn't someone try that? I'll assume it wasn't feasible for legacy reasons.

So this is the pragmatic incremental fix. 👍

@Pacman99
Copy link
Contributor Author

Pacman99 commented May 6, 2021

It's sad that this is eval-config instead of a submodule. Didn't someone try that? I'll assume it wasn't feasible for legacy reasons.

Wouldn't this change still be necessary even if its a submodule? I'm pretty sure you would still have to pass specialArgs to the submodule.

@roberth
Copy link
Member

roberth commented May 6, 2021

still have to pass specialArgs to the submodule.

You're right! I was trying to think of a nicer way to capture what's going on in that code, but indeed you'd need to pass it to submoduleWith, so it's an orthogonal issue.

Wouldn't this change still be necessary even if its a submodule?

No question about it.

@roberth roberth merged commit 0633b6a into NixOS:master May 6, 2021
@Pacman99 Pacman99 deleted the pass-specialargs branch May 7, 2021 00:02
@Pacman99 Pacman99 mentioned this pull request May 10, 2021
@SuperSandro2000
Copy link
Member

FYI: This broke my nixops configuration

nixops deploy -d server --check --include server
error: anonymous function at .../configuration.nix:1:1 called with unexpected argument 'specialArgs'

       at nixpkgs/lib/modules.nix:312:8:

          311|       # works.
          312|     in f (args // extraArgs)
             |        ^
          313|   else
(use '--show-trace' to show detailed location information)
error: evaluation of the deployment specification failed
make: *** [Makefile:34: sodium] Error 1

@infinisil
Copy link
Member

infinisil commented May 11, 2021

@SuperSandro2000 What's your configuration.nix? This error sounds like you're missing an ellipsis (...) in the arguments to configuration.nix

@SuperSandro2000
Copy link
Member

Yeah, I added the three dots back which fixed it

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/cant-upgrade-nixos-unexpected-argument-specialargs/13539/3

@roberth
Copy link
Member

roberth commented Oct 29, 2021

It's sad that this is eval-config instead of a submodule. Didn't someone try that? I'll assume it wasn't feasible for legacy reasons.

It can be done #143207, indeed avoids eval-config.nix. Haven't found a real reason why it shouldn't work. It seems cleaner by relying on the evalModules interface, which is smaller than the eval-config.nix interface.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

specialisation + flakes results in infinite recursion

5 participants