nixos/logiops: init #287399
nixos/logiops: init #287399
Conversation
kylechui
force-pushed
the
feat/add-logiops
branch
from
0fb058e
to
4cfbc6e
Compare
|
Ref #167388 |
kylechui
force-pushed
the
feat/add-logiops
branch
2 times, most recently
from
8f3bdff
to
900394b
Compare
|
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:
Usually, I'd encourage to write a nixos test, but since this is hardware dependent it might be a bit difficult. |
|
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. |
There was a problem hiding this comment.
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)
|
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 |
|
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:
Good luck! I'll try to stick around for the review but I have less time for nixpkgs at the moment. |
|
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. |
kylechui
force-pushed
the
feat/add-logiops
branch
from
626dfc3
to
7246e13
Compare
|
The commit title is wrong, should be 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 |
kylechui
force-pushed
the
feat/add-logiops
branch
3 times, most recently
from
4ed9525
to
2d5937c
Compare
|
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 |
| # 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" |
There was a problem hiding this comment.
| 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" |
There was a problem hiding this comment.
Is this so the udev rule will (potentially) use an overridden version of systemd?
| after = [ "multi-user.target" ]; | ||
| wants = [ "multi-user.target" ]; |
There was a problem hiding this comment.
Is there a specific reason why we need this? Usually I thought wantedBy multi-user.target is enough.
There was a problem hiding this comment.
It is used in the original package, see this file
| Also see <https://github.com/PixlOne/logiops/blob/main/logid.example.cfg> for an example config. | ||
| ''; | ||
| default = { }; | ||
| example = { |
There was a problem hiding this comment.
This is way to big to properly render on search.nixos.org
kylechui
force-pushed
the
feat/add-logiops
branch
from
ea0c356
to
4f249fd
Compare
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`.
kylechui
force-pushed
the
feat/add-logiops
branch
from
4f249fd
to
14d265d
Compare
Description of changes
Logiopsis third-party software for configuring Logitech mice, e.g remapping buttons or scroll behavior. It is already packaged innixpkgs, but there is nosystemdservice 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
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.