xow: init at 0.2 #76185

pmiddend · 2019-12-22T20:13:21Z

Motivation for this change

I have first tested xow on my local machine and it worked flawlessly. Then I created the package and my first NixOS service with it. I tested this service using a VM and it worked.

Things done

nixos/modules/services/hardware/xow.nix

pkgs/misc/drivers/xow/default.nix

pmiddend · 2019-12-25T09:21:38Z

Thanks for the thorough review @infinisil!

nixos/modules/services/hardware/xow.nix

pkgs/misc/drivers/xow/default.nix

nixos/modules/services/hardware/uinput.nix

jtojnar · 2019-12-26T05:04:06Z

pkgs/misc/drivers/xow/default.nix

+    sha256 = "03ajal91xi52svzy621aa4jcdf0vj4pqd52kljam0wryrlmcpbr3";
+  };
+
+  makeFlags = "BUILD=RELEASE";


Suggested change

makeFlags = "BUILD=RELEASE";

makeFlags = [

"INSTALL_PATH=${placeholder ''out''}/bin"

"SERVICE_PATH=${placeholder ''out''}/lib/systemd/user"

"BUILD=RELEASE"

];

will allow you to use the default installPhase.

Sadly, not really:

cp: cannot create regular file '/nix/store/jd1mrvsb21pcxn4s1x6681rkypybj455-xow-0.2/lib/systemd/user': No such file or directory

I'm not sure about fixing this, though. Should the xow author do this in the Makefile?

Apparently, it is not trying to create the dirctetory. You will also need to add mkdir -p $out/lib/systemd/user into preInstall.

Hmm, apparently, the install target also tries to enable and start the service, which is an incredibly bad practice:

https://github.com/medusalix/xow/blob/897eee07351f88a04eb16a02b8bca0b91f91cad4/Makefile#L32-L33

Hi, developer here. Could you please explain why this is a bad practice?

Thanks for the explanation. I've fixed some of the issues in the install branch, though I'm still unsure about the udev rules. We've basically got three options:

Make uinput and USB devices r/w for all users (current option, similar to ds4drv).

Make uinput and USB devices r/w for current user (udev tag uaccess). What do we do in the service file? Only specify a single user?

Make uinput and USB devices r/w for a single group (could be named xow).

What do you think? What's the best option (both for NixOS and users building from source)?

I am not a udev expert but I have seen uaccess recommended over GROUP and MODE in other PRs. Though apparently, the filename must be ordered before 73-seat-late.rules according to systemd/systemd#4288 (comment). As for the service, would it be possible to make it a user service? That way, it would already run under plugdev group (I understand is required for udev access).

As far as I understand, with user services we would get a service instance of xow for each user session that is currently active. That's quite problematic because only one instance can have the USB device open at the same time. I'm now using systemd's DynamicUser (which should be safer than running as root), but that didn't work with uaccess.

Would SupplementaryGroups=plugdev work? Otherwise, I would try writing to systemd ML.

It certainly works, but it's limited to Ubuntu and Debian and is deprecated according to systemd.

jtojnar · 2019-12-26T05:07:27Z

pkgs/misc/drivers/xow/default.nix

+    rev = "v${version}";
+    sha256 = "03ajal91xi52svzy621aa4jcdf0vj4pqd52kljam0wryrlmcpbr3";
+  };
+


In postPatch we should fix the path in the service file:
https://github.com/medusalix/xow/blob/897eee07351f88a04eb16a02b8bca0b91f91cad4/xow.service#L5

Also, an issue should be opened upstream.

What's a better path for the executable in the service file in this case?

The path should be based on the INSTALL_PATH passed to make, instead of the hardcoded /usr/local/bin.

You mean make should change the service file to adapt the path and install that to the proper location?

Yes. This is usually done by having foo.service: foo.service.in target and creating foo.service from foo.service.in by replacing the template values. Though, usually, the value to replace is set in configure step with Autotools.

Okay. Instead of patching this systemd and Makefile file in lots of places, I'd like to open an issue upstream and see if we can get the whole file + installation process fixed for the next xow release.

Any improvements to the build process are welcome, though I'd like to keep the Makefile as simple as possible (part of the reason I didn't consider Autotools).

pkgs/misc/drivers/xow/default.nix

jtojnar · 2019-12-26T05:13:27Z

nixos/modules/services/hardware/xow.nix

+    services.hardware.uinput.enable = true;
+
+    systemd.services.xow = {
+      wantedBy = [ "multi-user.target" ];


You should use the upstream service definition via systemd.packages and only override those values that are wrong/missing.

nixos/modules/services/hardware/xow.nix

pkgs/misc/drivers/xow/default.nix

medusalix · 2019-12-29T23:06:29Z

Please put this issue on hold until I've figured out the licensing issues (medusalix/xow#15). I forgot to add the necessary disclaimer for Mediatek's firmware that's included with xow, so we'll have to wait and see if Mediatek approves its distribution. If they do, then this package will probably end up in the unfree section of nixpkgs.

medusalix · 2020-01-23T22:38:41Z

Since I didn't hear back from Mediatek (sent multiple mails to different people), I guess they are fine with the current license terms.

@pmiddend Please take a look at the changes to the Makefile in xow v0.3. I've extracted some of the hardcoded paths to variables which should make the packaging much easier for you.

infinisil

This looks good to me

infinisil · 2020-02-16T02:15:11Z

@jtojnar Looking good for you too?

jtojnar

xow 0.3 was released which sets the bindir in the service file correctly. We should update to that and drop our service settings.

jansol · 2020-03-29T15:20:13Z

@pmiddend Do you have time to update this PR so we can get it merged? If not and you don't mind, I can take over.

pmiddend · 2020-03-29T15:36:16Z

@jansol Sorry, xow sadly isn't even something I need anymore. If you like, please take over.

jansol · 2020-03-30T01:11:50Z

Continuing work in #83732, this PR can be closed.

pmiddend requested a review from infinisil as a code owner December 22, 2019 20:13

ofborg bot added 6.topic: nixos 8.has: module (update) labels Dec 22, 2019

pmiddend force-pushed the xow-init branch from e49238d to 92d4c37 Compare December 22, 2019 20:28

ofborg bot added 8.has: package (new) 11.by: package-maintainer 10.rebuild-darwin: 0 10.rebuild-linux: 1-10 labels Dec 22, 2019

ehmry approved these changes Dec 23, 2019

View reviewed changes

infinisil requested changes Dec 24, 2019

View reviewed changes

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

pkgs/misc/drivers/xow/default.nix Outdated Show resolved Hide resolved

pkgs/misc/drivers/xow/default.nix Outdated Show resolved Hide resolved

pmiddend force-pushed the xow-init branch from 92d4c37 to 95dab20 Compare December 25, 2019 09:18

pmiddend force-pushed the xow-init branch from 95dab20 to 1f7f3cc Compare December 25, 2019 12:36

infinisil requested changes Dec 25, 2019

View reviewed changes

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

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

pkgs/misc/drivers/xow/default.nix Outdated Show resolved Hide resolved

pmiddend force-pushed the xow-init branch from 1f7f3cc to 2f54a70 Compare December 25, 2019 18:15

infinisil requested changes Dec 25, 2019

View reviewed changes

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

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

jtojnar requested changes Dec 26, 2019

View reviewed changes

pmiddend force-pushed the xow-init branch 2 times, most recently from 65909b1 to 5359329 Compare December 28, 2019 09:09

xow: init at 0.2

0f64213

pmiddend force-pushed the xow-init branch from 5359329 to 0f64213 Compare December 28, 2019 09:17

infinisil approved these changes Jan 24, 2020

View reviewed changes

jtojnar requested changes Feb 16, 2020

View reviewed changes

jansol mentioned this pull request Mar 30, 2020

xow: init at 0.4 #83732

Merged

10 tasks

jtojnar closed this Mar 30, 2020

pmiddend deleted the xow-init branch March 30, 2020 06:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

xow: init at 0.2 #76185

xow: init at 0.2 #76185

pmiddend commented Dec 22, 2019

pmiddend commented Dec 25, 2019

jtojnar Dec 26, 2019

pmiddend Dec 28, 2019

jtojnar Dec 28, 2019

jtojnar Dec 28, 2019

medusalix Dec 29, 2019

medusalix Jan 3, 2020 •

edited

Loading

jtojnar Jan 3, 2020

medusalix Jan 3, 2020 •

edited

Loading

jtojnar Jan 4, 2020

medusalix Jan 4, 2020

jtojnar Dec 26, 2019

pmiddend Dec 26, 2019

jtojnar Dec 26, 2019

pmiddend Dec 27, 2019

jtojnar Dec 27, 2019 •

edited

Loading

pmiddend Dec 28, 2019

medusalix Dec 29, 2019

jtojnar Dec 26, 2019

medusalix commented Dec 29, 2019

medusalix commented Jan 23, 2020

infinisil left a comment

infinisil commented Feb 16, 2020

jtojnar left a comment

jansol commented Mar 29, 2020

pmiddend commented Mar 29, 2020

jansol commented Mar 30, 2020

-  makeFlags = "BUILD=RELEASE";
+  makeFlags = [
+    "INSTALL_PATH=${placeholder ''out''}/bin"
+    "SERVICE_PATH=${placeholder ''out''}/lib/systemd/user"
+    "BUILD=RELEASE"
+  ];

xow: init at 0.2 #76185

xow: init at 0.2 #76185

Conversation

pmiddend commented Dec 22, 2019

Motivation for this change

Things done

pmiddend commented Dec 25, 2019

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

medusalix Jan 3, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

medusalix Jan 3, 2020 • 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

jtojnar Dec 27, 2019 • 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

medusalix commented Dec 29, 2019

medusalix commented Jan 23, 2020

infinisil left a comment

Choose a reason for hiding this comment

infinisil commented Feb 16, 2020

jtojnar left a comment

Choose a reason for hiding this comment

jansol commented Mar 29, 2020

pmiddend commented Mar 29, 2020

jansol commented Mar 30, 2020

medusalix Jan 3, 2020 •

edited

Loading

medusalix Jan 3, 2020 •

edited

Loading

jtojnar Dec 27, 2019 •

edited

Loading