stylix: mkEnableTarget should not check stylix.enable

[MattSturgeon]

Note

This PR is comprised of intentionally separate commits, please don't squash 🙂

The global stylix.enable option is not relevant to the per-target enable option's default. Instead it should be checked in addition to the per-target enable option so that is is effective even when a target is explicitly enabled.

Since the target module has to check the global enable separately from the per-target enable, including it in the per-target option's value has no effect.

I spotted this while working on #1244. It felt odd that enable options had a seemingly redundant default: stylix.enable && stylix.autoEnable.

See also: #543
Closes: #419

Example

To give a concrete example of where this default doesn't make sense:

{
  stylix.enable = false;
  stylix.autoEnable = true;
  stylix.targets.foo.enable = true;
}

Here, the stylix.enable option is set to false, so no targets should be enabled, regardless of their per-target enable option's value.

However if the foo target assumes it only needs to read its own enable option, in this example it would define its config even though Stylix is disabled globally.

Things done

• Changed the mkEnableTarget default.
• Added a note to the docs, clarifying that a target module must always check stylix.enable in addition to its own enable option.
• Searched treewide for problematic modules; fixed the three issues I found.

• Tested locally
• Tested in testbed
• Commit message follows commit convention
• Fits style guide
• Respects license of any existing code used

Notify maintainers

• GTK: @danth
• Kitty: @trueNAHO
• Kubecolor: @ajgon

[danth]

See the discussion in #419, which I believe is the same change as this (but without the additional fixes)

[MattSturgeon]

See the discussion in #419, which I believe is the same change as this (but without the additional fixes)

Correct, this is the same change. Sorry I didn't spot your PR, I'll cherry-pick 11166aa into this PR so you get proper attribution.

Regarding the issues discussed on that PR, they very much sound like issues caused by incorrectly guarded modules, which this PR should resolve (unless I've missed any bad modules). I'd be interested in hearing whether @trueNAHO can reproduce the reported issue on this branch.

[trueNAHO]

This PR is comprised of intentionally separate commits, please don't squash 🙂

Sure :)

The global stylix.enable option is not relevant to the per-target enable option's default. Instead it should be checked in addition to the per-target enable option so that is is effective even when a target is explicitly enabled.

Since the target module has to check the global enable separately from the per-target enable, including it in the per-target option's value has no effect.

I spotted this while working on #1244. It felt odd that enable options had a seemingly redundant default: stylix.enable && stylix.autoEnable.

[...]

See the discussion in #419, which I believe is the same change as this (but without the additional fixes)

Correct, this is the same change. Sorry I didn't spot your PR, I'll cherry-pick 11166aa into this PR so you get proper attribution.

Yes, the stylix.enable guard is currently duplicated in the lib.mkEnableTarget option and the /modules/<MODULE>/<PLATFORM>.nix modules, which is not ideal. As mentioned out in the previously linked discussion, I am in favor of removing the stylix.enable guard in the modules and keeping it in the global lib.mkEnableTarget environment:

The individual modules are already gated by stylix.enable, so there is no need for it to also affect the value of stylix.targets.«name».enable.

This should not produce any noticeable change in behavior.

Note: I have not tested this yet.

Following the reasoning from #1009 to abstract and simplify module declarations, it would be better to replace

