stylix/home-manager-integration: enable even when stylix.enable = false#2089
stylix/home-manager-integration: enable even when stylix.enable = false#2089ThinkChaos wants to merge 1 commit intonix-community:masterfrom
stylix.enable = false#2089Conversation
…lse` Always enable this, even when `!stylix.enable`: if the HM module isn't imported, it prevents users from setting any stylix related options in their HM config. That would go against the standard pattern for `enable` options: they should not affect what you're allowed to write in `config`, thus we need to import the module; nor affect the value of the module's options, thus we need to propagate the NixOS `config.stylix` values.
71a3618 to
697d310
Compare
There was a problem hiding this comment.
Thanks for the patch, but this seems to be a duplicate of the following pending patch review:
From e698150f68a2e443c9c01133ee072abaab3de47b Mon Sep 17 00:00:00 2001 From: NAHO <90870942+trueNAHO@users.noreply.github.com> Date: Tue, 19 Aug 2025 23:36:32 +0200 Subject: [PATCH 2/3] stylix/home-manager-integration: do not guard on stylix.enable Do not guard on stylix.enable because the original issue from commit 73b7d0f30039 ("stylix: guard home-manager-integration config (#1494)") seems to have resolved itself in the meantime. This allows testbeds to declare Stylix target options when Stylix is disabled. Reverts: 73b7d0f30039 ("stylix: guard home-manager-integration config (#1494)") --- stylix/home-manager-integration.nix | 32 ++++++++++++++--------------- 1 file changed, 15 insertions(+), 17 deletions(-) diff --git a/stylix/home-manager-integration.nix b/stylix/home-manager-integration.nix index a169e1a4f..51b1cbd59 100644 --- a/stylix/home-manager-integration.nix +++ b/stylix/home-manager-integration.nix @@ -208,22 +208,20 @@ in }; }; - config = lib.mkIf config.stylix.enable ( - lib.optionalAttrs (options ? home-manager) ( - lib.mkMerge [ - (lib.mkIf config.stylix.homeManagerIntegration.autoImport { - home-manager.sharedModules = - [ - config.stylix.homeManagerIntegration.module - ] - ++ lib.optionals config.stylix.homeManagerIntegration.followSystem copyModules; - }) - (lib.mkIf config.home-manager.useGlobalPkgs { - home-manager.sharedModules = lib.singleton { - config.stylix.overlays.enable = false; - }; - }) - ] - ) + config = lib.optionalAttrs (options ? home-manager) ( + lib.mkMerge [ + (lib.mkIf config.stylix.homeManagerIntegration.autoImport { + home-manager.sharedModules = + [ + config.stylix.homeManagerIntegration.module + ] + ++ lib.optionals config.stylix.homeManagerIntegration.followSystem copyModules; + }) + (lib.mkIf config.home-manager.useGlobalPkgs { + home-manager.sharedModules = lib.singleton { + config.stylix.overlays.enable = false; + }; + }) + ] ); }-- #1857
I will close this for now as duplicate.
|
Thanks for the reply! I don't see that patch in the linked MR, is it pushed somewhere else? EDIT: found the PR #1857 |
Reviewing this exact patch is pending in #1857 (comment), with the danger being that it reverts a previously known to work fix. I am in favor of this change because it seems to work, although this assumption could be wrong. Since it changes the core, I would like approval from at least one of the other Stylix maintainers.
Yes, it could be cherry-picked and merged earlier, but that does not change that its review has been pending. Please continue this discussion in #1857 (comment) to avoid further fragmentation.
Sorry for providing the wrong GitHub URL. At least it was the correct patch. I updated my previous message. |
Always enable this, even when
!stylix.enable: if the HM module isn't imported, it prevents users from setting any stylix related options in their HM config.That would go against the standard pattern for
enableoptions: they should not affect what you're allowed to write inconfig, thus we need to import the module; nor affect the value of the module's options, thus we need to propagate the NixOSconfig.stylixvalues.