lib: add option parameter for default value priority #296979
Conversation
github-actions
bot
added
6.topic: module system
ofborg
bot
added
10.rebuild-darwin: 1-10
10.rebuild-darwin: 1
10.rebuild-linux: 1-10
labels
|
Friendly ping. |
There was a problem hiding this comment.
I've put my thoughts on the design in a comment.
I'll be interested to hear what @infinisil thinks about the design.
When that's agreed, we'll need:
- tests in
lib/tests/modules.sh - documentation for the new
mkOptionarguments in the manual - update the docs generation to show the new info
nixos/lib/make-options-doc/default.nixpkgs/tools/nix/nixos-render-docs/
|
Thank you all for the feedback, I still intend to continue this but it may take a bit. |
|
Another thing we may want to think about while fixing this issue is cases where you want to override the default value (i.e. remove an item from a list) but still allow other modules to merge in their values too. |
Atemu
force-pushed
the
options-additive
branch
from
2a6744e to
4d596d8
Compare
|
I've changed the setting to take a priority value instead of a boolean and exposed the priority values in a sane manner. Will take a look at docs and tests next. |
ofborg
bot
added
10.rebuild-darwin: 0
Atemu
force-pushed
the
options-additive
branch
from
4d596d8 to
fdd007f
Compare
github-actions
bot
added
6.topic: nixos
Atemu
changed the title
|
I've added documentation but I don't know how to touch nixos-render-docs. I think I'd prefer to not add it for now. |
ofborg
bot
added
10.rebuild-darwin: 1-10
10.rebuild-darwin: 1
and removed
10.rebuild-darwin: 0
There was a problem hiding this comment.
Looks pretty good from a computational perspective, but this does create a problem with the meaning of the word "default" now, ambiguously referring to
- certain priorities
- a preset value having some declared priority that's not necessarily a "default" priority
We can handle a bit of ambiguity, but the possibility of having "a default that isn't a default" does kind of make it cross out of the grey area.
Ideally we'd rename mkOption { default, defaultText } to something like mkOption { preset, presetText } (and presetPriority). That'd be a huge migration, and canonicalizing the option declarations will have a measurable performance cost during such a migration.
Nonetheless, I do believe most modules are yet to be written...
We've managed the markdown migration pretty well, and this is a simpler syntax change with fewer steps (nothing analogous to remove mdDoc).
It's a heavy decision, but one or two such choices make the difference between devolving into a weird legacy system or something that's actually nice to use.
I think the migration path would be
- now
- add
presetandpresetTextas alternatives - add
presetPriority - set
defaultanddefaultTextinevalOptionValuefor compatibility with code that reads those values
- after a release cycle
- warn when any
default*attributes is set, inmkOption - warn when
default*is used, inevalOptionValue
- after another release cycle
- remove
default*.
@Atemu deep apologies for the scope increase.
@infinisil am I going insane?
|
I can see where you're coming from but I think I still prefer I think it's perfectly expectable for configuration system's default values to remain in effect when an option is set, so I don't think this violates the principle of least surprise. I'd even argue it's more of a quirk of the NixOS module system that this wasn't possible before. We're used to only overrides of course but I don't think an outsider would consider it odd if both overriding and merging a default value was possible. I don't see a dependency between PR and potentially renaming I'd prefer to keep the name for |
github-actions
bot
added
8.has: module (update)
|
Another point that just came to mind is that most of what this is useful for is already possible without this PR (as described in the commit message), it just isn't as nice to implement. It's actually already in use in a few places. |
wegank
added
the
2.status: merge conflict
I needed to expose the baseline priority (and ideally the other priorities too) for external use. While I was at it, I gave mkDefault and defaultOverridePriority (not the same thing in any way, confused yet?) better names.
When you define an option's default value, the expectation is that whenever the
option is set somewhere, the default value will be overridden by that setting
rather than merged.
It's sometimes the case however that your option's default value should be
merged with what is being set rather than overwritten by it; only allowing
explicit overrides via `mkForce`. Creating such an option was not possible
previously as all options' default values were forcefully clamped via
mkOptionDefault.
There were some workarounds such as creating additional options ("things" +
"extraThings") which you'd then have to explain to users and generally aren't
ergonomical to use.
Another workaround was to explicitly set options value to the default value in
the implementation but those two lines of code might be many lines apart from
another and it's generally an odd pattern. Example:
{
options.foo = lib.mkOption {
default = [ "foo" ];
};
# Many more lines of code
config.foo = options.foo.default;
}
It is now possible to control this behaviour at option declaration which is much
clearer in the code and is introspect-able.
{
options.foo = lib.mkOption {
default = [ "foo" ];
defaultPriority = lib.modules.priorities.baseline;
};
# Many more lines of code
}
The implementation is trivial.
If this becomes used more widely, it should be rendered in the generated options
manual and nixos-search.
It's a bit unfortunate to have to change things again but hopefully this iteration will last for longer.
I don't expect this to be used much but it should exist for completeness' sake.
This makes the wording clearer with more examples and usage of the new names for functions and priority values.
Atemu
force-pushed
the
options-additive
branch
from
d5d0fbd to
75ca593
Compare
`v.default` always fails because v is the (boolean) attrvalue of the default settings. `lib.mkOptionDefault v` would work fine. But let's just plainly set the default option value. This way, we can later convert the `settings` option to use @Atemu's `defaultPriority` (NixOS#296979) without forcing users to update their config.
`v.default` always fails because v is the (boolean) attrvalue of the default settings. `lib.mkOptionDefault v` would work fine. But let's just plainly set the default option value. This way, we can later convert the `settings` option to use @Atemu's `defaultPriority` (NixOS#296979) without forcing users to update their config.
It's sometimes the case that your option's default value should be additive such that setting it results in the default value being merged with whatever was set.
Previously, you had to add the default value in the implementation which might be many lines away from the option declaration and is generally an odd pattern.
This is a lot clearer, simpler and doesn't add much complexity.
If this becomes used more widely, it should be rendered in the generated options' manual and nixos-search.
Description of changes
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-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.