danth / stylix

System-wide colorscheming and typography for NixOS
https://stylix.danth.me/
MIT License
1.21k stars 155 forks source link

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

Open sodiboo opened 2 months ago

sodiboo commented 2 months ago

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 commented 2 months ago

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.

-- https://github.com/danth/stylix/issues/542#issuecomment-2323405533

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 commented 2 months ago

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

danth commented 2 months ago

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).