stylix: remove cfg.enable from mkEnableTarget default#419
Closed
stylix: remove cfg.enable from mkEnableTarget default#419
cfg.enable from mkEnableTarget default#419Conversation
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.
trueNAHO
reviewed
Jun 11, 2024
Member
There was a problem hiding this comment.
This should not produce any noticeable change in behavior.
Note: I have not tested this yet.
In my standalone Home Manager setup, I receive the following error:
error: A definition for option `stylix.image' is not of type `path or package convertible to it'. Definition values:
- In `/nix/store/d33h4m5arzybvnvl7ibmis8svpahsadm-source/modules/stylix': null
Note that this could be a problem with my setup.
Merging #407 would help.
Member
Author
|
That's unusual. Could be caused by some of the recent changes, but I suspect it's not this one. Can you share your config? |
|
Hey I would like to comment that removing the target toggles would cause regressions for edge-case uses such as those in my own NixOS configuration:
Having these toggles make it easier for me to fine tune what exactly I want to use; especially since there's currently no way to change the smaller details of how stylix applies colors (basically what PR #270 proposes with having 'swatches' for different applications). |
Member
Author
|
I think you've misunderstood the change here - the individual target enable options won't be removed. Instead, it's just removing the behaviour where the default for each individual enable option changes to |
Ah, you're right! I did completely misinterpret the change. Thank you for the clarification! 😅 |
Member
My standalone Home Manager setup 3 is fairly complex. The relevant minimal working example is: nix build .#checks.x86_64-linux.standalone-homeManager-programs-mpv-enable-falseThe minimal working example (MWE) is generated based on my MPV module 1, which Upon applying the following patch, the previous MWE fails: From e3b838a88fe463828de19c7a22eb1a7823a0d1ee Mon Sep 17 00:00:00 2001
From: NAHO <90870942+trueNAHO@users.noreply.github.com>
Date: Tue, 11 Jun 2024 01:27:43 +0200
Subject: [PATCH] chore!: 2024-06-11 01:27:43 +0200
---
flake.lock | 7 ++++---
flake.nix | 2 +-
2 files changed, 5 insertions(+), 4 deletions(-)
diff --git a/flake.lock b/flake.lock
index ad971932..0ef77b0e 100644
--- a/flake.lock
+++ b/flake.lock
@@ -548,15 +548,16 @@
]
},
"locked": {
- "lastModified": 1718013167,
- "narHash": "sha256-L+IzjhovTTqOzqLXjrfGFsDPVuCLWZTah+rt7wRkGJ8=",
+ "lastModified": 1718047991,
+ "narHash": "sha256-S/6sxMs6aWlHqveNRTyQq4sQEMICtmmem61P0Vq2Lfo=",
"owner": "danth",
"repo": "stylix",
- "rev": "7682713f6af1d32a33f8c4e3d3d141af5ad1761a",
+ "rev": "11166aa5a2b92437f03cf0b57fd51ef490dee635",
"type": "github"
},
"original": {
"owner": "danth",
+ "ref": "remove-default-cfg-enable",
"repo": "stylix",
"type": "github"
}
diff --git a/flake.nix b/flake.nix
index c83e2c25..4a8f1519 100644
--- a/flake.nix
+++ b/flake.nix
@@ -89,7 +89,7 @@
nixpkgs.follows = "nixpkgs";
};
- url = "github:danth/stylix";
+ url = "github:danth/stylix/remove-default-cfg-enable";
};
# This input ensures consistent versioning across inputs.
--
2.44.0Note that the following complementary MWE, setting all nix build .#checks.x86_64-linux.standalone-homeManager-programs-mpv-enable-trueUnless you see what the problem is, I can try to build a simple static MWE that does not rely on my complex generation process. Also, the GitHub Actions currently fail due to timeout. Everything works as expected.
I suspect the problem stems from the fact that something is not properly guarded. Either on my end or in Stylix. |
Member
Potentially related: #421 |
MercuryHgO
approved these changes
Jun 11, 2024
MercuryHgO
approved these changes
Jun 11, 2024
2 tasks
Contributor
|
what is the status of this PR? |
Member
Author
|
It seems there was an issue with some modules not being guarded properly with Note that this PR may also cause changes in behaviour for anyone who uses the value of a |
Member
Author
|
@trueNAHO could you test this again now that it's been updated to the latest code? |
Member
My nix build .#checks.x86_64-linux.standalone-inputs-home-manager-programs-mpv-enable-falseMPV test still fails, but this time with another error: The same error appears in my LibreWolf module: nix build .#checks.x86_64-linux.standalone-inputs-home-manager-programs-librewolf-enable-falseFor reference, this test creates a minimal standalone Home Manager configuration that imports my LibreWolf module while disabling all Here are my related (unedited) modules:
Since my failing tests disable their modules, they should simplify to the following:
In that case, the problem seems to be caused by merely importing Stylix itself: imports = [inputs.stylix.homeManagerModules.stylix];Importing Stylix itself and enabling it, resolves the error. However, forcefully enabling Stylix is obviously not the intended behaviour. It is possible, that this is some bug in my setup. But without further debugging, I will just blindly assume that this is a Stylix issue, since everything works fine before this PR :) For reference, here are the full error logs: On a different note, I noticed that the proposal itself has not been discussed 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 declaration forgot the Adding the |
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The individual modules are already gated by
stylix.enable, so there is no need for it to also affect the value ofstylix.targets.«name».enable.This should not produce any noticeable change in behavior.
Note: I have not tested this yet.