nixos/logiops: init #287399

kylechui · 2024-02-09T06:40:24Z

Description of changes

Logiops is third-party software for configuring Logitech mice, e.g remapping buttons or scroll behavior. It is already packaged in nixpkgs, but there is no systemd service associated with it.

Closes #226575

Note: I'm not quite sure how to write tests for this service, so any help in that department would be much appreciated!

Things done

Add a 👍 reaction to pull requests you find important.

nixos/modules/services/hardware/logiops.nix

h7x4 · 2024-02-10T13:32:36Z

Ref #167388

nixos/modules/services/hardware/logiops.nix

kylechui · 2024-02-11T01:12:11Z

What would be the most idiomatic way to test my fork using my NixOS machine (which uses a flake)? Would it be to add my fork as an upstream flake input, or perhaps rebuild and point to my local copy of nixpkgs somehow?

h7x4 · 2024-02-11T01:53:17Z

What would be the most idiomatic way to test my fork using my NixOS machine (which uses a flake)? Would it be to add my fork as an upstream flake input, or perhaps rebuild and point to my local copy of nixpkgs somehow?

I'm not entirely sure. I've had a bit of trouble testing PRs with modules myself, without ruining my local setup. AFAIK, there are at least these alternatives:

include the changes to your config locally, and override with package overlays and disabledModules.
(arguably the best option, but takes a bit to set up, especially considering this is a hardware module) create a nixos configuration with the settings you need, and spin it up in a QEMU VM with nixos-rebuild build-vm.
Set up a test machine or a permanent test VM.
override your local nixpkgs to the unstable fork - be very careful with this, as it might run migrations and do irreversible changes to the state of the system.

Usually, I'd encourage to write a nixos test, but since this is hardware dependent it might be a bit difficult.

kylechui · 2024-02-18T09:19:31Z

Some of the changes in here only make sense with respect to #289701.

P.S. I've set up a test VM, but have yet to figure out how to perform PCIe passthrough in order to fully test the behavior of the mouse on the VM. I've been able to observe the configuration getting created in the right location though.

ckiee

Hi,

I'm rather confused by why you branched off from keyd for this instead of continuing the previous PR? @h7x4 and I had a whole saga implementing the libconfig serializer which makes me feel a bit bad about this, as it seems you didn't look at the history of this much at all?

You're finishing it up which is great for others; but I wish you'd use some of the code I've been running for almost 3 years now (#167388). It's proven to be very stable. (Last commit)

nixos/modules/services/hardware/logiops.nix

kylechui · 2024-02-19T00:22:45Z

Hey @ckiee, I didn't mean any offense by implementing this code myself. If your PR has been working well, then should that be marked as non-draft and merged into nixpkgs instead (i.e. close this one)? I'm afraid I'm a bit confused as to what the current state of things is, or exactly which parts of your code you "wish I could've used" in this PR.

Edit: I was roughly following the keyd PR because (in my mind), both pieces of software seemed roughly similar (i.e. consisting of some application binary relating to hardware + some systemd service). Thus I wanted to try emulating it via some services.logiops.enable setting.

ckiee · 2024-02-19T00:32:23Z

Yeah thank you for the response, I'm sorry about my initial hostility.

I meant it'd have been nice if you'd continued off from where I stopped with my PR's code saving us some review time here (and making me happy to see my code get used after all) but it doesn't matter too much now since you've already done a pretty good job with this new PR which I assume works. It's just a bit of a shame for my end, but it is better to get you happy & contributing to nixpkgs so I'd be glad if you went ahead.

A few more notes:

PR title should be nixos/logiops: init
Commits should be rebased and formatted as in CONTRIBUTING.md and my PR can serve as a reference too.

Good luck! I'll try to stick around for the review but I have less time for nixpkgs at the moment.

ckiee · 2024-02-19T00:40:17Z

Oh also, for the tests I think just making sure the service starts up is enough. We don't do any such thing for nvidia/… afaik.

nixos/modules/services/hardware/logiops.nix

ckiee · 2024-02-22T15:20:53Z

The commit title is wrong, should be nixos/logiops: init too. And you were right about example, it doesn't need pkgs., sorry about that.

weird udev rule aside (probably ok) and lack of testing on my machine it looks pretty good. i have other matters to attend to though, so ping me in a while if you haven't found another reviewer. cc @h7x4

nixos-discourse · 2024-02-24T19:26:32Z

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review/3032/3509

SuperSandro2000 · 2024-02-25T17:42:55Z

nixos/modules/services/hardware/logiops.nix

