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

treewide: disable programs.nix-index.enable option by default #132

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

trueNAHO
Copy link

This patchset does some cleanup and disables the programs.nix-index.enable option by default for the following reason:

treewide: disable programs.nix-index.enable option by default

Disable the programs.nix-index.enable option by default to align with
Nix conventions and streamline configuration sharing.

This avoids needing to remember to disable this module across all
configurations and avoids highly discouraged conditional imports that
often lead to infinite recursions [1].

[...]

[1]: https://discourse.nixos.org/t/conditional-module-imports/34863/2

The programs.nix-index.enable option was first introduced in commit 75e347f ("Allow command-not-found integration with home-manager.") and set to true:

enable = lib.mkDefault true;

To this day, programs.nix-index.enable is still set to true:

config.programs.nix-index.enable = lib.mkDefault true;


NAHO (5):
  tests: remove default option declaration
  readme: remove trailing whitespaces
  treewide: disable programs.nix-index.enable option by default
  readme: improve programs.nix-index-database.comma.enable description
  readme: subjectively improve examples

 README.md      | 88 ++++++++++++++++++++++++++++++++------------------
 nix/shared.nix |  2 --
 tests.nix      |  6 ++--
 3 files changed, 60 insertions(+), 36 deletions(-)

@r-vdp
Copy link
Collaborator

r-vdp commented Dec 12, 2024

Should we maybe start by printing a warning if the option is set to true by this module, to give people the chance to update their configs?

@trueNAHO
Copy link
Author

Should we maybe start by printing a warning if the option is set to true by this module, to give people the chance to update their configs?

What about doing the opposite? If programs.nix-index.enable == false, then we emit a warning to inform users that this module may have been unbeknownst disabled. This warning should be removed after some time, like after the release of nixos-25.12.

@r-vdp
Copy link
Collaborator

r-vdp commented Dec 12, 2024

Should we maybe start by printing a warning if the option is set to true by this module, to give people the chance to update their configs?

What about doing the opposite? If programs.nix-index.enable == false, then we emit a warning to inform users that this module may have been unbeknownst disabled. This warning should be removed after some time, like after the release of nixos-25.12.

But that warning would be hard to avoid in case you actually conditionally enable the module. I think it should be possible to not have any warnings to avoid warning fatigue.

Fixes: 5f154bf ("nixos-module: disable programs.command-not-found by default")
Link: nix-community#132
Disable the programs.nix-index.enable option by default to align with
Nix conventions and streamline configuration sharing.

This avoids needing to remember to disable this module across all
configurations and avoids highly discouraged conditional imports that
often lead to infinite recursions [1].

BREAKING CHANGE: The programs.nix-index.enable option is disabled by
default, meaning that this module has no effect unless the following is
declared:

    programs.nix-index.enable = true;

[1]: https://discourse.nixos.org/t/conditional-module-imports/34863/2

Link: nix-community#132
trueNAHO added a commit to trueNAHO/nix-index-database that referenced this pull request Dec 13, 2024
@trueNAHO trueNAHO force-pushed the treewide-disable-programs-nix-index-enable-option-by-default branch from 4b18f07 to fd1dc42 Compare December 13, 2024 13:47
@trueNAHO trueNAHO marked this pull request as draft December 13, 2024 13:48
@trueNAHO trueNAHO marked this pull request as ready for review December 13, 2024 13:54
@trueNAHO
Copy link
Author

trueNAHO commented Dec 13, 2024

What about doing the opposite? If programs.nix-index.enable == false, then we emit a warning to inform users that this module may have been unbeknownst disabled. This warning should be removed after some time, like after the release of nixos-25.12.

But that warning would be hard to avoid in case you actually conditionally enable the module. I think it should be possible to not have any warnings to avoid warning fatigue.

This should be resolved by commit shared: add programs.nix-index.acknowledgeBreakingChange option.

For reference, I am using this PR as follows:

commit ebbf6a3cb5255e8717ee00650e382de7971765e1
Author: NAHO <[email protected]>
Date:   2024-12-12 15:40:47 +0100

    nix-index: init
    
    Initialize the nix-index module with pre-generated databases and comma
    [1] support by leveraging the nix-index-database [3] wrapper of
    nix-index [2].
    
    This changes the behaviour of the fish_command_not_found function.
    
    [1]: https://github.com/nix-community/comma
    [2]: https://github.com/nix-community/nix-index
    [3]: https://github.com/nix-community/nix-index-database

diff --git a/flake.lock b/flake.lock
index 9df4f840..41d49c60 100644
--- a/flake.lock
+++ b/flake.lock
@@ -355,6 +355,27 @@
         "type": "github"
       }
     },
+    "nix-index-database_2": {
+      "inputs": {
+        "nixpkgs": [
+          "nixpkgs"
+        ]
+      },
+      "locked": {
+        "lastModified": 1734097052,
+        "narHash": "sha256-FKXpL6/RWwKo3B5AxqUeYOddxXuvuzMAAP6ffg253no=",
+        "owner": "trueNAHO",
+        "repo": "nix-index-database",
+        "rev": "fd1dc4242b789bd6e22309c73a97c4119819eae7",
+        "type": "github"
+      },
+      "original": {
+        "owner": "trueNAHO",
+        "ref": "treewide-disable-programs-nix-index-enable-option-by-default",
+        "repo": "nix-index-database",
+        "type": "github"
+      }
+    },
     "nixpkgs": {
       "locked": {
         "lastModified": 1733392399,
@@ -470,6 +491,7 @@
         "home-manager": "home-manager",
         "logo": "logo",
         "nix-alien": "nix-alien",
+        "nix-index-database": "nix-index-database_2",
         "nixpkgs": "nixpkgs_3",
         "nixvim": "nixvim",
         "os": "os",
diff --git a/flake.nix b/flake.nix
index 9c751904..969d25d0 100644
--- a/flake.nix
+++ b/flake.nix
@@ -84,6 +84,14 @@
       url = "github:thiagokokada/nix-alien";
     };
 
+    # TODO: Replace this fork with upstream once [1] is merged.
+    #
+    # [1]: https://github.com/nix-community/nix-index-database/pull/132
+    nix-index-database = {
+      inputs.nixpkgs.follows = "nixpkgs";
+      url = "github:trueNAHO/nix-index-database/treewide-disable-programs-nix-index-enable-option-by-default";
+    };
+
     nixpkgs.url = "github:nixos/nixpkgs/nixos-unstable";
 
     nixvim = {
diff --git a/home_configurations/private/full/default.nix b/home_configurations/private/full/default.nix
index 12c27f01..9b799cda 100644
--- a/home_configurations/private/full/default.nix
+++ b/home_configurations/private/full/default.nix
@@ -192,6 +192,7 @@ lib.dotfiles.homeManagerConfiguration.homeManagerConfiguration
             full = true;
           };
 
+          nix-index.enable = true;
           nix-your-shell.enable = true;
           password-store.enable = true;
           qutebrowser.enable = true;
diff --git a/home_configurations/private/full/imports.nix b/home_configurations/private/full/imports.nix
index 9ac923bd..2e34e554 100644
--- a/home_configurations/private/full/imports.nix
+++ b/home_configurations/private/full/imports.nix
@@ -159,6 +159,7 @@
     ../../../modules/inputs/home-manager/programs/lazygit
     ../../../modules/inputs/home-manager/programs/man
     ../../../modules/inputs/home-manager/programs/mpv
+    ../../../modules/inputs/home-manager/programs/nix-index
     ../../../modules/inputs/home-manager/programs/nix-your-shell
     ../../../modules/inputs/home-manager/programs/password-store
     ../../../modules/inputs/home-manager/programs/qutebrowser
diff --git a/modules/inputs/home-manager/programs/fish/default.nix b/modules/inputs/home-manager/programs/fish/default.nix
index da99254f..aebe8df3 100644
--- a/modules/inputs/home-manager/programs/fish/default.nix
+++ b/modules/inputs/home-manager/programs/fish/default.nix
@@ -90,15 +90,6 @@
                 description = "Output parent directory multiple times";
               };
 
