Skip to content

brightnessctl: init at 0.3.2#42102

Merged
matthewbauer merged 2 commits intoNixOS:masterfrom
dje4321:brightnessctl
Jun 16, 2018
Merged

brightnessctl: init at 0.3.2#42102
matthewbauer merged 2 commits intoNixOS:masterfrom
dje4321:brightnessctl

Conversation

@dje4321
Copy link
Contributor

@dje4321 dje4321 commented Jun 16, 2018 •
edited
Loading

Motivation for this change

Adding brightnessctl for better backlight support. brightnessctl package is what provides the binary and the brightnessctl module enables the udev rule for access from userspace for anyone in the video group.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • 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 execution of all binary files (usually in ./result/bin/)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Fits CONTRIBUTING.md.

@dje4321
Copy link
Contributor Author

dje4321 commented Jun 16, 2018

probably isnt perfect but ill appreciate the feedback on ways to improve it. First time making a package for nixos and first time making a PR.

@grahamc thanks for the help earlier. I know what i did wrong. checked out the local nixpkg version for my system instead of doing it for master. its what i get for following the manual to close but not close enough

@GrahamcOfBorg GrahamcOfBorg added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Jun 16, 2018
matthewbauer
matthewbauer suggested changes Jun 16, 2018
View reviewed changes
pkgs/misc/brightnessctl/default.nix Outdated
substituteInPlace 90-brightnessctl.rules --replace %k '*'
'';

installPhase = ''
Copy link
Member

@matthewbauer matthewbauer Jun 16, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should be able to reuse the install from the Makefile with makeFlags = "PREFIX=/ DESTDIR=$(out)";. This is a little bit better practice especially for when renames, new files, etc are introduced.

pkgs/misc/brightnessctl/default.nix

meta = {
homepage = "https://github.com/Hummer12007/brightnessctl";
license = stdenv.lib.licenses.mit;
Copy link
Member

@matthewbauer matthewbauer Jun 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add yourself as a maintainer here?

Copy link
Contributor Author

@dje4321 dje4321 Jun 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the feedback. Ill work on getting them implemented.

pkgs/misc/brightnessctl/default.nix Outdated
name = "brightnessctl-${version}";
version = "0.3.2";

src = fetchurl {
Copy link
Member

@matthewbauer matthewbauer Jun 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You may want to use fetchFromGitHub here. IMO it makes things more readable.

dje4321
dje4321 commented Jun 16, 2018
View reviewed changes
Copy link
Contributor Author

@dje4321 dje4321 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes Submitted

@matthewbauer matthewbauer merged commit 1b7ce4c into NixOS:master Jun 16, 2018
@puffnfresh
Copy link
Member

puffnfresh commented Jun 18, 2018 •
edited
Loading

The executable under bin doesn't have executable permission.

@cleverca22
Copy link
Contributor

cleverca22 commented Aug 10, 2018

and the udev rules appear to be missing from $out

@nixos-discourse
Copy link

nixos-discourse commented Mar 24, 2020

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/new-package-that-manipulates-udev/6391/2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@matthewbauer matthewbauer matthewbauer requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@dje4321 @puffnfresh @cleverca22 @nixos-discourse @matthewbauer @GrahamcOfBorg