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

configure the evaluation of modules in a modular way #51894

Closed
wants to merge 1 commit into from

Conversation

ottidmes
Copy link

@ottidmes ottidmes commented Dec 12, 2018

Motivation for this change

After realizing that all solutions I could come up with to replace the lib module argument in my NixOS configuration had some shortcoming or another (#51797), I realized that was I really just wanted to be able to do, was to supply arguments to nixos/lib/eval-config.nix based on my configuration (i.e. as part of my configuration). This would allow me to supply my configuration with a custom lib in a pure manner, i.e. no ad hoc extension mechanism through an impure environment variable, and would work with all existing code. Of course this purity cannot be enforced, you could always still call getEnv or similar, but this solution should not reduce purity to my knowledge. If it does, please do tell. This solution also addresses the non-modular arguments to evalModules, because except for the modules attribute to determine the configureModules attribute, everything else can be set through this configureModules attribute. And all the convenience attributes have been kept, but rewritten to their modular equivalent, so they could now be considered syntax sugar when using configureModules, but will still be allowed non-modular when supplied directly to nixos/lib/eval-config.nix for backwards compatibility. I thought it best to only allow the one module to have a configureModules attribute, which ought to be the entry point (e.g. /etc/nixos/configuration.nix), but unless it is safe to assume the first item in the list of modules is this entry point, I have no way to enforce this.

I have tested it with some complex configurations, but the code is not yet ready for a potential merge it its current state, so keep this in mind when giving feedback, but I thought it best to already make a PR so I could already get feedback, so that I know sooner rather than later whether it is a good idea to begin with and what I could do to refine it. Here is an example of it in use:

{
  configureModules = {
    lib = import ../../shared/lib;
  };

  imports = [
    ./main.nix
  ];
}
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • 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 nox --run "nox-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)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@@ -10,26 +10,20 @@ with lib.types;

rec {

/* Evaluate a set of modules. The result is a set of two
evalModules = import ./eval-modules.nix;
Copy link
Author

Choose a reason for hiding this comment

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

This would equate the non-modular attributes allowed with that of nixos/lib/eval-config.nix. It should only allow the non-modular attributes of evalModules that were allowed before.

attributes: ‘options’: the nested set of all option declarations,
and ‘config’: the nested set of all option values.
!!! Please think twice before adding to this argument list! The more
Copy link
Author

Choose a reason for hiding this comment

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

This comment should probably be retained and put above the new evalModules and maybe indicate that they are there only for backwards compatibility, and as such should be considered deprecated.

}:

let extraArgs_ = extraArgs; pkgs_ = pkgs;
extraModules = let e = builtins.getEnv "NIXOS_EXTRA_MODULE_PATH";
Copy link
Author

Choose a reason for hiding this comment

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

Although there is no need anymore for this environment variable, in theory at least, in practice people might still want to use it, so this should probably be kept somehow for backwards compatibility.

evalModule = rec {
_file = toString ./eval-modules.nix;
key = _file;
config = lib.mkMerge [
Copy link
Author

Choose a reason for hiding this comment

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

Or should I have gone with lib.optionalAttrs to add pkgs to _module.args?

then x callArgs'
else callArgs' // x // builtins.listToAttrs (map
(name: { inherit name; value = callArgs'.${name} // x.${name}; })
(builtins.filter (name: x ? ${name}) [ "args" "specialArgs" ]));
Copy link
Author

Choose a reason for hiding this comment

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

If configureModules is just an attrset rather than a function mapping the existing attrset, I would expect the attributes args and specialArgs to be merged with the existing attrset.

(name: { inherit name; value = callArgs'.${name} // x.${name}; })
(builtins.filter (name: x ? ${name}) [ "args" "specialArgs" ]));

configArgs = let len = builtins.length configurers; in if len == 1
Copy link
Author

Choose a reason for hiding this comment

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

I wish I had a better heuristic to determine the entry point module, but all we got is a module list at this point, so this is the best I could come up with.

in lib.evalModules' {
inherit prefix;
modules = modules ++ [ evalModule ];
specialArgs = { modulesPath = toString ../nixos/modules; } // specialArgs // { inherit lib; };
Copy link
Member

Choose a reason for hiding this comment

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

The modulesPath is NixOS-specific. This might be undesirable if others projects (e.g. home-manager) want to use nixpkgs' module system.

@ottidmes
Copy link
Author

ottidmes commented Jun 2, 2019

Although I still like the idea behind this PR, I am no longer in favor of the current implementation, because it makes the configuration less composable, e.g. you can no longer pass your configuration to a function that does something like this:

configuration: import <nixpkgs/nixos> { configuration = [ configuration ./other.nix ]; }

@ottidmes ottidmes closed this Jun 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants