Skip to content
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: add mkEnabledOption #94983

Closed
wants to merge 1 commit into from
Closed

lib: add mkEnabledOption #94983

wants to merge 1 commit into from

Conversation

@ju1m
Copy link
Contributor

@ju1m ju1m commented Aug 9, 2020

Motivation for this change

Some enable* options are better with a true default, but there is only a mkEnableOption in the lib.
Eg. openFirewall in #94917

Things done
  • Add a mkEnabledOption in lib/options.nix.
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • 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 nixpkgs-review --run "nixpkgs-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)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
@Ma27
Copy link
Member

@Ma27 Ma27 commented Aug 9, 2020

I think that the description "whether to enable x" is fairly unintuitive for a "disable-option". If we actually want to add a "disable"-option, the options should be named accordingly (disable = mkDisableOption ...) btw.

@ju1m
Copy link
Contributor Author

@ju1m ju1m commented Aug 9, 2020

@Ma27 I understand but I would not reason on the name of the option there because this is no longer done for mkEnableOption which is now also used to make options without "enable" in their name like: debug, trace, openFirewall, ... So, like in those, the "disable" in mkDisableOption would not refer to the name of the option (eg. openFirewall, enforce, ...) but to its intent : to let the user disable something.

Moreover, I would stick to the enable option-name convention instead of introducing disable options (of which I can't found a single one in Nixpkgs) because this facilitates any potential future change of the default, and avoid the cognitive overhead of a double negation like disable = false.

This other name can be found in nixos/modules/services/networking/i2pd.nix:

mkEnableTrueOption = name: mkEnableOption name // { default = true; };

To be truly option-name and option-intent agnostic we could have a mkFalseByDefaultOption and rename mkTrueByDefaultOption, but this is longer, could not wrap description with Whether to enable, and at this point I think would not be clearer than a direct option declaration.

@xaverdh
Copy link
Contributor

@xaverdh xaverdh commented Aug 9, 2020

Maybe we should just encourage people to use mkEnableOption name // { default = true; } instead of manually writing down the option? Actually find that quite readable and concise – i.e. do we really have to encode this in a new function?

@Infinisil
Copy link
Member

@Infinisil Infinisil commented Aug 9, 2020

Some enable* options are better with a true default, but there is only a mkEnableOption in the lib.
Eg. openFirewall in #94917

openFirewall should really be false by default, because we want the user to know that they potentially open themselves to the network.

@ju1m
Copy link
Contributor Author

@ju1m ju1m commented Aug 10, 2020

openFirewall should really be false by default, because we want the user to know that they potentially open themselves to the network.

@Infinisil, this example may indeed be a case where openFirewall defaulting to true is not the wisest default, let it be discussed in the relevant PR. However sometimes it has been decided that openFirewall would default to true, like in openssh.openFirewall:

openFirewall = mkOption {
type = types.bool;
default = true;

@ju1m
Copy link
Contributor Author

@ju1m ju1m commented Aug 10, 2020

Maybe we should just encourage people to use mkEnableOption name // { default = true; } instead of manually writing down the option? Actually find that quite readable and concise – i.e. do we really have to encode this in a new function?

@xaverdh That's a possibility, though it does not set example to false which would make more sense, and is less clear when the name description spans over several lines.

@Ma27
Copy link
Member

@Ma27 Ma27 commented Aug 10, 2020

openFirewall should really be false by default, because we want the user to know that they potentially open themselves to the network.

This is totally out of scope here, however I should note that openFirewall is usually false by default, the only exception is sshd where we really want to make sure that people don't lock themselves out. See also Graham's comment about this: #60124 (comment)

That's a possibility, though it does not set example to false which would make more sense

Is the example of an option of type boolean really that important though?

So, like in those, the "disable" in mkDisableOption would not refer to the name of the option (eg. openFirewall, enforce, ...) but to its intent : to let the user disable something.

True, but all examples you mentioned can be used as equivalent to enable, however disable is the opposite and therefore unintuitive IMHO.

@ajs124
Copy link
Member

@ajs124 ajs124 commented Sep 9, 2020

I've been thinking of adding this for a while. Which isn't much of an endorsement but more of an "I've also come up with this was too lazy to do it". My (troll) name for it would have been mkEnabledOption, because it makes an options that's already enabled.

@ju1m ju1m changed the title lib: add mkDisableOption lib: add mkEnabledOption Sep 10, 2020
@ju1m
Copy link
Contributor Author

@ju1m ju1m commented Sep 10, 2020

Thanks @ajs124, your mkEnabledOption proposal is greater, I've changed this PR to introduce it instead.

@dasJ
Copy link
Member

@dasJ dasJ commented Oct 13, 2020

A lighter implementation might be { ... }@args: mkEnableOption args // { default = true; }

@Infinisil
Copy link
Member

@Infinisil Infinisil commented Oct 21, 2020

I think introducing such a function doesn't add much benefit, since as previously stated, the same can be achieved with mkEnableOption // { default = true; }, which is imo easier to understand and also tells the reader "Watch out there is something enabled by default here". In addition, generally the default state of "enable" options should be false, not true. By introducing such a function we would encourage true defaults. And finally, mkEnabledOption is very easy to confuse with mkEnableOption, both during authoring and reviewing of modules. If at all, it would have to be named differently.

Overall, I'm rather opposed to this change.

@ju1m
Copy link
Contributor Author

@ju1m ju1m commented Oct 27, 2020

@Infinisil Oh right, I haven't thought about the possible confusion, this is unfortunate. Concerning the other arguments I would only repeat #94983 (comment). This said, by following the suggestion made to me to span out this PR from #101071 and an old version of #94917, where mkDisableOption is defined for factoring, I was definitively not expecting this PR to be a controversial one, it was just a function arising naturally for me. But in the end the use cases seem to be so little, so thank you all for your time and comments but I no longer want to spend time on defending this PR and wish us all to focus on more important ones. Like said #101071 where I'm completely lost.

@ju1m ju1m closed this Oct 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants