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

brightnessctl: Add systemd support #79663

Merged
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.
This makes it possible to use brightnessctl without udev rules / suid.
@flokli
Copy link
Contributor

@flokli 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.

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]: #79663
@primeos primeos requested a review from Infinisil as a code owner Feb 10, 2020
@primeos
Copy link
Member Author

@primeos primeos commented Feb 10, 2020

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

@flokli flokli merged commit aaa1c7b into NixOS:master Feb 13, 2020
16 checks passed
16 checks passed
brightnessctl on x86_64-darwin No attempt
Details
Evaluation Performance Report Evaluator Performance Report
Details
brightnessctl on aarch64-linux Success
Details
brightnessctl on x86_64-linux Success
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A darwin-tested
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release-combined.nix -A tested
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A unstable
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
dtzWill added a commit to dtzWill/nixpkgs that referenced this pull request Feb 14, 2020
…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
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
0 of 10 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.