-
-
Notifications
You must be signed in to change notification settings - Fork 13.9k
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
lib/modules: add definedOptionsOnly
option to evalModules
#333799
base: master
Are you sure you want to change the base?
lib/modules: add definedOptionsOnly
option to evalModules
#333799
Conversation
8de64a3
to
8b0fab1
Compare
TODO: consider promoting custom filter function to `lib.attrsets`?
8b0fab1
to
eb0c65c
Compare
There's still things to address* but I'm happy to undraft this now. * e.g. additional test cases and moving the filtering function to |
definedOptionsOnly
in config
definedOptionsOnly
option to evalModules
This definitely addresses a need, but also spends ~1.5% performance overhead* we'll never get back. I think we could get a speedup instead! Relevant context:
So basically what I'm getting at is to create a new type for this, which is similar to You won't lose all "fixpoint" functionality, because these That said, any time you have a conditional attribute, the condition will have to be computable without accessing the attrset it contributes to, which is something you never have to worry about with non-freeformType modules. This reminds me of the alternate solution, which is to filter out the undefined attributes at the last moment, ie not in the option format.generate
"foo"
{}
(filterAttrs
(name: _: opt.settings.${name}.isDefined or true)
opt.settings.value) Probably Nixpkgs has a good recursive example somewhere that supports "option trees", ie options declared in a nested attrset. * Estimating performance with OfBorg is dark magic, but I've already sold my soul so it's fine. When you open the Evaluation Performance Report commit status Details, you'll find the report, including a cpuTime measure. That one's noise. Instead, allocations are a decent proxy for performance, and far less noisy. In my experience, the module system system has about 5× the effect on a NixOS evaluation whereas OfBorg spends a lot of time in packages, hence 0.3% * 5 = 1.5%. Not huge by itself, but still something to try avoid, as more features will come along. |
Thanks for the feedback and performance insight! Just so we're on the same page, due to lazy evaluation this performance hit is "opt-in", i.e. it only affects users who enable
Thanks for linking those, I had looked at them before opening this but I forgot to add them to the PR description! (Now updated). The record type is interesting, but I don't think it fits my needs of not including nested undefined options, since IIUC it is a fairly simple (and flat) type. As for #158598 and #158594, this PR is definitely a solution to #158598, however I feel the approach suggested in #158594 is a little more complex and "long-term" feeling. While my implementation here is fairly simple and small in scope, the proposal (especially in this comment) could be fairly wide-reaching and end up being a fairly large refactor. I'll read over your gist here a few times after I've posted this reply, as I've not yet fully absorbed it. I like the concept hinted at in the subheading "minimod: A stripped down module system". Again, my first impressions are it'd be a potentially better, but far more involved solution than the one proposed here.
That's a clever approach, and I've contemplated taking an approach like this in the past. The big roadblock for me here is the "ancestor" attrs or undefined options potentially conflicting with empty freeform definitions. E.g. if we have a freeform option with an undefined suboption This is essentially a slightly better implementation of what we're already doing in nixvim, which is have all sub-options default to null and then filter out |
Description of changes
Adds a new
definedOptionsOnly
argument toevalModules
andtypes.submoduleWith
, defaulting to false. When set to true undefined options are filtered from the final merged config instead of being present as stubs that throw "used but not defined" when evaluated.Fixes #158598
See also: #158594, #257511
Motivation
In Nixvim, we have a number of freeform (rfc42-style)
settings
options. Most of these have a number of sub-options declared, however we only want these to show up in the generated lua tables when actually defined.To clarify, nixvim has a philosophy of only generating lua config when explicitly enabled via its nix options. Up until now, we've been using sub-options that default to
null
and filtering out null values, however this has some issues:null
to represent lua'snil
Instead, I would like for nixvim to use sub-options without a default, but only include them in the resulting config when defined.
This was originally discussed in this matrix thread.
I'm sure this would also be useful for some nixos and home-manager submodules.
Can it be a
_module
option?Currently, I expose the configured value via a read-only option
_module.definedOptionsOnly
, however due to infinite recursion we cannot use this as a read-write option that actually decides which behaviour to use.Having this as an
evalModules
arg is kinda ugly, but I can't think of a way we could define it as a module option without running into infinite recursion.More specifically, because
config
changes its actual shape (which attrs are present) based on whether we're filtering out undefined options, we cannot use it recursively to evaluate options from modules that depend onconfig
.Custom attr filtering
Additionally, I had to write a custom filtering function, as
filterAttrsRecursive
wasn't powerful enough to filter out option leaf-nodes based onisDefined
and branch-nodes that are empty after filtering the leaves. Should I create a PR to offer afilterAttrsWith
function in the public lib?Things done
nix.conf
? (See Nix manual)sandbox = relaxed
sandbox = true
nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)Add a 👍 reaction to pull requests you find important.