-              fish_command_not_found = {
-                body =
-                  validateFunctionArguments
-                  "fish_command_not_found"
-                  "nix run nixpkgs#$argv[1] -- $argv[2..]";
-
-                description = "What to do when a command wasn't found";
-              };
-
               fish_mode_prompt = fishModePrompt "";
 
               fish_greeting = {
diff --git a/modules/inputs/home-manager/programs/nix-index/default.nix b/modules/inputs/home-manager/programs/nix-index/default.nix
new file mode 100644
index 00000000..da9f05f3
--- /dev/null
+++ b/modules/inputs/home-manager/programs/nix-index/default.nix
@@ -0,0 +1,28 @@
+{
+  config,
+  inputs,
+  lib,
+  ...
+}: {
+  imports = [inputs.nix-index-database.hmModules.nix-index];
+
+  options.dotfiles.inputs.home-manager.programs.nix-index.enable =
+    lib.dotfiles.mkEnableOption;
+
+  config = lib.mkMerge [
+    {
+      programs.nix-index.acknowledgeBreakingChange = true;
+    }
+
+    (
+      lib.mkIf
+      config.dotfiles.inputs.home-manager.programs.nix-index.enable
+      {
+        programs = {
+          nix-index-database.comma.enable = true;
+          nix-index.enable = true;
+        };
+      }
+    )
+  ];
+}

@trueNAHO trueNAHO force-pushed the treewide-disable-programs-nix-index-enable-option-by-default branch from fd1dc42 to 928ab2e Compare December 13, 2024 14:01
@trueNAHO
Copy link
Author

trueNAHO commented Dec 13, 2024

Changelog

v2: 928ab2e

  • Rename commit shared: add config.programs.nix-index.acknowledgeBreakingChange option to shared: add programs.nix-index.acknowledgeBreakingChange option

v1: fd1dc42

  • Add commit shared: add config.programs.nix-index.acknowledgeBreakingChange option, allowing end-users to explicitly acknowledge and disable the breaking change warning
  • Add more changes to commit readme: subjectively improve examples
  • Reorder commits such that commit shared: add config.programs.nix-index.acknowledgeBreakingChange option and treewide: disable programs.nix-index.enable option by default are the last two commits
  • Add Link: https://github.com/nix-community/nix-index-database/pull/132 tag to every commit

v0: 4b18f07

@Mic92
Copy link
Member

Mic92 commented Dec 23, 2024

@r-vdp can you have a look again?

@r-vdp
Copy link
Collaborator

r-vdp commented Dec 23, 2024

I'll try to have a look at it tomorrow.

I would prefer to move the second commit to another PR, because it's not related to the functional change here and I don't feel like arguing about stylistic changes.

I don't love the idea of the extra option to acknowledge the change, I'd like to avoid it if possible, but if there's no better way, then we leave it.
I wonder if we cannot just assign lib.warn "..." true as the option default and have people assign true or false explicitly, instead of introducing yet another option. But I'm not sure if the module system is sufficiently non-strict to avoid forcing values assigned at lower priority than the one that will end up used.

@trueNAHO
Copy link
Author

trueNAHO commented Dec 24, 2024

I would prefer to move the second commit to another PR, because it's not related to the functional change here and I don't feel like arguing about stylistic changes.

The

readme: subjectively improve examples
tests: remove default option declaration

commits are included in this patchset to avoid merge conflicts with the

treewide: disable programs.nix-index.enable option by default
shared: add programs.nix-index.acknowledgeBreakingChange option

commits, while the

readme: remove trailing whitespaces

commit is included in this patchset for simplicity reasons.

I don't love the idea of the extra option to acknowledge the change, I'd like to avoid it if possible, but if there's no better way, then we leave it. I wonder if we cannot just assign lib.warn "..." true as the option default and have people assign true or false explicitly, instead of introducing yet another option. But I'm not sure if the module system is sufficiently non-strict to avoid forcing values assigned at lower priority than the one that will end up used.

The following end-user module triggers the warning when config.nix-index.enable == false, if we declare config.programs.nix-index.enable to lib.mkDefault (lib.warn "..." false):

{
  config,
  inputs,
  lib,
  ...
}: {
  imports = [inputs.nix-index-database.hmModules.nix-index];
  options.nix-index.enable = lib.mkEnableOption "nix-index";

  config = lib.mkIf config.nix-index.enable {
    programs = {
      nix-index-database.comma.enable = true;
      nix-index.enable = true;
    };
  };
}

In that case, end-users have to use the following potentially undesired workaround:

11,15c11,13
<   config = lib.mkIf config.nix-index.enable {
<     programs = {
<       nix-index-database.comma.enable = true;
<       nix-index.enable = true;
<     };
---
>   config.programs = {
>     nix-index-database.comma.enable = config.nix-index.enable;
>     nix-index.enable = config.nix-index.enable;

If we provide an explicit breaking change option, end-users get a proper warning once this option is deprecated:

{
  config,
  inputs,
  lib,
  ...
}: {
  imports = [inputs.nix-index-database.hmModules.nix-index];
  options.nix-index.enable = lib.mkEnableOption "nix-index";

  config = lib.mkMerge [
    {
      # This will trigger a proper warning once this upstream option is
      # deprecated with lib.mkRemovedOptionModule.
      programs.nix-index.acknowledgeBreakingChange = true;
    }

    (
      lib.mkIf config.nix-index.enable {
        programs = {
          nix-index-database.comma.enable = true;
          nix-index.enable = true;
        };
      }
    )
  ];
}

Otherwise, end-users have to use the workaround and will never be notified when the workaround is no longer necessary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants