Skip to content

nixos/logiops init#124158

Closed
zeorin wants to merge 3 commits intoNixOS:masterfrom
zeorin:feature/logiops-init
Closed

nixos/logiops init#124158
zeorin wants to merge 3 commits intoNixOS:masterfrom
zeorin:feature/logiops-init

Conversation

@zeorin
Copy link
Copy Markdown
Contributor

@zeorin zeorin commented May 23, 2021

Motivation for this change

logiops was requested in #122208.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • 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 nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Added a release notes entry if the change is major or breaking
  • Fits CONTRIBUTING.md.
To do

The service definition doesn't really specify any options, other than enable and extraConfig.

@hawkw, you mentioned that you'd started on a config for this. Would you like to contribute that to this module?

There's also the accepted settings RFC to consider. I think that the libconfig-style configs would lend themselves well to this approach, but there doesn't seem to be a parser yet for this in nixpkgs. I had started on this approach before just using extraConfig. The code for that is still in the file, commented out. This should be removed before submitting the PR.

@github-actions github-actions bot 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/` labels May 23, 2021
@zeorin zeorin force-pushed the feature/logiops-init branch from 6bc88d7 to 1c3ebc2 Compare May 23, 2021 19:18
zeorin added 2 commits May 23, 2021 21:21
Package requested in NixOS#122208
Service definition for logiops, which was requested in NixOS#122208.
@zeorin zeorin force-pushed the feature/logiops-init branch from 1c3ebc2 to 15d7e69 Compare May 23, 2021 19:21
Copy link
Copy Markdown
Member

@aanderse aanderse left a comment

Choose a reason for hiding this comment

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

Looking good. Just a few minor points you might want to consider.

Restart = "on-failure";
};
wantedBy = [ "multi-user.target" ];
restartTriggers = [ pkgs.logiops ];
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is redundant. Maybe you wanted cfg.extraConfig here, but I'm not sure.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Is it redundant because I'm already referencing the package in the ExecStart line?

Restarting on cfg.extraConfig is a good idea, as a user I would expect changes to the config to take effect once I've rebuilt NixOS (currently it doesn't).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Exactly! You understand perfectly now 👍

};

config = mkIf cfg.enable {
systemd.services.logiops = {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think you should reuse the upstream systemd unit and then customize it. If you're not familiar how to do that please feel free to ask questions and I can go over in detail. From a high level you just add the package to systemd.packages and that will include any units in the resulting system. From there you can add customizations as needed. Be sure to keep the wantedBy directive because NixOS can't parse that out of upstream units.

You'll also want to patch the location of /usr/bin/kill in the upstream unit.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I must admit I haven't the foggiest clue how I would reuse the upstream systemd unit and then customize it, but I agree that it is a better approach. I am very keen to learn, though!

Initially, my derivation wouldn't build because the upstream unit file installation failed, so I looked around nixpkgs for what other packages had done and found this: https://github.com/NixOS/nixpkgs/blob/85784a66c8848de93c6ee3d0fb47ec425491f404/pkgs/servers/monitoring/icinga2/no-systemd-service.patch, and I copied that approach. Specifically, when I don't apply my CMake patch, the error I get is:

…
[ 93%] Building CXX object src/logid/CMakeFiles/logid.dir/util/worker_thread.cpp.o
[ 95%] Building CXX object src/logid/CMakeFiles/logid.dir/util/task.cpp.o
[ 96%] Building CXX object src/logid/CMakeFiles/logid.dir/util/thread.cpp.o
[ 98%] Building CXX object src/logid/CMakeFiles/logid.dir/util/ExceptionHandler.cpp.o
[100%] Linking CXX executable ../../logid
[100%] Built target logid
installing
install flags: SHELL=/nix/store/xvvgw9sb8wk6d2c0j3ybn7sll67s3s4z-bash-4.4-p23/bin/bash install
[100%] Built target logid
Install the project...
-- Install configuration: "Release"
-- Installing: /nix/store/vi59a2gy74cg81vd2smk920jiqz36nvl-logiops-0.2.3/bin/logid
CMake Error at src/logid/cmake_install.cmake:70 (file):
  file cannot create directory:
  /nix/store/71lqc2a8cslg4wxj6ypla7gvflphjhn0-systemd-247.6/lib/systemd/system.
  Maybe need administrative privileges.
Call Stack (most recent call first):
  cmake_install.cmake:47 (include)


make: *** [Makefile:105: install] Error 1
builder for '/nix/store/hjxy35w6xkwb5hivml15y9m7kwng0x90-logiops-0.2.3.drv' failed with exit code 2
cannot build derivation '/nix/store/diz9yg8vxxaisfdisiqxhpm5fazsx8pf-unit-logiops.service.drv': 1 dependencies couldn't be built
cannot build derivation '/nix/store/x88mi1xav2csv6n5nzdf4xwc9hl8gj2f-system-units.drv': 1 dependencies couldn't be built
cannot build derivation '/nix/store/i6s5n1700gcq8xlqxwj57cfabh3h74rn-etc.drv': 1 dependencies couldn't be built
cannot build derivation '/nix/store/67l36cbh1gfxnh2xig67y3v5hmwm37dv-nixos-system-monarch-21.05.2549.a1007637cea.drv': 1 dependencies couldn't be built
error: build of '/nix/store/67l36cbh1gfxnh2xig67y3v5hmwm37dv-nixos-system-monarch-21.05.2549.a1007637cea.drv' failed

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@zeorin this looks like a "bug" (intended feature which was never properly implemented?) from upstream to me:

https://github.com/PixlOne/logiops/blob/6bb470000952405b6817032c43c7215f7d070428/src/logid/CMakeLists.txt#L92-L105

If you set the value of SYSTEMD_SERVICES_INSTALL_DIR then upstream doesn't do install the .service file, which seems wrong.

I would propose that you create a PR upstream to fix this with the following commit: https://github.com/aanderse/logiops/commit/c723d0fb6bcc647ee35fe3eeb728e21ba93e1c90

I tested the above commit with this commit on top of your PR in nixpkgs and it all built as expected with the desired outcome: https://github.com/zeorin/nixpkgs/compare/feature/logiops-init...aanderse:logiops?expand=1

Let me know if you have any questions or need a hand on this.

serviceConfig = {
Type = "simple";
ExecStart = "${pkgs.logiops}/bin/logid";
User = "root";
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hi, I've also been tooling around with writing a service for logiops and I found out how to get around running the service as root. This is my config so far:

services.udev.extraRules = ''
  KERNEL=="uinput", GROUP="input", MODE:="0660", OPTIONS+="static_node=uinput"
  SUBSYSTEM=="hidraw", ATTRS{idVendor}=="046d", MODE="0660", GROUP="input"
'';

And in the service:

DynamicUser = true;
SupplementaryGroups = "input";
NoNewPrivileges = true;
ProtectSystem = "strict";
ProtectHome = true;
ProtectProc = "invisible";
ProcSubset = "pid";
PrivateTmp = true;
PrivateUsers = true;
PrivateMounts = true;
ProtectKernelTunables = true;
ProtectKernelModules = true;
ProtectKernelLogs = true;
ProtectControlGroups = true;
RestrictNamespaces = true;

Which seems to be working, as far as the Systemd restrictions go. Any thoughts?

@rofrol
Copy link
Copy Markdown
Contributor

rofrol commented Nov 9, 2021

Any news on that?

@ckiee ckiee mentioned this pull request Apr 5, 2022
13 tasks
ckiee added a commit to ckiee/nixpkgs that referenced this pull request May 5, 2022
Supersedes NixOS#124158 since it seems dead and things have changed[0].
This is a follow up on my NixOS#165936.

[0] https://github.com/NixOS/rfcs/blob/master/rfcs/0042-config-option.md
@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 19, 2022
@s1341
Copy link
Copy Markdown
Contributor

s1341 commented Jun 29, 2022

I want this too... What is the blocker?

@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 29, 2022
@anund
Copy link
Copy Markdown
Contributor

anund commented Jul 27, 2022

See #165936 for basic packaging. @s1341 #167388 would give you the module as well.

@Janik-Haag Janik-Haag added the 12.first-time contribution This PR is the author's first one; please be gentle! label Jun 12, 2023
@abathur
Copy link
Copy Markdown
Member

abathur commented Aug 27, 2023

Tentatively closing this since it hasn't been updated since the package itself landed in #165936.

Happy to reopen if I'm overstepping. (That said, the draft PR #167388 covers the service/config component of this, and ckiee@094fd26 suggests that there are acute reasons to prefer helping land that approach over reheating this one?)

@abathur abathur closed this Aug 27, 2023
@zeorin zeorin deleted the feature/logiops-init branch October 20, 2023 18:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

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/` 12.first-time contribution This PR is the author's first one; please be gentle!

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants