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

lib/modules: add definedOptionsOnly option to evalModules #333799

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

MattSturgeon
Copy link
Contributor

@MattSturgeon MattSturgeon commented Aug 10, 2024

Description of changes

Adds a new definedOptionsOnly argument to evalModules and types.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:

  • Filtering null values isn't enough when dealing with nested sub-options, we also need to filter empty attrs
  • Filtering empty attrs is problematic if users wish to define an empty lua table
  • Filtering null also isn't ideal, because users may wish to define null to represent lua's nil

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 on config.

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 on isDefined and branch-nodes that are empty after filtering the leaves. Should I create a PR to offer a filterAttrsWith function in the public lib?

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@github-actions github-actions bot added 6.topic: module system About "NixOS" module system internals 6.topic: lib The Nixpkgs function library labels Aug 10, 2024
@MattSturgeon MattSturgeon force-pushed the evalModules-definedOptsOnly-arg branch 2 times, most recently from 8de64a3 to 8b0fab1 Compare August 13, 2024 21:26
TODO: consider promoting custom filter function to `lib.attrsets`?
@MattSturgeon MattSturgeon force-pushed the evalModules-definedOptsOnly-arg branch from 8b0fab1 to eb0c65c Compare August 13, 2024 22:01
@MattSturgeon
Copy link
Contributor Author

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 lib.attrsets, but I'd like feedback before doing either of these.

@MattSturgeon MattSturgeon marked this pull request as ready for review August 13, 2024 22:26
@MattSturgeon MattSturgeon changed the title lib/modules: WIP support for definedOptionsOnly in config lib/modules: add definedOptionsOnly option to evalModules Aug 13, 2024
@roberth
Copy link
Member

roberth commented Aug 14, 2024

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:

  • This would be an alternate solution to config-driven submodules? #158598, without the performance benefit.
  • [exploratory] Module system types.record, types.fix #257511 would be even simpler and faster, because when you're doing this, you're probably modeling a config file, and not doing a bunch of "programming" - two use cases, each with their own solution
    • that PR was too ambitious, trying to recover full submodule capabilities with its fix type; ignore that part; just record should be enough

So basically what I'm getting at is to create a new type for this, which is similar to submodule, but without the fixpoint and function syntax, and designed to do pretty much exactly what you need.
Fast is what you get by getting rid of things, and by avoiding submodule you get to do that, while also keeping submodule itself comparatively simple (well, comparing against current submodule; not against a types.record of course).

You won't lose all "fixpoint" functionality, because these records will still tend to end up in the options of modules (or submodules; same). So you still get to reference them, but it will be settings.foo = config.settings.bar; instead of settings = {config,...}: { foo = config.bar; }, but that pattern is quite rare anyway.

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 apply but before generating the config file, e.g.

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.

@MattSturgeon
Copy link
Contributor Author

MattSturgeon commented Aug 14, 2024

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!

* 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 definedOptionsOnly, right?

Relevant context:

  • This would be an alternate solution to config-driven submodules? #158598, without the performance benefit.
  • [exploratory] Module system types.record, types.fix #257511 would be even simpler and faster, because when you're doing this, you're probably modeling a config file, and not doing a bunch of "programming" - two use cases, each with their own solution
    that PR was too ambitious, trying to recover full submodule capabilities with its fix type; ignore that part; just record should be enough

So basically what I'm getting at is to create a new type for this, which is similar to submodule, but without the fixpoint and function syntax, and designed to do pretty much exactly what you need.

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.
EDIT: on closer inspection, I think it can work.

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.

This reminds me of the alternate solution, which is to filter out the undefined attributes at the last moment, ie not in the option apply but before generating the config file, e.g.

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.

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 foo.bar.baz, we might assume we can filter out the empty foo.bar attrset, however we don't know if it has a freeform definition or not.

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 null values and empty attrs before generating lua code. See the PR description for the downsides of this approach.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: lib The Nixpkgs function library 6.topic: module system About "NixOS" module system internals 10.rebuild-darwin: 0 10.rebuild-linux: 1-10
Projects
None yet
Development

Successfully merging this pull request may close these issues.

config-driven submodules?
2 participants