Skip to content

stylix: guard home-manager-integration config#1494

Merged
trueNAHO merged 1 commit intonix-community:masterfrom
Flameopathic:guard-base16Scheme-check
Jul 1, 2025
Merged

stylix: guard home-manager-integration config#1494
trueNAHO merged 1 commit intonix-community:masterfrom
Flameopathic:guard-base16Scheme-check

Conversation

@Flameopathic
Copy link
Copy Markdown
Contributor

@Flameopathic Flameopathic commented Jun 13, 2025

Fixes #1492

Things done

Notify maintainers

@MattSturgeon

@Flameopathic Flameopathic force-pushed the guard-base16Scheme-check branch from c1dcda6 to 7a71572 Compare June 13, 2025 12:42
@MattSturgeon
Copy link
Copy Markdown
Member

MattSturgeon commented Jun 13, 2025

I'm not sure if it is acceptable practice to use lib.mkIf on an option default. I will revise as requested.

You can't do it as part of the option declaration, however you can do it by moving the default definition from options to config:

config.stylix.base16Scheme = lib.mkIf (...) (lib.mkOptionDefault (...));

That said, this doesn't solve the issue. With this approach you would get option `stylix.base16Scheme` used but not defined errors because the underlying issue isn't in the option's value, but the fact that its value is read when it shouldn't be.

See also #1492 (comment)

@Flameopathic Flameopathic marked this pull request as draft June 13, 2025 13:32
@Flameopathic Flameopathic force-pushed the guard-base16Scheme-check branch from 7a71572 to 3a146f3 Compare June 14, 2025 11:29
@Flameopathic Flameopathic changed the title stylix: disable base16Scheme check with Stylix stylix: guard homeManagerIntegration.followSystem with cfg.enable Jun 14, 2025
@Flameopathic Flameopathic marked this pull request as ready for review June 14, 2025 13:57
@Flameopathic
Copy link
Copy Markdown
Contributor Author

CC @danth

Comment thread stylix/home-manager-integration.nix Outdated
@Flameopathic Flameopathic changed the title stylix: guard homeManagerIntegration.followSystem with cfg.enable stylix: default homeManagerIntegration.followSystem to cfg.enable Jun 20, 2025
@Flameopathic Flameopathic force-pushed the guard-base16Scheme-check branch from 3a146f3 to 531ddc9 Compare June 20, 2025 22:23
Copy link
Copy Markdown
Contributor

@0xda157 0xda157 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Copy Markdown
Member

@trueNAHO trueNAHO left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The patch LGTM.

@Keyruu and @paholg, could you test whether this PR resolves #1492?

For reference, I have successfully tested that this PR still evaluates my standalone Home Manager setup:

Tested-by: NAHO <90870942+trueNAHO@users.noreply.github.com>

I still feel like the correct thing to do is avoid evaluating stylix.base16Scheme when stylix.enable = false,1 however IDK how big of a treewide cleanup this will be. Such a change would (of course) be easier if we added a regression test.

-- #1492 (comment)

Yes, improving the test suite is a slow but ongoing process 1 2. Although this part of the codebase could indeed be polished, this is surprisingly low priority and should probably just continue receiving hotfixes for the foreseeable future, until it inevitably needs to be rewritten to continue making progress on other more "important" tasks.

Footnotes

  1. Ideally, none of stylix's option values would be evaluated when stylix.enable = false

@trueNAHO trueNAHO requested a review from danth June 23, 2025 15:09
@MattSturgeon
Copy link
Copy Markdown
Member

MattSturgeon commented Jun 23, 2025

Ideally, none of stylix's option values would be evaluated when stylix.enable = false

If so, then this PR doesn't fully address the issue. Changing a default is not sufficient to ensure stylix.enable = false always disables the feature.

Copy link
Copy Markdown
Contributor

@0xda157 0xda157 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally, none of stylix's option values would be evaluated when stylix.enable = false

In that case config should be guarded by mkIf config.stylix.enable

