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

Allow submodules to use custom lib/evalModules #98952

Closed
wants to merge 3 commits into from

Conversation

infinisil
Copy link
Member

Motivation for this change

This allows submodules to use a custom evalModules function, which is useful when the modules aren't compatible with the default version, especially for the lib module argument.

Note that previously it was possible to pass a custom lib argument using specialArgs.lib. However that still evaluated the modules using the standard lib.evalModules, which causes problems when e.g. specialArgs.lib.mkOption uses a newer nixpkgs version that adds another mkOption attribute (like #97114) but the standard lib.evalModules can't handle that attribute yet.

This allows fixing of infinisil/nixus#34
This is an extension of #75031

Ping @roberth @bqv

Things done
  • Added tests for all relevant features
  • Added docs

The module system already uses the parent modules _type as a fallback,
so we don't need to inject the file in a weird way

With a future commit that allows passing custom lib versions, this will
also allow usage of older nixpkgs versions that don't have
aa61342 yet
This allows evaluating modules with older lib versions
@infinisil infinisil added the 6.topic: module system About "NixOS" module system internals label Sep 27, 2020
@bqv
Copy link
Contributor

bqv commented Sep 27, 2020

Lgtm!

@ofborg ofborg bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: documentation This PR adds or changes documentation 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 10.rebuild-linux: 1 labels Sep 27, 2020
@ajs124
Copy link
Member

ajs124 commented Sep 27, 2020

I think this might be useful to us for our docs generation stuff @dasJ

@roberth
Copy link
Member

roberth commented Sep 27, 2020

This invites a new type of complexity when troubleshooting module system issues. We'll have to consider the possibility that other versions of the module system are integrated into e.g. the options list. Not only consider it; clearly it will happen as evidenced by the fact that this is not a two-line diff.
I'd rather have a solution where we use dependency injection to have a single version of the module system in both applications of the module system. That way you don't introduce any new surface area for incompatibilities.
The module system is already doing a fairly good job with backward compatibility on its actual interface, so we can keep leveraging that when it comes to the issue of integrating module applications.
I know it's asking a little bit more from users, allowing lib to be passed as an argument, but that's a small price to pay to keep the module system stable and its compatibility manageable.

So unless there's a fundamental reason why passing lib can't work, it's a no.

Note that it's still possible to just invoke whichever evalModules without types.submodule and use the resulting config attribute as desired, without causing any unexpected breakage due to module system internals changing.

@infinisil
Copy link
Member Author

Hmm I see what you mean. I think we can organize the problem like this:

  1. Reliance on evalModules input API, which is:
    a. Trivial arguments specialArgs, args and prefix
    b. The modules argument, which needs to contain compatible modules
  2. Reliance on evalModule's output API, which is:
    a. .config exists
    b. .options is an nested attribute set of options, suitable for doc generation with lib.optionAttrSetToDocList, which relies on some option properties like .loc, .description, etc.

My opinions:

  • 1.a This is probably fine, those args won't be removed
  • 1.b Could be problematic with e.g. { lib }: { some.submodule.value = lib.mkDefault true; }, since the submodule receives a mkDefault constructed with the lib from the parent module evaluation.
  • 2.a Not problematic. The value can be arbitrary, there's no API about it
  • 2.b This could be very problematic. We could "fix" this by just not exposing any sub options if evalModules is set
  • In addition, future versions of submoduleWith might want to rely on more of evalModule's API

So in conclusion: it's not ideal.

@infinisil
Copy link
Member Author

infinisil commented Sep 28, 2020

However, 1.b and 2.b are actually already a problem, because setting specialArgs.lib can cause them.

Here's an idea though:

  • Fix 1.b by only allowing paths to be passed as submodules that have an evalModules set. Because paths can't refer to variables outside. So the problem about using a lib from outside can't exist. The only problem with this is that it will be very inconvenient, disallowing many non-problematic cases as well.
  • Fix 2.b by not exposing any options for submodules that have an evalModules set. This actually sounds kind of reasonable, as the parent module eval won't necessarily be able to process the options returned by the submodule. However the submodule can still expose a rendered documentation as a declared option.

@infinisil
Copy link
Member Author

Thinking about this again, I had the idea of just using otherLib.types.submodule as the type of the option, which one might expect to automatically use evalModules and co. from otherLib. However there's some gotchas (in addition to the above ones):

  • It only works reliably if you make sure to use this type for all declarations of the same option. This is because type merging combines the types in an arbitrary order, and the first one chosen is the one that determines where the type merging function comes from.
  • Some submodule type dependencies shouldn't come from the otherLib, since they're still part of the original module eval. Specifically this includes mkOptionType, path.check

So this won't work well

@infinisil
Copy link
Member Author

It would be possible to implement the idea from #98952 (comment), which would result in a safe use of this kind of feature. However the fix for 1.b is very restricting.

As an alternative, I think this should rather be fixed by ensuring that all functions which interact with the module evaluation (like mkOption, mkDefault, but also all types) are just implemented by propagating their arguments. This is already the case for mkDefault, which just turns into { _type = "override"; priority = 1000; content = ...; }. This is also almost the case with mkOption. The bigger problem are the types. There would have to be changed to e.g.

{
  attrsOf = elemType: {
    _type = "option-type";
    name = "attrsOf";
    argument = elemType;
  };
}

While the module evaluation side would then actually take care of implementing this type. We could also have a _type = "option-type-api", which would expose a stable interface for custom third-party type definitions.

With these changes we'll be able to write modules like

{ lib, ... }: {
  # A nested submodule that uses a different `lib` for its evaluation
  options.some.submodule = lib.mkOption {
    # Even though we use the toplevel modules `lib` value, the module evaluation can decide how `attrsOf` is implemented for itself.
    type = lib.types.attrsOf lib.types.int;
  };

  # Meanwhile options in the toplevel module evaluation work as well with the same `lib`
  options.normal = lib.mkOption {
    type = lib.types.attrsOf lib.types.int;
  };
}

This is a rather big undertaking, but I think it's a step in the right direction, and not just for the motivation I gave in this PR. This goes toward having a stable module system API, which we currently don't really have. Especially the types are rather interlinked with the module system which would be nice to change.

@infinisil
Copy link
Member Author

So I think this is a problem for another day. I'll close this PR, but will open a new one with just the first 2 commits, which are still worthwhile to add (it's just a refactoring and a test).

@infinisil infinisil closed this Jan 9, 2021
@infinisil infinisil deleted the submodule-improvements branch January 9, 2021 06:54
@infinisil infinisil mentioned this pull request Jan 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: module system About "NixOS" module system internals 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: documentation This PR adds or changes documentation 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 10.rebuild-linux: 1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants