Use mkEnableOption throughout Nixos where appropriate

[langston-barrett]

Motivation for this change

From #18524:

Many, many services use the following snippet:

      enable = mkOption {
        default = false;
        type = types.bool;
        description = ''
          Whether to enable Mopidy, a music player daemon.
        '';
      };

That's why mkEnableOption was created!

  mkEnableOption = name: mkOption {
    default = false;
    example = true;
    description = "Whether to enable ${name}.";
    type = lib.types.bool;
  };

I wrote this script:

#!/usr/bin/env nix-shell
#! nix-shell -i bash -p ag

git checkout master nixos/modules/
cd "nixos/modules"

# Loop through files with "enable = mkOption"
for file in $(IFS=\n ag -l "enable = mkOption"); do
  # This line will exit 0 if the default for "enable" is false.
  if perl -0777 -e 'exit 1 if (<STDIN> =~ m/.+?enable\s+=\s+mkOption\s+{(.|\n)+?default\s+=\s+true;(.|\n)+?};/)' < "$file"; then
    # This line will replace
    # enable = mkEnableOption "Foobar, the baz";
    # with
    # enable = mkEnableOption "Foobar, the baz";
    perl -0777 -p -i -e 's/(.+?)enable\s+?=\s+?mkOption\s+?\{.+?enable ([^\n]+?)\..+?};/\1enable = mkEnableOption "\2";/igs' "$file"
  fi
done

cd ../../
# Loop through the modified files and confirm the diffs
for file in $(git status --porcelain | awk 'match($1, "M"){print $2}'); do
  echo "----------------------------------------------------"
  echo "Does the diff look ok for $file?"
  echo "----------------------------------------------------"
  git --no-pager diff master "$file"
  read -r -p "[y/N]: " response
  case $response in
    [yY][eE][sS]|[yY])
      git add "$file"
      tmp=${file%.nix}
      name=${tmp##*/}
      git commit -m "$name module: use mkEnableOption"
      ;;
    *)
      git checkout master "$file"
      ;;
  esac
done

I was conservative, and only accepted diffs where the description was close to exactly the same. I've also broken it up into a commit per file, which will allow for easier reverting (without redoing all this work), should a problem arise.

Next steps would be to review the diffs where the script broke/did not correctly fill in the description, and manually change those to use mkEnableOption.

Things done

• Tested using sandboxing
  (nix.useSandbox on NixOS,
  or option build-use-sandbox in nix.conf
  on non-NixOS)
• Built on platform(s)
  • NixOS
  • OS X
  • Linux
• Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
• Tested execution of all binary files (usually in ./result/bin/)
• Fits CONTRIBUTING.md.

[mention-bot]

@siddharthist, thanks for your PR! By analyzing the annotation information on this pull request, we identified @domenkozar, @edolstra and @unjordy to be potential reviewers

[domenkozar]

I (very opinionated) think such abstractions do more harm than good. I don't mind a little verbosity (no logic, just parameters) for sake of less abstraction.

But having said that, this one is not too bad and it's mostly self explainatory once the definition of the function is looked up.

+0

[groxxda]

+1 on making stuff consistent
but like domen I'm also not sure whether mkEnableOption is really superior or whether we should get rid of it.

+0

[langston-barrett]

I think this has two major advantages:

1. Less repetition/boilerplate
2. Consistency in examples and phrasing

And one major disadvantage:

1. Having to look up the function definition

I leave it up to the maintainers which of those goals are more important. If we decide not to merge this PR, I propose eliminating mkEnableOption altogether for the sake of consistency.

[rasendubi]

-0.5 as this potentially overlaps with #14860 (which adds more value)

[chris-martin]

I think we should get rid of mkEnableOption.

1. I believe getting new people contributing is extremely important to NixOS. Nixpkgs, being without static types, is somewhat intimidating to read as a newcomer. You gain a foothold in this repo is recognizing patterns (such as seeing mkOption in a lot of places), grepping for things (such as to find examples of how mkOption is used), and building new things by following examples in the code (such as adding a new option by copying and modifying an existing mkOption call). More functions means less uniformity among the packages and a more difficult initial orientation process.
2. This is likely to encourage poor documentation. The default description, Whether to enable ${name}, is not sufficient for all boolean options. mkEnableOption rewards not including a more meaningful description, and it introduces friction into doing so later (since it requires changing to a different function).

[ryanartecona]

I emphatically agree with @chris-martin. I remember it being very tricky to orient myself around NixOS modules, and I don't think this would help.

[rasendubi]

After a week, feedback is mostly negative. I propose to close this.

[domenkozar]

Sorry @siddharthist :)

[fstamour]

@siddharthist I ❤️ your script thought. Pretty streamlined way of doing a bunch of similar changes and still keeping them in separated an validated commits.

[jagajaga]

@siddhanathan can you please reopen and retest your PR?

[langston-barrett]

@jagajaga I will if/once we reach a consensus. Let's continue the discussion on #18767.