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

Jump to bottom

brightnessctl: Add systemd support #79663

Merged
merged 2 commits into from Feb 13, 2020
Merged

brightnessctl: Add systemd support #79663

merged 2 commits into from Feb 13, 2020

Conversation

primeos
Copy link
Member

@primeos primeos commented Feb 9, 2020

This makes it possible to use brightnessctl without udev rules / suid.
I've used this on my laptop for quite some time now (but there wasn't a stable release until now) and didn't have any issues.

This should still be compatible with the existing NixOS module (but I've never used that - could somebody confirm this?) but now one can also just install brightnessctl and use it without any extra configuration (on a systemd based system).

We could either enable this by default and still support the udev rules (like in this PR), drop the udev rules / NixOS module, or add a "use flag" but don't enable this by default (less recommended).

cc @megheaiulian @dje4321 @globin

Motivation for this change
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@primeos
brightnessctl: Add systemd support
cf5dd26 
This makes it possible to use brightnessctl without udev rules / suid.
@flokli
Copy link
Contributor

flokli commented Feb 9, 2020

@primeos quickly skimming through brightnessctl.c, building with systemd support will change the method that's used to write from writing to the file enabled access for via the udev rule to logind_set_brightness instead.

I don't really see the point adding the udev files as well. The services.udev.package mechanism only works with systemd in NixOS, so we can assume logind to be installed as well.

The NixOS module is only adding it brightnessctl to environment.systemPackages and to services.udev.package.

As the latter isn't needed anymore, I don't see much value in keeping the module - We could just notify users they only need to add the package, as logind takes care of permissions.

@ofborg ofborg bot requested a review from megheaiulian February 9, 2020 23:32
@primeos
nixos/brightnessctl: Remove the module
5282bc9 
Due to the support of the systemd-logind API the udev rules aren't
required anymore which renders this module useless [0].
Note: brightnessctl should now require a working D-Bus setup and a valid
local logind session for this to work.

[0]: NixOS#79663
@primeos
Copy link
Member Author

primeos commented Feb 10, 2020 •
edited

@flokli since the systemd-logind API is used as a fallback option it could make a difference in some special cases (e.g. $DBUS_SESSION_BUS_ADDRESS not set, via SSH, not a valid local logind session - but I haven't testet this and this wouldn't be a normal setup). But in normal cases it should work as expected, so it should be fine I guess.

I've added a commit to remove the module (or should we deprecate it more gracefully with only a warning for now?).

@ofborg ofborg bot requested a review from megheaiulian February 10, 2020 23:10
@flokli flokli merged commit aaa1c7b into NixOS:master Feb 13, 2020
dtzWill pushed a commit to dtzWill/nixpkgs that referenced this pull request Feb 14, 2020
@flokli @dtzWill
Merge pull request NixOS#79663 from primeos/brightnessctl-systemd-sup…
02ab2a5 
…port

brightnessctl: Add systemd support
(cherry picked from commit aaa1c7b)
primeos added a commit to primeos/nixpkgs that referenced this pull request Feb 15, 2020
@primeos
nixos/brightnessctl: Remove the module
e8042ed 
Due to the support of the systemd-logind API the udev rules aren't
required anymore which renders this module useless [0].
Note: brightnessctl should now require a working D-Bus setup and a valid
local logind session for this to work.

[0]: NixOS#79663

(cherry picked from commit 5282bc9)
@primeos primeos mentioned this pull request Feb 15, 2020
Merged
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@megheaiulian megheaiulian megheaiulian approved these changes

@infinisil infinisil Awaiting requested review from infinisil

Assignees
No one assigned
Labels
6.topic: nixos 8.has: module (update) 10.rebuild-darwin: 0 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@primeos @flokli @megheaiulian