lib/modules: add mkUnset, mkOverrideUnset #63553
Conversation
|
Can't say anything about the implementation, but the API seems good to me :) Very much like to have this feature as well! |
There was a problem hiding this comment.
@rhendric kool!
A few thoughts:
- this works awkward with default values - if option has some default, mkUnset will remove it (but restoring default value isn't that hard, as we can query
options) - attrs inside lists are impossible to unset. (those are also impossible to
mkForce, so no big deal)
But this definitely has it's uses.
|
Do you have some concrete examples of where this would have been really useful? In general I think attrsets having an One thing I'd like to see for this is a test in |
|
@infinisil this is mostly useful for |
|
@infinisil, there's also things like
Ah, that's where they live! Yes, I'll gladly do that. |
There was a problem hiding this comment.
I think this is a very nice idea and as far as I can tell the implementation is fine. I would like to see a few tests under lib/tests/modules, though. Both to help verify that the implementation behaves as expected and to avoid future regressions.
Edit: I notice now that @infinisil already asked for tests 🙂
rhendric
force-pushed
the
submit/mkUnset
branch
2 times, most recently
from
a986d00
to
aad0ed4
Compare
Using only `mkForce`/`mkOverride`, a module or configuration has no way of removing an attribute specified elsewhere; the best you could do is to replace that attribute with a null, empty set, or similar. This may result in undesired behavior, if a module implementation takes some action for every attribute in a set, for example. `mkOverrideUnset` is a function intended to be used like `mkOverride`: a `config.some.attribute.path = lib.mkOverrideUnset p` means that, if `p` is the winning priority among all definitions of `some.attribute.path`, then `some.attribute.path` will not exist at all in the final configuration values. `mkUnset` is a shorthand for `mkOverrideUnset 50`, analogous to `mkForce`.
rhendric
force-pushed
the
submit/mkUnset
branch
from
aad0ed4
to
8eef912
Compare
|
Ping, @nbp? (Or whoever is the appropriate person to pull the lever—apologies if I got it wrong.) |
|
I have not yet looked into the code of the patch, but I will try to look at it by the end of the weekend, feel free to remind me over irc if I forgot. Multiple comments:
|
|
There is not justification specific to |
There was a problem hiding this comment.
This code does not push down the unset property down to any attributes within, for the simple reason that there is no attribute within.
The problem is that overriden properties are evaluated at the location of option declaration, and as such one cannot use mkUnset above a set of attribute to unset, like mkOverride or mkIf. Therefore the following test case would not work with the proposed patch:
{ options.foo.bar = mkOption { type = attrsOf int; … }
config = mkMerge {
(mkUnset)
({ foo = mkUnset; })
# ({ foo.bar = mkUnset; })
# ({ foo.bar.alpha = mkUnset; })
({ foo.bar.alpha = 1; })
};
}
|
Hello, I'm a bot and I thank you in the name of the community for your contributions. Nixpkgs is a busy repository, and unfortunately sometimes PRs get left behind for too long. Nevertheless, we'd like to help committers reach the PRs that are still important. This PR has had no activity for 180 days, and so I marked it as stale, but you can rest assured it will never be closed by a non-human. If this is still important to you and you'd like to remove the stale label, we ask that you leave a comment. Your comment can be as simple as "still important to me". But there's a bit more you can do: If you received an approval by an unprivileged maintainer and you are just waiting for a merge, you can @ mention someone with merge permissions and ask them to help. You might be able to find someone relevant by using Git blame on the relevant files, or via GitHub's web interface. You can see if someone's a member of the nixpkgs-committers team, by hovering with the mouse over their username on the web interface, or by searching them directly on the list. If your PR wasn't reviewed at all, it might help to find someone who's perhaps a user of the package or module you are changing, or alternatively, ask once more for a review by the maintainer of the package/module this is about. If you don't know any, you can use Git blame on the relevant files, or GitHub's web interface to find someone who touched the relevant files in the past. If your PR has had reviews and nevertheless got stale, make sure you've responded to all of the reviewer's requests / questions. Usually when PR authors show responsibility and dedication, reviewers (privileged or not) show dedication as well. If you've pushed a change, it's possible the reviewer wasn't notified about your push via email, so you can always officially request them for a review, or just @ mention them and say you've addressed their comments. Lastly, you can always ask for help at our Discourse Forum, or more specifically, at this thread or at #nixos' IRC channel. |
stale
bot
added
the
2.status: stale
stale
bot
removed
the
2.status: stale
|
I marked this as stale due to inactivity. → More info |
stale
bot
added
the
2.status: stale
|
Something like this is still imporant to me |
stale
bot
removed
the
2.status: stale
|
I marked this as stale due to inactivity. → More info |
stale
bot
added
the
2.status: stale
stale
bot
removed
the
2.status: stale
|
Just ran into a case where I needed this again. |
|
I think something like this is still probably a good idea, and I understand my own code well enough, but I don't understand the surrounding system well enough to properly address what review feedback I got, and I'm no longer maintaining the NixOS configuration I had that wanted this. So I'm going to close this PR and I explicitly grant permission to any other interested party to use any or all of this code as a starting point for a new one. |
Motivation for this change
Using only
mkForce/mkOverride, a module or configuration has no wayof removing an attribute specified elsewhere; the best you could do is
to replace that attribute with a null, empty set, or similar. This may
result in undesired behavior, if a module implementation takes some
action for every attribute in a set, for example.
mkOverrideUnsetis a function intended to be used likemkOverride: aconfig.some.attribute.path = lib.mkOverrideUnset pmeans that, ifpis the winning priority among all definitions of
some.attribute.path,then
some.attribute.pathwill not exist at all in the finalconfiguration values.
mkUnsetis a shorthand formkOverrideUnset 50, analogous tomkForce.Things done
sandboxinnix.confon non-NixOS)nix-shell -p nix-review --run "nix-review wip"./result/bin/)nix path-info -Sbefore and after)