lib/modules: add mkUnset, mkOverrideUnset

[rhendric]

Motivation for this change

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.

Things done

• Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
• Built on platform(s)
  • NixOS
  • macOS
  • other Linux distributions
• Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
• Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-review wip"
• Tested execution of all binary files (usually in ./result/bin/)
• Determined the impact on package closure size (by running nix path-info -S before and after)
• Assured whether relevant documentation is up to date
• Fits CONTRIBUTING.md.

[netixx]

Can't say anything about the implementation, but the API seems good to me :)

Very much like to have this feature as well!

[danbst]

@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.

[infinisil]

Do you have some concrete examples of where this would have been really useful? In general I think attrsets having an .enable option is nicer than this, but I understand that not all attrset options provide that, so this might be needed after all.

One thing I'd like to see for this is a test in nixpkgs/lib/tests though.

[danbst]

@infinisil this is mostly useful for attrsOf XXX option types, which are easy to add new ones, but impossible to remove existing ones. Luckily we have systemd.services.*.enable, but we don't have containers.*.enable or services.nginx.virtualHosts.*.enable,

[rhendric]

@infinisil, there's also things like systemd.services.*.unitConfig.*, which, once set, can't be individually removed without clobbering the entire unitConfig (you can set them individually to null, but that still leaves an empty Foo= line in the generated unit file). I suspect .enable wouldn't be a palatable way to handle individual unitConfig entries. I wonder if this pattern—using an attrset of configuration options in which an absent attribute induces a different outcome than an attribute that is present with some neutral value—appears elsewhere in nixpkgs... if not, maybe this is just an edge case.

One thing I'd like to see for this is a test in nixpkgs/lib/tests though.

Ah, that's where they live! Yes, I'll gladly do that.

[infinisil]

Cool, sounds reasonable to add this then. I'm also going to ping @nbp and @rycee.

[rycee]

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 🙂

[rycee]

Looks good to me!

[rhendric]

Ping, @nbp? (Or whoever is the appropriate person to pull the lever—apologies if I got it wrong.)

[nbp]

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:

• the proper wording is undefined, and mkUndef. Unset implies an imperative order of execution, which does not exist here. Instead, you define an empty value that take precedence or not over other values which are defined.
• This has nothing to do with mkOverride, this provide a non-existing value which then can be combined with mkOverride and mkIf. mkOVerrideUndef is literally equivalent to mkOverride … mkUndef.
• This used to exists and got removed, and we should investigate why before merging these changes.

[nbp]

mkNotDef got removed by 0e33368#diff-490491facef0aa6a6dbe8ffc5bd97097 , probably because the properties managed to be implemented without this attribute.

There is not justification specific to mkNotDef, only arguments about the efficiency of the module system.

[nbp]

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; })
  };
}

[stale]

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]

I marked this as stale due to inactivity. → More info

[dali99]

Something like this is still imporant to me

[stale]

I marked this as stale due to inactivity. → More info

[RaitoBezarius]

Just ran into a case where I needed this again.

[rhendric]

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.