@Flameopathic Flameopathic force-pushed the guard-base16Scheme-check branch from 531ddc9 to a9e6c7f Compare June 24, 2025 11:34
@Flameopathic Flameopathic changed the title stylix: default homeManagerIntegration.followSystem to cfg.enable stylix: guard home-manager-integration config Jun 24, 2025
Copy link
Copy Markdown
Member

@trueNAHO trueNAHO left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Let's wait a bit if the bug reporters also test this PR.

@trueNAHO
Copy link
Copy Markdown
Member

I managed to get my [previous] configuration to work with the Stylix revision in the pull request by importing the home-manager module via home-manager.sharedModules, as suggested, and removing the global module.

-- alex-massa, #1492 (comment)

This PR has been successfully tested by alex-massa:

Tested-by: https://github.com/alex-massa

Let's wait a bit if the bug reporters also test this PR.

I suggest merging this PR on Monday if the bug reporters have still not replied until then, giving them 7 days to reply. For reference, #1492 has been open for two weeks and has currently 7 upvotes.

@Flameopathic Flameopathic force-pushed the guard-base16Scheme-check branch from a9e6c7f to 342c4a2 Compare June 29, 2025 15:41
@stylix-automation stylix-automation bot added the topic: documentation Documentation additions or improvements label Jun 29, 2025
@Flameopathic
Copy link
Copy Markdown
Contributor Author

I have slightly updated documentation to reflect this change.

@trueNAHO trueNAHO changed the title stylix: guard home-manager-integration config stylix: guard home-manager-integration config Jul 1, 2025
@trueNAHO trueNAHO merged commit 73b7d0f into nix-community:master Jul 1, 2025
7 checks passed
stylix-automation bot pushed a commit that referenced this pull request Jul 1, 2025
Closes: #1492
Link: #1494

Reviewed-by: Matt Sturgeon <matt@sturgeon.me.uk>
Reviewed-by: awwpotato <awwpotato@voidq.com>
Tested-by: https://github.com/alex-massa
Reviewed-by: NAHO <90870942+trueNAHO@users.noreply.github.com>
Tested-by: NAHO <90870942+trueNAHO@users.noreply.github.com>
(cherry picked from commit 73b7d0f)
@stylix-automation
Copy link
Copy Markdown
Contributor

Successfully created backport PR for release-25.05:

trueNAHO pushed a commit that referenced this pull request Jul 1, 2025
Closes: #1492
Link: #1494

Reviewed-by: Matt Sturgeon <matt@sturgeon.me.uk>
Reviewed-by: awwpotato <awwpotato@voidq.com>
Tested-by: https://github.com/alex-massa
Reviewed-by: NAHO <90870942+trueNAHO@users.noreply.github.com>
Tested-by: NAHO <90870942+trueNAHO@users.noreply.github.com>
(cherry picked from commit 73b7d0f)
LunaCOLON3 pushed a commit to LunaCOLON3/stylix that referenced this pull request Jul 14, 2025
Closes: nix-community#1492
Link: nix-community#1494

Reviewed-by: Matt Sturgeon <matt@sturgeon.me.uk>
Reviewed-by: awwpotato <awwpotato@voidq.com>
Tested-by: https://github.com/alex-massa
Reviewed-by: NAHO <90870942+trueNAHO@users.noreply.github.com>
Tested-by: NAHO <90870942+trueNAHO@users.noreply.github.com>
trueNAHO added a commit to trueNAHO/stylix that referenced this pull request Aug 21, 2025
Do not guard on stylix.enable because the original issue from commit
73b7d0f ("stylix: guard home-manager-integration config (nix-community#1494)")
seems to have resolved itself in the meantime.

This allows testbeds to declare Stylix target options when Stylix is
disabled.

Reverts: 73b7d0f ("stylix: guard home-manager-integration config (nix-community#1494)")
@Flameopathic Flameopathic deleted the guard-base16Scheme-check branch September 24, 2025 13:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

topic: documentation Documentation additions or improvements

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Broken when stylix.image is null

4 participants