+    # Add a `udev` rule to restart `logiops` when the mouse is connected
+    # https://github.com/PixlOne/logiops/issues/239#issuecomment-1044122412
+    services.udev.extraRules = ''
+      ACTION=="add", SUBSYSTEM=="input", ATTRS{id/vendor}=="046d", RUN{program}="${pkgs.systemd}/bin/systemctl --no-block try-restart logiops.service"


Suggested change

ACTION=="add", SUBSYSTEM=="input", ATTRS{id/vendor}=="046d", RUN{program}="${pkgs.systemd}/bin/systemctl --no-block try-restart logiops.service"

ACTION=="add", SUBSYSTEM=="input", ATTRS{id/vendor}=="046d", RUN{program}="${config.systemd.package}/bin/systemctl --no-block try-restart logiops.service"

Is this so the udev rule will (potentially) use an overridden version of systemd?

SuperSandro2000 · 2024-02-25T17:44:54Z

nixos/modules/services/hardware/logiops.nix

+      after = [ "multi-user.target" ];
+      wants = [ "multi-user.target" ];


Is there a specific reason why we need this? Usually I thought wantedBy multi-user.target is enough.

It is used in the original package, see this file

nixos/modules/services/hardware/logiops.nix

SuperSandro2000 · 2024-02-25T17:45:18Z

nixos/modules/services/hardware/logiops.nix

