nixos/light: add control options #59742
Conversation
ghost
requested a review
from 
| ${light} -O | ||
| ''; | ||
|
|
||
| systemd.services.light = { |
There was a problem hiding this comment.
This should not be necessary, as systemd already handles this.
There was a problem hiding this comment.
| services.actkbd = mkIf cfg.mediaKeys.enable { | ||
| enable = true; | ||
| bindings = [ | ||
| { keys = [ 224 ]; events = [ "key" "rep" ]; command = "${light} -U ${toString cfg.mediaKeys.step}"; } |
There was a problem hiding this comment.
I'm not sure we actually want to include these sorts of options in NixOS as they increase evaluation time for each user, and are not very complicated. I would prefer we add documentation to the wiki on how to do this.
There was a problem hiding this comment.
Or maybe this is better fit for NUR
There was a problem hiding this comment.
@grahamc What's the purpose of light module then? Also there is an inconsistency for laptop users. We have sound.mediaKeys but have no brightness keys control.
There was a problem hiding this comment.
Right. The PR adding that also received the same feedback after it had been merged, and it wasn't removed to avoid breaking users who had already enabled it. The purpose is for setting up the udev rules and whatnot that the module already does.
nixos/modules/programs/light.nix
Outdated
| @@ -4,6 +4,7 @@ with lib; | |||
|
|
|||
| let | |||
| cfg = config.programs.light; | |||
| light = if cfg.devicePath != null then "${pkgs.light}/bin/light -s ${cfg.devicePath}" else "${pkgs.light}/bin/light"; | |||
There was a problem hiding this comment.
I'd do something like the following (untested):
light = "${pkgs.light}/bin/light" + optionalString (cfg.devicePath != null) " -s ${cfg.devicePath}";
|
This should not be necessary, as systemd already handles this.
This has the advantage of not needing systemd, being configurable (instead of using some magic clamping), and setting brightness even before systemd starts, which seems like a nice addition to me.
Though, the problem in NixOS is that with systemd, systemd will override the backlight anyway, so I agree that either this piece of code does not belong to NixOS, or, alternatively, this should disable `systemd-backlight` when enabled.
I'm not sure we actually want to include these sorts of options in NixOS as they increase evaluation time for each user, and are not very complicated. I would prefer we add documentation to the wiki on how to do this.
By that logic NixOS should not have any optional services in `baseModules` (which was discussed multiple times before and rejected). They might not be very complicated, but I imagine that enabling one declarative option as opposed to copy-pasting the same code between configs is the reason NixOS exists.
My only other nitpick is the same as the one by @Synthetica9 above.
Otherwise, this LGTM.
|
Right. You can disable clamping with
I would disable |
|
Overall, these changes seem more suited for either documentation or NUR as @infinisil suggested. |
|
The saving / restoring GC code doesn't belong, as systemd handles that for us. The device path seem reasonable in the context of keybindings, but keybindings enshrined in the module also seems off-topic for NixOS's services. Defining keybindings in the core of NixOS is essentially building a desktop environment in our services. Our services should be lighter and more generic. These settings should be in documentation, examples, or NUR. |
|
The saving / restoring GC code doesn't belong, as systemd handles that for us.
I don't see how that applies to this module. systemd has timesync or whatever, but we have ntp and chrony modules, systemd has resolved or whatever, but we still have dhcpcd and dnsmasq, it has networkd but we have scripted networking.
This provides an alternative to systemd backlight without non-configurable defaults given by the God himself, which is the whole point.
The device path seem reasonable in the context of keybindings, but keybindings enshrined in the module also seems off-topic for NixOS's services.
Defining keybindings in the core of NixOS is essentially building a desktop environment in our services. Our services should be lighter and more generic. These settings should be in documentation, examples, or NUR.
By that logic you should remove a lot of NixOS modules, e.g, from the top of my head ./nixos/modules/services/web-servers/lighttpd/cgit.nix enshrines cgit URLs, most modules enshrine common /var and /run paths, we enshrine most uids and usernames, etc etc.
I don't see any technical arguments against enshrining good defaults if the actual value does not really matter or you can configure and/or disable them easily, which, btw, this module does and systemd builtin does not.
@gnidorah
I plan to adopt this to SLNOS notchb branch anyways, so do not get discouraged. If you finish it, I'll take it (and if you don't, I'll take it and finish it myself later).
|
Please let me know what should I fix there. |
|
Please let me know what should I fix there.
I still think systemd unit needs to be disabled. But it seems this is not trivial and requires some hacking in nixos/modules/system/boot/systemd.nix (which I already did and will PR soon).
|
|
Systemd module change: #59827.
The polish of this built on top of the systemd change:
https://github.com/oxij/nixpkgs/commits/pull/59742-light-backlight
oxij@54cfd0b
IMHO the complexity of it warrants it to be merged to NixOS despite whatever anyone else says, making it work properly without conflicts and race conditions was quite a pain in the ass because everyone and their grandmother wrestles for backlight control on Linux+systemd. Good luck describing that in a wiki page without using exactly the code from my branch.
The end result in awesome, though. Finally, a proper backlight control via intel_backlight (i.e. properly granular, not just 15 ACPI steps) everywhere, including the linux ttys.
|
Motivation for this change
As discussed in #57602 and #58609
cc @primeos @oxij in case you're interested
Things done
sandboxinnix.confon non-NixOS)nix-shell -p nix-review --run "nix-review wip"./result/bin/)nix path-info -Sbefore and after)