nixos/dockd: add new service #49734

cyraxjoe · 2018-11-04T09:52:34Z

Motivation for this change

Adds the dockd service for nixos and all the dependencies, libthinkpad and the daemon itself "dockd".

Dockd is a dock management daemon for the Lenovo Thinkpads, very helpful if you are running minimal wm, like i3. It allows you to configure for example, a set of multiple monitors when you dock you laptop.

https://github.com/libthinkpad/dockd

Things done

Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
Built on platform(s)
- [X ] 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"
[X ] 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.

FRidh · 2018-11-04T10:04:22Z

@GrahamcOfBorg eval

roberth · 2018-11-04T11:29:53Z

nixos/modules/services/hardware/dockd.nix

+      '';
+    };
+
+    dockedConfPath = mkOption {


I appreciate the effort you took to make this configurable, but I don't think it's necessary to maintain this solution. Most services simply define a configuration in etc.

Of course your solution is more powerful, because it allows the possibility to have a mutable config file, but is that worth the overhead of a different solution?

I may well be wrong, because I don't know anything about operating dockd.

Thank you for your comment.

About the configs... dockd has two config files

The docked.conf this file has to be writable to be set with dockd --config docked (executed by the user running under the graphical session), this command captures the current monitor/screens configuration in xrandr and create an ini file with those values. (on a predefined location hard coded in the executable)

The undocked.conf this file has to be writable to be set with dockd --config undocked (executed by the user running under the graphical session), this command captures the current monitor/screens configuration in xrandr and create an ini file with those values (on a predefined location hard coded in the executable). Basically this is the same as (1) but the result will be written into another path.

And two hooks/executables:

The dock.hook script/executable, it will be executed when the laptop gets removed from the dock, you can put any arbitrary command in this file.

The undock.hook script/executable, it will be executed when the laptop gets removed from the dock, you can put any arbitrary command in this file.

I'm not familiar with the options to manage an etc file.. but that seems like a potential good solution, the only problem is that, if you want to change the configuration (dockd --config docked|undocked) the user would need to copy those new values into the configuration.nix otherwise the changes will be overridden on the next nixos-rebuild.

So it's more of configuration management tool. That makes sense. Could you add this information in the option descriptions?

roberth · 2018-11-04T19:08:12Z

nixos/modules/services/hardware/dockd.nix

+     pkgs.dockd.override {
+       inherit (cfg) dockedConfPath undockedConfPath;
+       dockHookPath = (if cfg.dockHook != ""
+                       then pkgs.writeScript "dock.hook"


Missing argument?

roberth · 2018-11-04T19:10:33Z

nixos/modules/services/hardware/dockd.nix

+                         then pkgs.writeScript "undock.hook" cfg.undockHook
+                         else cfg.undockHookPath);
+     };
+  in mkIf cfg.enable {


Could you add two assertions that check that only one of undockHook and undockHookPath (and some for dock) is set?

Oops, intended to add the link https://nixos.org/nixos/manual/index.html#sec-assertions-assertions

roberth

I really appreciate the effort you put into improving the module, but it seems that some things are a bit more complicated than necessary now.

It'd be really nice if you could explain the two modes of operation (mutable/immutable) for this module and what the implications are.

roberth · 2018-11-19T12:22:54Z

nixos/modules/services/hardware/dockd.nix

+        Path to the executable to run when the ThinkPad is placed on the dock.
+        If set, make sure the file exists and is executable by the user of the session.
+
+        Example: "/etc/dockd/dock.hook"


For examples, you can use a mkOption argument attribute like example = "/etc/dockd/dock.hook";

roberth · 2018-11-19T12:26:34Z

nixos/modules/services/hardware/dockd.nix

+          undockHook = if cfg.undockHook == null then "exit 0" else cfg.undockHook;
+          inherit(pkgs) writeScript;
+        in
+        pkgs.dockd.override {


Is it necessary to build the package differently based on configuration? This is generally not needed and will cause user systems to download the build dependencies and build the package unnecessarily.

The problem is that the real fix needs to be addressed in the upstream project. The path to the executable is hard-coded in the cpp source code https://github.com/libthinkpad/dockd/blob/master/hooks.cpp. And the hack that this package does is to patch the source code to replace the hard-coded path to the one specified in the dockd package. The "improvement" is that if you want, you can have a script also in the nix store or in any other place in the system not just /etc/dockd/{dock,undock}.hook.

I though the patch was to replace it with getenv but I must have been thinking about another package. Sorry about that. Using getenv would be an improvement, but considering the relative importance and size of dockd, I think your solution is already sufficient.

Using getenv might be a better idea 🤔 . I'll consider it this weekend. Thanks again.

roberth · 2018-11-19T12:32:52Z

nixos/modules/services/hardware/dockd.nix

+        # set to null, specified that the user is not interested on doing nothing
+        # with the hooks
+        let
+          dockHook = if cfg.dockHook == null then "exit 0" else cfg.dockHook;


The empty string is a perfectly fine script, equivalent to exit 0, so you don't need to do this.

This also means you can simplify the type to disallow null again.

The need to have null there as a valid option, was because I wanted to distinguish between an explicit null to intent "I don't care" and a string. But yes, I can just remove the possibility for null. Originally I added that because if I didn't provide a default it would cause some issues on my if expressions when deciding on using the xHookPath vs xHook. Issues about the attribute not existing, even if the assertions that I made should have "prevented" that situation, but apparently the evaluation of the asserts are not done prior the full evaluation.... Anyway, I agree I can make it simpler.

roberth · 2018-11-19T12:36:38Z

nixos/modules/services/hardware/dockd.nix

+      # verify that only one of (dockHook | dockHookPath) is defined
+      {
+        assertion =
+         if ((cfg.dockHook != null && cfg.dockHook != "") && (cfg.dockHookPath != ""))


This can be simplified to assertion = dockHook == "" || dockHookPath == ""; (same for undock)
(removing null as suggested and simplify)

This was the original approach, but changed because of the idea of null intending "I don't care" and a string that you want to evaluate this as a script. I can make as it was before.

roberth · 2018-11-19T12:39:21Z

nixos/modules/services/hardware/dockd.nix

+      {
+        assertion =
+          if cfg.dockHookPath != "" then
+          (builtins.pathExists cfg.dockHookPath) else true;


builtins.pathExists will check the path on the machine that evaluates the configuration, which may not be the machine that runs the configuration.
This would break for example a setup where a company has CI for its fleet of laptops. Also, it doesn't work in restrict-eval enabled.

🤔 yeah, I never considered that when I added this assertion.

cyraxjoe · 2018-11-19T20:37:09Z

@roberth I can simplify the expression, but getting back to the idea of adding dockd in nixos, do you think is a good idea to add a project that is maybe rather immature, in terms of the hard-code of paths and maybe the quality of the project. Also, maybe this is not that relevant to a lot of people to put it directly in nixos, I could just add this in some other repo and to be used by those who really want to have this sort of thing going on. 🤔 . Also thanks for the review, you've been very kind and helpful.

roberth · 2018-11-20T16:11:47Z

Thanks :)
Since we don't have a standard solution for sharing packages outside the nixpkgs repo, I do think nixpkgs may be a good place for this. However, if you feel like trying out NUR and it satisfies your needs, you could go with that. Discoverability of dockd will be worse, so you'll need to do some promotion.

mmahut · 2019-08-11T06:09:27Z

Are there any updates on this pull request, please?

cyraxjoe · 2019-11-21T22:14:19Z

Are there any updates on this pull request, please?

@mmahut I'm no longer using NixOS in the particular thinkpad where dockd was relevant (using it along i3wm).

Also the upstream dockd project it no very well maintained, that's why it kind of discouraged me to keep working on this PR and officially include a package that would turn abandon-ware very quickly.

Still, I can put the work on a additional overlay to be included by those who don't really care about anything and just enjoy the pleasures of automatically adjust into the additional monitors when is docked. With enough 👏s and 🎉s I can make it happen.

@roberth would it be better if I just close this PR? To at least make it -1 in the number of open PRs?

roberth · 2019-11-25T18:30:17Z

Hey @cyraxjoe,
Kinda sad story 😕; it looks like you put some of your best effort into the module but it was a bit complicated.
I don't think we have a policy about PR closing yet, so I'd say it's up to you. If you think you'll spend time on this, leave it open, but by the looks of it you can close it. If you leave the branch on your repo it will still be usable here, even if closed.
I'll mention this on #nixos in case someone wants to pick it up from here.

stale · 2020-06-01T03:24:20Z

Thank you for your contributions.
This has been automatically marked as stale because it has had no activity for 180 days.
If this is still important to you, we ask that you leave a comment below. Your comment can be as simple as "still important to me". This lets people see that at least one person still cares about this. Someone will have to do this at most twice a year if there is no other activity.
Here are suggestions that might help resolve this more quickly:

Search for maintainers and people that previously touched the
related code and @ mention them in a comment.
Ask on the NixOS Discourse. 3. Ask on the #nixos channel on
irc.freenode.net.

cyraxjoe added 4 commits November 4, 2018 02:07

Add me (cyraxjoe) into the maintainers list

4c8b9c6

libthinkpad: init at 2.6

730e679

dockd: init at 1.21

0ca909c

nixos/dockd: new service

69b0699

GrahamcOfBorg added 6.topic: nixos 8.has: documentation 8.has: module (update) labels Nov 4, 2018

roberth reviewed Nov 4, 2018

View reviewed changes

Add assertions for the new nixos/dockd service.

07a4b64

cyraxjoe requested a review from infinisil as a code owner November 19, 2018 01:40

roberth requested changes Nov 19, 2018

View reviewed changes

stale bot added the 2.status: stale label Jun 1, 2020

cyraxjoe closed this Sep 3, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

nixos/dockd: add new service #49734

nixos/dockd: add new service #49734

cyraxjoe commented Nov 4, 2018

FRidh commented Nov 4, 2018

roberth Nov 4, 2018

cyraxjoe Nov 4, 2018

roberth Nov 4, 2018

roberth Nov 4, 2018

cyraxjoe Nov 4, 2018

roberth Nov 4, 2018

roberth Nov 4, 2018

roberth left a comment

roberth Nov 19, 2018

roberth Nov 19, 2018

cyraxjoe Nov 19, 2018 •

edited

Loading

roberth Nov 20, 2018

cyraxjoe Nov 20, 2018

roberth Nov 19, 2018

roberth Nov 19, 2018

cyraxjoe Nov 19, 2018

roberth Nov 19, 2018

cyraxjoe Nov 19, 2018

roberth Nov 19, 2018

cyraxjoe Nov 19, 2018

cyraxjoe commented Nov 19, 2018

roberth commented Nov 20, 2018

mmahut commented Aug 11, 2019

cyraxjoe commented Nov 21, 2019

roberth commented Nov 25, 2019

stale bot commented Jun 1, 2020

nixos/dockd: add new service #49734

nixos/dockd: add new service #49734

Conversation

cyraxjoe commented Nov 4, 2018

Motivation for this change

Things done

FRidh commented Nov 4, 2018

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

roberth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyraxjoe Nov 19, 2018 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyraxjoe commented Nov 19, 2018

roberth commented Nov 20, 2018

mmahut commented Aug 11, 2019

cyraxjoe commented Nov 21, 2019

roberth commented Nov 25, 2019

stale bot commented Jun 1, 2020

cyraxjoe Nov 19, 2018 •

edited

Loading