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

nixos/dockd: add new service #49734

Closed
wants to merge 5 commits into from
Closed

Conversation

cyraxjoe
Copy link
Contributor

@cyraxjoe cyraxjoe commented Nov 4, 2018

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
Copy link
Member

FRidh commented Nov 4, 2018

@GrahamcOfBorg eval

'';
};

dockedConfPath = mkOption {
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your comment.

About the configs... dockd has two config files

  1. 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)

  2. 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:

  1. 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.

  2. 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.

Copy link
Member

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

Missing argument?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

😅 yeah

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

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

@roberth roberth left a comment

Choose a reason for hiding this comment

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

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.

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"
Copy link
Member

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@cyraxjoe cyraxjoe Nov 19, 2018

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

# 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;
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

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

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@cyraxjoe
Copy link
Contributor Author

@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
Copy link
Member

roberth commented Nov 20, 2018

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
Copy link
Member

mmahut commented Aug 11, 2019

Are there any updates on this pull request, please?

@cyraxjoe
Copy link
Contributor Author

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
Copy link
Member

roberth commented Nov 25, 2019

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
Copy link

stale bot commented Jun 1, 2020

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:

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

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 1, 2020
@cyraxjoe cyraxjoe closed this Sep 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 6.topic: nixos 8.has: documentation 8.has: module (update)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants