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

all modules are enabled by default, and they don't just style my configuration #543

Open
sodiboo opened this issue Sep 1, 2024 · 3 comments
Labels
technical debt Things which should be cleaned up or improved

Comments

@sodiboo
Copy link
Contributor

sodiboo commented Sep 1, 2024

I noticed this after hyprpaper stopped building and breaks my configuration (see #542). But the thing is, i don't use hyprpaper or hyprland. I use niri and swaybg. hyprpaper shouldn't be in my system closure at all, my system configuration has zero matches for hypr. but the stylix hyprland module always sets services.hyprpaper.enable = true: unless i opt out by disabling that target or setting stylix.autoEnable = false;. it would make sense if hyprland was actually enabled, to also enable hyprpaper for the theming. but i don't use hyprland, this shouldn't be enabled by default.

for a lot of options like dunst this is perfectly okay, because all it does is set some dunst settings. these don't do anything by themselves other than possibly generating a config file. but the Hyprland module (and likely a few others) do additional stuff, that actually increase the size of my system closure.

it would make a lot of sense if the standard lib.mkIf prelude in stylix would check for the relevant program being enabled in general. e.g. services.hyprpaper.enable = true; should be gated behind a mkIf wayland.windowManager.hyprland.enable. if i understand it right, it would probably be easiest and cleanest to do this in the autoEnable parameter of mkEnableTarget, simply passing the relevant enable gate rather than unconditionally setting that to true.

this change should really be unconditionally part of all modules, really rather than just those that look to "have side-effects", because truth is, even if they are just setting the options, that might have the side effect of causing a file to be generated that otherwise wouldn't be. this happens for example in my niri module (which isn't part off this repo, but would fit right in as far as i'm concerned) and isn't a huge issue since it's "just" the config file, but it goes against the general philosophy of nix and ultimately contributes to added evaluation time and closure sizes, even for modules you never enabled or even knew existed!

@trueNAHO trueNAHO added the technical debt Things which should be cleaned up or improved label Sep 1, 2024
@trueNAHO
Copy link
Collaborator

trueNAHO commented Sep 1, 2024

I noticed this after hyprpaper stopped building and breaks my configuration (see #542). But the thing is, i don't use hyprpaper or hyprland. I use niri and swaybg. hyprpaper shouldn't be in my system closure at all, my system configuration has zero matches for hypr. but the stylix hyprland module always sets services.hyprpaper.enable = true: unless i opt out by disabling that target or setting stylix.autoEnable = false;. it would make sense if hyprland was actually enabled, to also enable hyprpaper for the theming. but i don't use hyprland, this shouldn't be enabled by default.

There is already a pending PR for this:

As if hyprpaper is enabled by default. The work around is disabling the hyperland target all together with home-manager.users.user.stylix.targets.hyprland.enable = false;.

#501 should resolve this.

-- #542 (comment)

it would make a lot of sense if the standard lib.mkIf prelude in stylix would check for the relevant program being enabled in general. e.g. services.hyprpaper.enable = true; should be gated behind a mkIf wayland.windowManager.hyprland.enable. if i understand it right, it would probably be easiest and cleanest to do this in the autoEnable parameter of mkEnableTarget, simply passing the relevant enable gate rather than unconditionally setting that to true.

AFAIK, the Nix convention is that <MODULE>.<OPTION> has no effect unless <MODULE>.enable is true. Any exceptions are likely bugs or outdated practices.

this change should really be unconditionally part of all modules, really rather than just those that look to "have side-effects", because truth is, even if they are just setting the options, that might have the side effect of causing a file to be generated that otherwise wouldn't be. this happens for example in my niri module (which isn't part off this repo, but would fit right in as far as i'm concerned) and isn't a huge issue since it's "just" the config file, but it goes against the general philosophy of nix and ultimately contributes to added evaluation time and closure sizes, even for modules you never enabled or even knew existed!

Unless upstream (NixOS or Home Manager) refuses to address this, it might be better to solve this problem generally.

I will try to resolve these issues upstream when cleaning up the code base. Let me know if you find any more impure Stylix modules.

@danth
Copy link
Owner

danth commented Sep 2, 2024

#544 will stop Hyprpaper being installed unless you're actually using Hyprland.

@danth
Copy link
Owner

danth commented Sep 2, 2024

To keep track of side effects, we could build the testbed configurations with and without Stylix, and use a tool like nvd to see what Stylix installed. This could be a script to run locally, or it could be part of CI (and possibly post a comment on the PR if something changed).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
technical debt Things which should be cleaned up or improved
Projects
None yet
Development

No branches or pull requests

3 participants