config = lib.mkIf (config.stylix.enable && config.stylix.targets."<TARGET>".enable) {

with

config = lib.mkIf config.stylix.targets."<TARGET>".enable {

I have thought about this simplification for a while now, and in fact it would probably fix my standalone test because the

https://github.com/danth/stylix/blob/ce45f19e8acb43e5f02888d873d451e2f994546b/modules/gtk/hm.nix#L41

declaration forgot the config.stylix.enable guard.

Adding the config.stylix.enable guard to every module is a leaky abstraction, introduces boilerplate, and is extremely easy to forget. Consequently, I am in favor of closing this PR and keeping the config.stylix.enable guard in the option declaration. This would also bring us one step closer to resolving #400.

-- #419 (comment)

Regarding the issues discussed on that PR, they very much sound like issues caused by incorrectly guarded modules, which this PR should resolve (unless I've missed any bad modules).

Thanks for resolving these inconsistencies.

From 8832db0b8670818a70e7b8a9ed0ec8685925b69e Mon Sep 17 00:00:00 2001
From: Daniel Thwaites <danthwaites30@btinternet.com>
Date: Mon, 10 Jun 2024 20:33:08 +0100
Subject: [PATCH 1/5] stylix: remove `cfg.enable` from `mkEnableTarget` default
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The global `stylix.enable` option is not relevant to the per-target
`enable` option's default.

The individual modules should already be gated behind `stylix.enable`,
so there is no need for it to also affect the value of
`stylix.targets.«name».enable`.

This should not produce any change in behavior for correctly written
modules.

---

To give a concrete example:

```nix
{
  stylix.enable = false;
  stylix.autoEnable = true;
  stylix.targets.foo.enable = true;
}
```

Here, the `stylix.enable` option is set to `false`, so no targets should
be enabled, regardless of their per-target `enable` option's value.

However if the `foo` target assumes it only needs to read its own
`enable` option, in this example it would define its config even though
Stylix is disabled globally.
---
 stylix/target.nix | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/stylix/target.nix b/stylix/target.nix
index 12b8494b3..1074acb3d 100644
--- a/stylix/target.nix
+++ b/stylix/target.nix
@@ -40,7 +40,7 @@
         humanName: autoEnable:
         lib.mkEnableOption "theming for ${humanName}"
         // {
-          default = cfg.enable && cfg.autoEnable && autoEnable;
+          default = cfg.autoEnable && autoEnable;
           example = !autoEnable;
         }
         // lib.optionalAttrs autoEnable {

Based on previous discussions, I am in favor of dropping this patch.

I'd be interested in hearing whether @trueNAHO can reproduce the reported issue on this branch.

Although I am not in favor of merging "[PATCH 1/5] stylix: remove cfg.enable from mkEnableTarget default", I can still test it. Since running all my tests takes about an hour and we might drop this patch anyway, it would be convenient for me if I do not have to run my tests.

From 427868c7bdf96481bb560398d8aa1a0ba34987c5 Mon Sep 17 00:00:00 2001
From: Matt Sturgeon <matt@sturgeon.me.uk>
Date: Sun, 11 May 2025 22:04:05 +0100
Subject: [PATCH 2/5] doc: note modules must check `stylix.enable`

Added a note to the docs to clarify that a target module must always
check the global `stylix.enable`, in addition to its own `enable` option.
---
 docs/src/modules.md | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/docs/src/modules.md b/docs/src/modules.md
index 4c8de6641..fb20f996c 100644
--- a/docs/src/modules.md
+++ b/docs/src/modules.md
@@ -38,7 +38,6 @@ All modules should have an enable option created using `mkEnableTarget`.
 This is similar to
 [`mkEnableOption`](https://nix-community.github.io/docnix/reference/lib/options/lib-options-mkenableoption/)
 from the standard library, however it integrates with
-[`stylix.enable`](./options/nixos.md#stylixenable) and
 [`stylix.autoEnable`](./options/nixos.md#stylixautoenable)
 and generates more specific documentation.

@@ -58,6 +57,13 @@ A general format for modules is shown below.
 }
 ```

+> [!CAUTION]
+> You **must** check _both_ `config.stylix.enable` _and_ your target's own
+> `enable` option before defining any config.
+>
+> In the above example this is done using
+> `config = lib.mkIf (config.stylix.enable && config.stylix.targets.«name».enable)`.
+
 The human readable name will be inserted into the following sentence:

 > Whether to enable theming for «human readable name».

From f9a1ecf4a18f3a200cf4e6729438e8981292f4a0 Mon Sep 17 00:00:00 2001
From: Matt Sturgeon <matt@sturgeon.me.uk>
Date: Sun, 11 May 2025 22:21:05 +0100
Subject: [PATCH 3/5] gtk: check `stylix.enable`

All targets should check the global `enable` option, not just their own.
---
 modules/gtk/hm.nix | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/modules/gtk/hm.nix b/modules/gtk/hm.nix
index 48774d541..dfb63c1a0 100644
--- a/modules/gtk/hm.nix
+++ b/modules/gtk/hm.nix
@@ -38,7 +38,7 @@ in
     flatpakSupport.enable = config.lib.stylix.mkEnableTarget "support for theming Flatpak apps" true;
   };

-  config = lib.mkIf cfg.enable (
+  config = lib.mkIf (config.stylix.enable && cfg.enable) (
     lib.mkMerge [
       {
         warnings =

From 5e6b26ee85923c1dce2a8f1806a640ca50f4c99b Mon Sep 17 00:00:00 2001
From: Matt Sturgeon <matt@sturgeon.me.uk>
Date: Sun, 11 May 2025 22:21:05 +0100
Subject: [PATCH 4/5] kitty: check `stylix.enable`

All targets should check the global `enable` option, not just their own.
---
 modules/kitty/hm.nix | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/modules/kitty/hm.nix b/modules/kitty/hm.nix
index 72594ae55..2c2ed99b1 100644
--- a/modules/kitty/hm.nix
+++ b/modules/kitty/hm.nix
@@ -21,7 +21,7 @@ in
     };
   };

-  config = lib.mkIf cfg.enable {
+  config = lib.mkIf (config.stylix.enable && cfg.enable) {
     programs.kitty = {
       font = {
         inherit (config.stylix.fonts.monospace) package name;

From 7d94c0fe454a37004d144b676f1f32161b08c295 Mon Sep 17 00:00:00 2001
From: Matt Sturgeon <matt@sturgeon.me.uk>
Date: Sun, 11 May 2025 22:21:05 +0100
Subject: [PATCH 5/5] kubecolor: check `stylix.enable`

All targets should check the global `enable` option, not just their own.
---
 modules/kubecolor/hm.nix | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/modules/kubecolor/hm.nix b/modules/kubecolor/hm.nix
index 8a3061306..64d878fa5 100644
--- a/modules/kubecolor/hm.nix
+++ b/modules/kubecolor/hm.nix
@@ -1,9 +1,12 @@
 { config, lib, ... }:
+let
+  cfg = config.stylix.targets.kubecolor;
+in
 {
   options.stylix.targets.kubecolor.enable =
     config.lib.stylix.mkEnableTarget "kubecolor" true;

-  config = lib.mkIf config.stylix.targets.kubecolor.enable {
+  config = lib.mkIf (config.stylix.enable && cfg.enable) {
     programs.kubecolor.settings = {
       preset =
         if config.stylix.polarity == "either" then "" else "${config.stylix.polarity}";

LGTM.

[MattSturgeon]

I am in favor of removing the stylix.enable guard in the modules and keeping it in the global lib.mkEnableTarget environment:

That doesn't work. No matter what we put in the mkEnableTarget option's default value, users can override the per-target enable option's final value, resulting in the global stylix.enable option having no effect. That was what my example was attempting to demonstrate:

Example from above

{
  stylix.enable = false;
  stylix.autoEnable = true;
  stylix.targets.foo.enable = true;
}

We could work around this with an apply function, however (in this case) it would be ugly and (IMO) not really in the spirit of the module system:

Example option with an apply function

enable = lib.mkOption {
  type = lib.types.bool;
  default = config.stylix.autoEnable;
  apply = value: config.stylix.enable && value;
  description = ''
    Regardless of how you define this option, it can only be `true` if `stylix.enable` is also `true`.
  '';
};

An apply function modifies an option's final value, after all definition merging is done.

The problem with this approach is a configuration/module/user can never check which modules/targets/options would be enabled while the global enable option is disabled. Reading any enable options would simply be false.

I believe individual modules not needing to implement the condition by hand is the primary motivation for adding a mkTarget function that creates the entire module (#1130). Such a function can add additional mkIf conditions without needing to hackily modify the target-enable options.

[ajgon]

Late to the party, but I completely agree, the global enable should take precedence in this case. Thanks for keeping me posted 👍

[0xda157]

LGTM

[MattSturgeon]

Rebased to resolve conflicts

[trueNAHO]

I am in favor of removing the stylix.enable guard in the modules and keeping it in the global lib.mkEnableTarget environment:

That doesn't work. No matter what we put in the mkEnableTarget option's default value, users can override the per-target enable option's final value, resulting in the global stylix.enable option having no effect. [...]

Good point. I did not consider that.

We could work around this with an apply function, however (in this case) it would be ugly and (IMO) not really in the spirit of the module system:

[...]

An apply function modifies an option's final value, after all definition merging is done.

The problem with this approach is a configuration/module/user can never check which modules/targets/options would be enabled while the global enable option is disabled. Reading any enable options would simply be false.

Thanks for elaborating on this because I would have proposed apply otherwise :)

I believe individual modules not needing to implement the condition by hand is the primary motivation for adding a mkTarget function that creates the entire module (#1130). Such a function can add additional mkIf conditions without needing to hackily modify the target-enable options.

Agreed. Considering that all my tests pass in this PR, I also approve "[PATCH 1/5] stylix: remove cfg.enable from mkEnableTarget default". Hopefully, this does not introduce any weird edge case bugs.