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.

Sign up for GitHub

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

nixos/autorandr: make default target in systemd service configurable #42398

Merged
merged 1 commit into from Jul 5, 2018
Merged

nixos/autorandr: make default target in systemd service configurable #42398

merged 1 commit into from Jul 5, 2018

Conversation

Ma27
Copy link
Member

@Ma27 Ma27 commented Jun 22, 2018 •
edited

Motivation for this change

The .service file defining the systemd unit for autorandr.service
which is bundled with the package itself uses --default default in the
ExecStart section. This can be an issue when having multiple layouts
(e.g. default as workstation layout I mostly work on and mobile when
I go somewhere else).

When the service gets restarted and --default can't be applied,
however the current layout can't be detected (e.g. when working with an
unknown beamer) the service silently fails with a message like this:

Jun 22 18:44:46 hauptshuhle autorandr[3168]: /nix/store/h83b72ffm68nm8fyjnppljchp456a94r-xrandr-1.5.0/bin/xrandr: ca>
Jun 22 18:44:46 hauptshuhle autorandr[3168]: Failed to apply profile 'default' (line 718):
Jun 22 18:44:46 hauptshuhle autorandr[3168]:   Command failed: /nix/store/h83b72ffm68nm8fyjnppljchp456a94r-xrandr-1.>

As discussed in the IRC (see https://botbot.me/freenode/nixos/2018-07-05/?msg=101791455&page=6)
it's a bad long-term solution in terms of maintenance to manually patch
the service file bundled with the derivation, instead the service shall
be configured declaratively. Additionally this makes possible overrides
from the user-space way easier.

The udev rule (in $out/etc/udev/rules.d) won't' be affected, it
simply runs systemctl start autorandr.service when e.g. a new display
is added, so now udev communicates with the NixOS systemd unit.

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 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/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Fits CONTRIBUTING.md.

infinisil
infinisil reviewed Jun 24, 2018
View reviewed changes
nixos/modules/services/misc/autorandr.nix Outdated
@@ -28,7 +38,15 @@ in {
wantedBy = [ "sleep.target" ];
};

nixpkgs.overlays = mkIf (cfg.defaultTarget != null) [
Copy link
Member

Choose a reason for hiding this comment

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

Why an overlay and not a local package override?

Copy link
Member Author

Choose a reason for hiding this comment

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

you're actually right, I thought that it might sense to override the package when the option is set in the module system, but it's actually not needed as it only alters the .service file :)

@Ma27 Ma27 force-pushed the make-autorandr-target-configurable branch from fcd72ba to f4f47e5 Compare June 24, 2018 13:25
@infinisil
Copy link
Member

It would be possible to not have to rebuild autorandr on defaultTarget changes with a wrapper that symlinks everything, except the systemd file, which would be the only one patched. A neat way of doing this is to use symlinkJoin { paths = [ autorandr ]; postBuild = ''wrapProgram ..."; ... }. What do you think of this?

@Ma27
Copy link
Member Author

Ma27 commented Jul 4, 2018

@infinisil ack, goodpoint. I just hacked it together to see if it works (and autorandr doesn't take too long to build),but you're right, this might be a way cleaner solution %)

@Ma27 Ma27 force-pushed the make-autorandr-target-configurable branch from f4f47e5 to 2bda8a3 Compare July 5, 2018 11:43
@Ma27
Copy link
Member Author

Ma27 commented Jul 5, 2018

@infinisil done, would you mind having another look at it? 😄

@Ma27 Ma27 force-pushed the make-autorandr-target-configurable branch from 2bda8a3 to c223475 Compare July 5, 2018 11:47
@Ma27
nixos/autorandr: make default target in systemd service configurable
8325996 
The `.service` file defining the `systemd` unit for `autorandr.service`
which is bundled with the package itself uses `--default default` in the
`ExecStart` section. This can be an issue when having multiple layouts
(e.g. `default` as workstation layout I mostly work on and `mobile` when
I go somewhere else).

When the service gets restarted and `--default` can't be applied,
however the current layout can't be detected (e.g. when working with an
unknown beamer) the service silently fails with a message like this:

```
Jun 22 18:44:46 hauptshuhle autorandr[3168]: /nix/store/h83b72ffm68nm8fyjnppljchp456a94r-xrandr-1.5.0/bin/xrandr: ca>
Jun 22 18:44:46 hauptshuhle autorandr[3168]: Failed to apply profile 'default' (line 718):
Jun 22 18:44:46 hauptshuhle autorandr[3168]:   Command failed: /nix/store/h83b72ffm68nm8fyjnppljchp456a94r-xrandr-1.>
```

As discussed in the IRC (see https://botbot.me/freenode/nixos/2018-07-05/?msg=101791455&page=6)
it's a bad long-term solution in terms of maintenance to manually patch
the service file bundled with the derivation, instead the service shall
be configured declaratively. Additionally this makes possible overrides
from the user-space way easier.

The `udev` rule (in `$out/etc/udev/rules.d`) won't' be affected, it
simply runs `systemctl start autorandr.service` when e.g. a new display
is added, so now `udev` communicates with the NixOS systemd unit.
@Ma27 Ma27 force-pushed the make-autorandr-target-configurable branch from c223475 to 8325996 Compare July 5, 2018 12:43
@Ma27
Copy link
Member Author

Ma27 commented Jul 5, 2018

@infinisil @gramahc I modified the patch as discussed and linked the relevant parts of the IRC logs in the commit message %)

@infinisil
Copy link
Member

Awesome, looks much better, have you tested it?

@Ma27
Copy link
Member Author

Ma27 commented Jul 5, 2018

@infinisil yes I did, checked the resulting .service file and started the service which works fine.
I didn't write a test though as I'm absolutely not sure if there's a reliable way to simulate monitors (and monitor switches) during a testcase in a VM session

@infinisil infinisil merged commit 59b3ce2 into NixOS:master Jul 5, 2018
@infinisil
Copy link
Member

Nice!

@Ma27 Ma27 deleted the make-autorandr-target-configurable branch July 5, 2018 13:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@infinisil infinisil infinisil approved these changes

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

Successfully merging this pull request may close these issues.

None yet
3 participants
@Ma27 @infinisil @GrahamcOfBorg