Conversation
See NixOS#299736 for the reasons.
github-actions
bot
added
6.topic: module system
ofborg
bot
added
10.rebuild-darwin: 0
roberth
left a comment
roberth
left a comment
There was a problem hiding this comment.
I want the extra attribute to be useful; not in less than single digit percent of modules.
It seems as if a better solution is perhaps within reach.
| { value = builtins.addErrorContext "while evaluating the option `${showOption loc}':" value; | ||
| # The merged value before applying the options `apply` function to it. | ||
| # In general though, `apply` should not be used, it's an anti-pattern. | ||
| beforeApply = res.mergedValue; |
There was a problem hiding this comment.
This adds the cost of a whole attribute to every option, but delivers almost no value in almost all cases.
Also it doesn't fully solve the problem. What if opt.beforeApply is wrong, and a non-trivial number of modules relies on opt.beforeApply? mkOption { modifyBeforeApply = f; }? opt.beforeModifyApply?
I think at some point we just have to admit that modules aren't fit for certain options or option trees anymore.
What if instead of an option tree or submodule, we could have a types.adapter, which transforms all definitions arbitrarily, transforms the returned config and options arbitrarily, and can be implemented in whichever way the use case demands?
That way we don't have to further bloat the module system with mostly unnecessary attributes, and actually do a better job at being compatible.
There was a problem hiding this comment.
What if
opt.beforeApplyis wrong
Not sure what you mean by that?
and a non-trivial number of modules relies on opt.beforeApply
It only makes sense to rely on beforeApply if the option has an apply, otherwise it's the same as the options value. And apply is being discouraged already, you won't find much new uses. The main use case here really is just allowing certain reasonable configs for older options that already use apply and can't be migrated away from easily.
I agree that it's a high cost to pay for very little value though..
There was a problem hiding this comment.
This is also a good time to mention #299736 as an alternative implementation that doesn't add a new attribute to all options.
There was a problem hiding this comment.
What if
opt.beforeApplyis wrongNot sure what you mean by that?
This time opt.value is wrong. Next time opt.beforeApply is wrong. Why would this sequence stop?
And
applyis being discouraged already, you won't find much new uses.
I'm not convinced of that.
There was a problem hiding this comment.
opt.value is "wrong" this time because apply exists, so we need beforeApply. The sequence stops here because we don't have a "pre-apply" step. After the values get merged, it gets passed to apply directly.
I'm not convinced of that.
At least I'm heavily discouraging apply whenever I see it :)
There was a problem hiding this comment.
Hmm, I might have overreacted.
Bloat argument still stands though, or are we betting on replacing evalModules in the foreseeable future?
I like the idea of having time to do that... :)
| { value = builtins.addErrorContext "while evaluating the option `${showOption loc}':" value; | ||
| # The merged value before applying the options `apply` function to it. | ||
| # In general though, `apply` should not be used, it's an anti-pattern. | ||
| beforeApply = res.mergedValue; |
There was a problem hiding this comment.
Another angle is the current impossibility of using #257511 to simplify submodule. This can't currently be done, in large part because submodules have two values:
- the value also known as
config - the value with
_type = "configuration", aka theevalModulesresult, containingconfig, as well asoptionsand other "meta" or "out of band" things.
What if opt.beforeApply was already thing, submodule returned the _type = "configuration", and by convention, each submodule-typed option had apply = x: x.config or apply = x: removeAttrs ["_module"] x.config?
While that would be a little cumbersome to write, especially for more complicated hierarchies with multiple submodule/attrsOf nestings, it would be more capable, more efficient (fewer removeAttrs calls that copy almost everything), and possible to orthogonalize submodule into individually usable types, such as more or less attempted in #257511.
There was a problem hiding this comment.
Sounds interesting, though I don't think having beforeApply would be blocking that.
Once we deprecate apply entirely, beforeApply will vanish too :)
There was a problem hiding this comment.
Once we deprecate
applyentirely,beforeApplywill vanish too :)
I think apply will stick around until we replace modules themselves. Do we really want to inflict its removal on users?
|
I guess the original use case for this is now resolved using #306790, so I say let's close it unless we find another one :) |
4 participants
See #299736 for the reasons.
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.