-
-
Notifications
You must be signed in to change notification settings - Fork 14.2k
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, nixos: fix module aliases in presence of defaults #40418
Conversation
f0d3c98
to
2704f1c
Compare
Rebased. |
Looks OK to me but I happen to avoid using the module system, so I want to at least give a chance to more people to respond. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not mind the changes, but I do mind about the use case.
Using mkRenamedOptionModule
for non-option target will provide misleading documentation (Alias of <option>…</option>
) suggesting that it exists a target option, which would not be documented anywhere.
I think you should fallback to manual handling of such renaming for such cases.
lib/modules.nix
Outdated
}); | ||
config = mkMerge [ | ||
{ | ||
warnings = optional (warn && toOpt.isDefined) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be fromOpt
and not toOpt
. We want to warn uses of the renamed option, not uses of the new option.
lib/modules.nix
Outdated
warnings = optional (warn && toOpt.isDefined) | ||
"The option `${showOption from}' defined in ${showFiles toOpt.files} has been renamed to `${showOption to}'."; | ||
} | ||
(mkIf (fromOpt.isDefined) (setAttrByPath to (mkMerge fromOpt.definitions))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sounds like mkAliasAndWrapDefinitions (setAttrByPath to) fromOpt
?
2704f1c
to
d2b6402
Compare
Thanks for a review!
This should be `fromOpt` and not `toOpt`. We want to warn uses of the renamed option, not uses of the new option.
Oops, fixed.
This sounds like `mkAliasAndWrapDefinitions (setAttrByPath to) fromOpt` ?
Yep.
I do not mind the changes, but I do mind about the use case.
Using `mkRenamedOptionModule` for non-option target will provide misleading documentation (`Alias of <option>…</option>`) suggesting that it exists a target option, which would not be documented anywhere.
The use case is that you had an old option "foo" that now maps to "bar.foo", but "bar" has some other default value. It is uncommon thing to do, but when you have to do this (like for rspamd) current behavior is very confusing (and wrong).
I read "alias of X" as "sets X to a given value when set", which is exactly what it now does, as opposed to "sets X to a given value when set, overrides the default either way", which is what it did before.
I think you should fallback to manual handling of such renaming for such cases.
I feel that would mean that every such use case would have to copy-paste pieces of this `doRename` across nixos (or do something even stranger like #40434 does, for example).
Anyway, let's not merge this yet. I want to test the new fixed version (I'm pretty sure it should work, but better safe than sorry).
|
d2b6402
to
28d2fff
Compare
Before this change `mkRenamedOptionModule` would override option defaults even when the old option name is left unused. For instance ```nix { optios = { services.name.new = mkOption { default = { one = {}; }; }; }; imports = [ (mkRenamedOptionModule [ "services" "name" "old" ] [ "services" "name" "new" "two" ]) ]; config = {}; } ``` would evaluate to `{ config.services.name.new = { two = {}; }; }` when you'd expect it to evaluate to `{ config.services.name.new = { one = {}; }; }`.
28d2fff
to
449d43f
Compare
ping!
|
@nbp ping ping!
|
Do I understand correctly that |
Do I understand correctly that `mkRenamedOptionModule` mentioned in @nbp's objection is simply no longer included?
No. The core of `mkRenamedOptionModule` is `doRename`, so @nbp's philosophic objection to my example in the commit message still stands.
However, it is just a contrived example.
It makes total sense in `rspamd` case where `services.rspamd.bindSocket` became `services.rspamd.workers.normal.bindSockets` and the latter suboption (`normal.bindSockets`) is an option in the submodule of `services.rspamd.workers` with `normal` being introduced in a default value. It makes total sense from the perspective of semantics of `rspamd` declarative configuration.
Meanwhile, `master`'s implementation is clearly buggy because it doesn't do "option aliasing" ("set x when y is set") but something strangely different. In `rspamd`'s case, for instance, it always overrides the thing in the submodule even if the alias is never used.
In any case, all this really changes is
`x // setAttrByPath to (mkAliasDefinitions (getAttrFromPath from options))`
to
`mkMerge [ x (mkAliasAndWrapDefinitions (setAttrByPath to) fromOpt) ]`
and some `let`s and indent.
|
OK, I see. Yes, I agree it is easy to use for a misguided trick, but then again, I think that the entire module system is easy to misuse if that's one's goal… |
Before this change
mkRenamedOptionModule
would override option defaultseven when the old option name is left unused. For instance
would evaluate to
{ config.services.name.new = { two = {}; }; }
when you'd expect it to evaluate to
{ config.services.name.new = { one = {}; }; }
.Really /cc @edolstra and @nbp, since this touches the core.