illum: init at 0.4 #22175

dancek · 2017-01-26T14:08:09Z

Motivation for this change

There seemed to be no easy/automatic solution in NixOS to get the screen brightness adjustment keys working. At least in the case that BIOS doesn't handle them and you run a simplistic system without KDE/GNOME/XFCE. Illum is a simple daemon that listens to brightness keys and adjusts the screen backlight. Probably useful for other people, too.

Tested by running nix-build -A 'config.systemd.units."illum.service".unit', copying the resulting service definition to /run/systemd/system and running the service.

Things done

Tested using sandboxing
(nix.useSandbox on NixOS,
or option build-use-sandbox in nix.conf
on non-NixOS)
Built on platform(s)
- NixOS
- macOS
- Linux
Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
Tested execution of all binary files (usually in ./result/bin/)
Fits CONTRIBUTING.md.

mention-bot · 2017-01-26T14:08:13Z

@dancek, thanks for your PR! By analyzing the history of the files in this pull request, we identified @edolstra, @offlinehacker and @bjornfor to be potential reviewers.

dancek · 2017-01-26T19:35:15Z

Apparently set -eu -o pipefail fails on the shell Travis CI is building with. Looks like my /bin/sh is actually bash (I'm used to having a more lightweight shell for scripts), so probably the build will also fail on my machine after I set nix.useSandbox.

I'll try to make the build use bash explicitly if I can figure out how.

dancek · 2017-01-26T19:52:39Z

Strange. I enabled useSandbox and ran nixos-rebuild switch, but the build still works for me.

dancek · 2017-01-26T20:07:20Z

This version uses bash to build, I think. Not completely sure how buildInputs work.

globin · 2017-01-26T22:58:09Z

nixos/modules/services/hardware/illum.nix

+
+  config = mkIf cfg.enable {
+
+    environment.systemPackages = [ pkgs.illum ];


This probably shouldn't be necessary? Or do you have to map your keys to use an "illum client"?

I thought that's needed to mark the dependency on the illum package.

There's no need to map keys or anything like that. Illum just works, but there will probably be some configurable options in the future.

No the serviceConfig.ExecStart = "${pkgs.illum}/bin/illum-d"; is enough for that, this would just add the binaries to the path for all users.

dancek · 2017-02-04T12:20:45Z

It's been a while, but here's yet another version that explicitly uses bash ./configure to avoid the set -o pipefail error with a POSIX shell.

I removed the unnecessary environment.systemPackages definition too.

joachifm · 2017-02-04T14:11:37Z

pkgs/tools/system/illum/default.nix

+    sha256 = "05v3hz7n6b1mlhc6zqijblh1vpl0ja1y8y0lafw7mjdz03wxhfdb";
+  };
+
+  buildInputs = [ bash pkgconfig ninja libevdev libev ];


bash is already part of stdenv

joachifm · 2017-02-04T14:12:19Z

nixos/modules/services/hardware/illum.nix

+    services.illum = {
+
+      enable = mkOption {
+        default = false;


Please specify type = types.bool here; there's no inference right now.

joachifm · 2017-02-04T14:12:51Z

nixos/modules/services/hardware/illum.nix

+  config = mkIf cfg.enable {
+
+    systemd.services.illum = {
+      description = "illum";


Please flesh out the description a little bit.

joachifm · 2017-02-04T14:13:18Z

nixos/modules/services/hardware/illum.nix

+
+    systemd.services.illum = {
+      description = "illum";
+      after = [ "basic.target" ];


after = [ "basic.target" ] is strictly unnecessary; it is the systemd default.

joachifm · 2017-02-04T14:15:14Z

pkgs/tools/system/illum/default.nix

+  buildInputs = [ bash pkgconfig ninja libevdev libev ];
+
+  configurePhase = ''
+    bash ./configure


stdenv.shell can be safely assumed to be bash.

Earlier Travis builds failed on the line set -eu -o pipefail of the configure script, claiming pipefail is an invalid option. Pipefail is a bash extension, been around since bash 3 (2004). Therefore I assumed Travis builds with a POSIX shell (dash or such).

Is there a way to make Travis builds succeed while not explicitly running with bash?

Hm, I see. Sounds like an impurity issue (if travis runs on ubuntu, /bin/sh is dash, I think). You could either patch the shebang of configure (e.g., using the patchShebangs helper in preConfigure) or leave it as is; bash ought to be available in the build environment without adding it to the build inputs, at any rate.

I'll go with bash ./configure then. The script incorrectly specifies #!/bin/sh when it actually needs bash, so in my understanding patchShebangs might not suffice there.

dancek · 2017-02-04T17:26:52Z

@joachifm thanks for the review. Here's another version with the requested changes implemented.

I assume the Travis CI build will fail again because of set -o pipefail now that I removed the explicit bash call.

dancek · 2017-02-04T18:23:50Z

I changed back to bash ./configure to make the Travis build work. Sorry for clobbering the build queue with my tiny edits!

dancek · 2017-02-05T10:59:51Z

The build was successful and the requested changes have been implemented. Waiting for someone to merge this (or request other changes).

joachifm · 2017-02-05T15:41:36Z

Thank you

dancek force-pushed the illum branch from c8a1841 to 2d44a27 Compare January 26, 2017 20:02

globin reviewed Jan 26, 2017

View reviewed changes

dancek force-pushed the illum branch from 2d44a27 to 3ea3e77 Compare February 4, 2017 12:19

joachifm requested changes Feb 4, 2017

View reviewed changes

dancek force-pushed the illum branch from 3ea3e77 to 8a9e6ab Compare February 4, 2017 17:22

illum: init at 0.4

d91b39b

dancek force-pushed the illum branch from 8a9e6ab to d91b39b Compare February 4, 2017 18:23

joachifm merged commit 4459f26 into NixOS:master Feb 5, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

illum: init at 0.4 #22175

illum: init at 0.4 #22175

dancek commented Jan 26, 2017 •

edited

mention-bot commented Jan 26, 2017

dancek commented Jan 26, 2017

dancek commented Jan 26, 2017

dancek commented Jan 26, 2017

globin Jan 26, 2017

dancek Jan 27, 2017

globin Jan 27, 2017

dancek commented Feb 4, 2017

joachifm Feb 4, 2017

joachifm Feb 4, 2017

joachifm Feb 4, 2017

joachifm Feb 4, 2017

joachifm Feb 4, 2017

dancek Feb 4, 2017

joachifm Feb 4, 2017

dancek Feb 4, 2017

dancek commented Feb 4, 2017

dancek commented Feb 4, 2017

dancek commented Feb 5, 2017

joachifm commented Feb 5, 2017


		config = mkIf cfg.enable {

		environment.systemPackages = [ pkgs.illum ];

illum: init at 0.4 #22175

illum: init at 0.4 #22175

Conversation

dancek commented Jan 26, 2017 • edited

Motivation for this change

Things done

mention-bot commented Jan 26, 2017

dancek commented Jan 26, 2017

dancek commented Jan 26, 2017

dancek commented Jan 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dancek commented Feb 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dancek commented Feb 4, 2017

dancek commented Feb 4, 2017

dancek commented Feb 5, 2017

joachifm commented Feb 5, 2017

dancek commented Jan 26, 2017 •

edited