+        Also see <https://github.com/PixlOne/logiops/blob/main/logid.example.cfg> for an example config.
+      '';
+      default = { };
+      example = {


This is way to big to properly render on search.nixos.org

nixos/modules/services/hardware/logiops.nix

The logiops package already exists, but without a systemd service. refactor: Use `udev` to restart service when mouse connected. Old method used a somewhat jank `modprobe` to load the `hid_logitech_hidpp` kernel module. fix: Remove `IPAddressDeny` from service. It is redundant since we already set `PrivateNetwork = true`. fix: Allow service access to all of `/dev`. If the user has many devices connected, they may run into issues with the `DeviceAllow` permission. Ideally, we would only allow access to `/dev/uinput` and `/dev/hidraw*`, but globbing is unavailable for device node path specifications: https://www.freedesktop.org/software/systemd/man/252/systemd.resource-control.html refactor: Use `libconfig` generator. fix: Format type should be `libconfig`. fix: Incorrect way to add a file to `/etc`. fix: DeviceAllow permissions. `hidraw` is a character device in `/proc/devices`, so we should be able to match it in `DeviceAllow`. Additionally, matching against `/dev` doesn't do anything, as device node path specifications need to be fully specified. feat: Allow access to `AF_UNIX`. This is needed since the newer version of `logiops` requires GDBus. See PR#289701. fix: Update service restart condition. Sometimes the service will error out and fail to apply the user's configuration if the mouse is disconnected and then reconnected. The updated `udev` rule restarts the service any time a Logitech device is connected. feat: Add example config. fix: systemd wantedBy option. See: https://github.com/NixOS/nixpkgs/pull/287399/files/15700feac734cbacb3fb9dd0f5cbfc04a27058d9#r1493869013 feat: Allow package override. fix: Use `literalExpression` for example package. fix: Stop using `literalExpression` for example. test: Add test to see if service starts properly. fix: Revert `pkgs.` prefix for example. fix: Only restart the service if it is already running. refactor: Use overridden `systemd`.

github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: documentation This PR adds or changes documentation 8.has: changelog 8.has: module (update) This PR changes an existing module in `nixos/` labels Feb 9, 2024

kylechui marked this pull request as ready for review February 9, 2024 06:41

NixOSInfra added the 12. first-time contribution This PR is the author's first one; please be gentle! label Feb 9, 2024

kylechui commented Feb 9, 2024

View reviewed changes

nixos/modules/services/hardware/logiops.nix Outdated Show resolved Hide resolved

ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Feb 9, 2024

kylechui force-pushed the feat/add-logiops branch from 0fb058e to 4cfbc6e Compare February 9, 2024 07:18

kylechui changed the title ~~feat: Add logiops service.~~ logiops: Add logiops service Feb 9, 2024

h7x4 reviewed Feb 10, 2024

View reviewed changes

nixos/modules/services/hardware/logiops.nix Outdated Show resolved Hide resolved

h7x4 added the 8.has: module (new) This PR adds a module in `nixos/` label Feb 10, 2024

h7x4 reviewed Feb 10, 2024

View reviewed changes

nixos/modules/services/hardware/logiops.nix Outdated Show resolved Hide resolved

h7x4 reviewed Feb 10, 2024

View reviewed changes

nixos/modules/services/hardware/logiops.nix Show resolved Hide resolved

h7x4 reviewed Feb 10, 2024

View reviewed changes

nixos/modules/services/hardware/logiops.nix Outdated Show resolved Hide resolved

h7x4 reviewed Feb 10, 2024

View reviewed changes

nixos/modules/services/hardware/logiops.nix Outdated Show resolved Hide resolved

kylechui force-pushed the feat/add-logiops branch 2 times, most recently from 8f3bdff to 900394b Compare February 10, 2024 23:17

kylechui commented Feb 11, 2024

View reviewed changes

nixos/modules/services/hardware/logiops.nix Outdated Show resolved Hide resolved

ckiee suggested changes Feb 18, 2024

View reviewed changes

nixos/modules/services/hardware/logiops.nix Outdated Show resolved Hide resolved

nixos/modules/services/hardware/logiops.nix Outdated Show resolved Hide resolved

nixos/modules/services/hardware/logiops.nix Outdated Show resolved Hide resolved

ckiee mentioned this pull request Feb 18, 2024

logiops: 0.2.3 -> 0.3.3 #289701

Merged

13 tasks

kylechui changed the title ~~logiops: Add logiops service~~ nixos/logiops: init Feb 19, 2024

ckiee reviewed Feb 20, 2024

View reviewed changes

nixos/modules/services/hardware/logiops.nix Outdated Show resolved Hide resolved

ckiee reviewed Feb 20, 2024

View reviewed changes

nixos/modules/services/hardware/logiops.nix Show resolved Hide resolved

kylechui force-pushed the feat/add-logiops branch from 626dfc3 to 7246e13 Compare February 21, 2024 02:16

kylechui requested review from ckiee and h7x4 February 21, 2024 02:24

kylechui force-pushed the feat/add-logiops branch 3 times, most recently from 4ed9525 to 2d5937c Compare February 22, 2024 19:40

SuperSandro2000 reviewed Feb 25, 2024

View reviewed changes

kylechui force-pushed the feat/add-logiops branch from ea0c356 to 4f249fd Compare March 3, 2024 19:31

kylechui force-pushed the feat/add-logiops branch from 4f249fd to 14d265d Compare March 3, 2024 19:39

wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Apr 5, 2024

wegank added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jan 2, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

nixos/logiops: init #287399

nixos/logiops: init #287399

kylechui commented Feb 9, 2024 •

edited

Loading

h7x4 commented Feb 10, 2024

kylechui commented Feb 11, 2024

h7x4 commented Feb 11, 2024

kylechui commented Feb 18, 2024

ckiee left a comment

kylechui commented Feb 19, 2024 •

edited

Loading

ckiee commented Feb 19, 2024

ckiee commented Feb 19, 2024

ckiee commented Feb 22, 2024

nixos-discourse commented Feb 24, 2024

SuperSandro2000 Feb 25, 2024

kylechui Feb 25, 2024 •

edited

Loading

SuperSandro2000 Feb 25, 2024

kylechui Feb 25, 2024

SuperSandro2000 Feb 25, 2024

	ACTION=="add", SUBSYSTEM=="input", ATTRS{id/vendor}=="046d", RUN{program}="${pkgs.systemd}/bin/systemctl --no-block try-restart logiops.service"
	ACTION=="add", SUBSYSTEM=="input", ATTRS{id/vendor}=="046d", RUN{program}="${config.systemd.package}/bin/systemctl --no-block try-restart logiops.service"

		after = [ "multi-user.target" ];
		wants = [ "multi-user.target" ];

nixos/logiops: init #287399

Are you sure you want to change the base?

nixos/logiops: init #287399

Conversation

kylechui commented Feb 9, 2024 • edited Loading

Description of changes

Things done

h7x4 commented Feb 10, 2024

kylechui commented Feb 11, 2024

h7x4 commented Feb 11, 2024

kylechui commented Feb 18, 2024

ckiee left a comment

Choose a reason for hiding this comment

kylechui commented Feb 19, 2024 • edited Loading

ckiee commented Feb 19, 2024

ckiee commented Feb 19, 2024

ckiee commented Feb 22, 2024

nixos-discourse commented Feb 24, 2024

SuperSandro2000 Feb 25, 2024

Choose a reason for hiding this comment

kylechui Feb 25, 2024 • edited Loading

Choose a reason for hiding this comment

SuperSandro2000 Feb 25, 2024

Choose a reason for hiding this comment

kylechui Feb 25, 2024

Choose a reason for hiding this comment

SuperSandro2000 Feb 25, 2024

Choose a reason for hiding this comment

kylechui commented Feb 9, 2024 •

edited

Loading

kylechui commented Feb 19, 2024 •

edited

Loading

kylechui Feb 25, 2024 •

edited

Loading