lib, nixos: fix module aliases in presence of defaults #40418
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
oxij
force-pushed
the
lib/fix-module-aliases
branch
from
f0d3c98
to
2704f1c
Compare
|
Rebased. |
7c6f434c
approved these changes
May 29, 2018
|
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. |
nbp
suggested changes
Jun 4, 2018
There was a problem hiding this comment.
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.
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.
This sounds like mkAliasAndWrapDefinitions (setAttrByPath to) fromOpt ?
oxij
force-pushed
the
lib/fix-module-aliases
branch
from
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).
|
oxij
force-pushed
the
lib/fix-module-aliases
branch
from
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 = {}; }; }`.
oxij
force-pushed
the
lib/fix-module-aliases
branch
from
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… |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Before this change
mkRenamedOptionModulewould 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.