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

fix(nixos): sddm package not being installed #194

Merged
merged 2 commits into from
May 28, 2024

Conversation

Weathercold
Copy link
Contributor

Never use attrset merge operator with mk* attrsets since the right hand attrset overrides the left

Fixes #192

Never use attrset merge operator with `mk*` attrsets since the right hand attrset overrides the left
@Weathercold
Copy link
Contributor Author

It seems that aef5672 tries to install the theme package for all versions, but set the theme only if >= 24.05. However, since the options are only made accessible if >= 24.05, there's no way to enable sddm theming at all on older versions, so I changed the module to not enable config on older versions

@@ -13,12 +13,16 @@ let
versionAtLeast
;
cfg = config.services.displayManager.sddm.catppuccin;
enable = cfg.enable && config.services.displayManager.sddm.enable;
enable =
versionAtLeast ctp.getModuleRelease minVersion
Copy link
Contributor

@vdbe vdbe May 24, 2024

Choose a reason for hiding this comment

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

Why is this extra version check needed? You already can't enable cfg on versions under minVersion.

Maybe it's better to place the mkVersionedOpts on options.services.displayManager.sddm.catppuccin so you get this error
image
instead of this nothing happening or this error without the extra check
image

when trying to enable services.displayManager.sddm with a version under minVersion

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't test it, but theoretically, without the version check, enable depends on config.services.displayManager.sddm.catppuccin even when it's not defined and not enabled

Copy link
Member

@getchoo getchoo left a comment

Choose a reason for hiding this comment

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

lgtm. sorry for the wait and thank you!

@getchoo getchoo merged commit 144b70d into catppuccin:main May 28, 2024
6 checks passed
@Weathercold Weathercold deleted the fix-sddm branch July 25, 2024 05:13
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.

SDDM Themeing broke
3 participants