Skip to content

xppen: init package with nixos module#285660

Closed
ghost wants to merge 18 commits intomasterfrom
unknown repository
Closed

xppen: init package with nixos module#285660
ghost wants to merge 18 commits intomasterfrom
unknown repository

Conversation

@ghost
Copy link

@ghost ghost commented Feb 1, 2024

Description of changes

Added XPPen PenTablet package and a program.xppen module for it

Things done

I've initially made this for my friend who asked me if I could port XPPen on NixOS, I did, so now I'd like to add it to nixpkgs.
I have a few concerns however, the primary is that I'm not quite sure about the licensing of XPPen, there is practically no information I could find and the only way to contact XPPen is through mail which, if I'm completely honest, I just don't want to do.

I also don't own an XPPen tablet and don't really know much about them unlike my friend but he's not quite there yet in terms of being able to package it himself.

Do my changes relate to #213263 and the pentablet-driver package???

Also this is my very first time trying to add a package/module to nixpkgs so please tell me if I'm doing things wrong, I tried my best but I suppose there are things that can be improved.

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.05 Release Notes (or backporting 23.05 and 23.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@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 Feb 1, 2024
@ghost
Copy link
Author

ghost commented Feb 1, 2024

Also please anyone with an actual XPPen tablet tell me if what I've made actually works, I have no means of testing it myself, I just know that the app is running and the udev rules are applied 🙏🙏

@NixOSInfra NixOSInfra added the 12.first-time contribution This PR is the author's first one; please be gentle! label Feb 1, 2024
@h7x4 h7x4 added the 8.has: module (new) This PR adds a module in `nixos/` label Feb 2, 2024
@ghost ghost force-pushed the xppen-pentablet branch from 14a246a to c002eac Compare February 2, 2024 01:11
@ghost
Copy link
Author

ghost commented Feb 2, 2024

I tested the module, looks like I didn't break anything

@ghost
Copy link
Author

ghost commented Feb 2, 2024

Oh and also one other question. Is it maybe possible to backport this change to 23.11?

@h7x4
Copy link
Member

h7x4 commented Feb 2, 2024

Yes, I think so, but I'm a bit unsure about how to deal with backporting release notes. I'll ask around to see if I can find some answers.

In the meanwhile, could you squash your changes into the first three commits? If you're unsure about how to do it, feel free to ask. I might be a good idea to mark the PR as draft before push, because a rebase gone wrong will ping a ton of people.

@ghost ghost marked this pull request as draft February 2, 2024 02:43
@ghost ghost force-pushed the xppen-pentablet branch from 3c646c5 to e9a7ec5 Compare February 2, 2024 02:46
@ghost
Copy link
Author

ghost commented Feb 2, 2024

Didn't really want to bother remembering how to rebase properly so just git reset --soft'ed and recreated the commits. Hope it's good enough

@ghost ghost marked this pull request as ready for review February 2, 2024 02:56
@ghost
Copy link
Author

ghost commented Feb 2, 2024

Why is it suddenly unable to find libusb?

@h7x4
Copy link
Member

h7x4 commented Feb 2, 2024

Might've been after you moved it to pkgs/by-name. libusb1 is the correct one.

@ghost
Copy link
Author

ghost commented Feb 2, 2024

I borked it, okay, will never make changes on the phone again, must've missed something important .-.

@ghost
Copy link
Author

ghost commented Feb 2, 2024

h7x4, thank you so much for helping!

@ofborg ofborg bot added 8.has: package (new) This PR adds a new package 11.by: package-maintainer This PR was created by a maintainer of all the package it changes. 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Feb 2, 2024
@ghost ghost marked this pull request as draft February 2, 2024 11:40
@ghost ghost force-pushed the xppen-pentablet branch from 9e497f1 to 7fb703b Compare February 2, 2024 11:40
@ghost ghost force-pushed the xppen-pentablet branch from 632d981 to ed7e2a1 Compare February 10, 2024 11:56
@ghost
Copy link
Author

ghost commented Feb 10, 2024

Is this a good-enough service? I'm not familiar with how people usually do it in nixpkgs, I've found a few examples with grep and followed them (nginx and nifi)
Seems to be working for me

@ghost ghost requested a review from MinerSebas February 10, 2024 11:58
@ghost
Copy link
Author

ghost commented Feb 10, 2024

However it seems like this service runs every time I nixos-rebuild my system with it even if nothing was changed :/ Is this normal? Do I need to add some nixos-specific conditions?

@MinerSebas
Copy link
Contributor

The missing piece should just be adding restartTriggers = [ cfg.package ] to the service.

Copy link
Contributor

@eclairevoyant eclairevoyant left a comment

Choose a reason for hiding this comment

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

Thank you for your work so far! Some feedback below.


services.udev.packages = [ cfg.package ];

systemd.services.xppen-create-config-dir = {
Copy link
Contributor

Choose a reason for hiding this comment

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

This service seems like something that might be better achieved with systemd-tmpfiles

Copy link
Author

Choose a reason for hiding this comment

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

Unfortunately I don't think it can, those files are installed in /var because it's kind of a hack and are not temporary, those are user configuration files:///

Copy link
Contributor

@eclairevoyant eclairevoyant Feb 12, 2024

Choose a reason for hiding this comment

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

things created via systemd-tmpfiles are not necessarily temporary, despite the name.

Copy link
Author

Choose a reason for hiding this comment

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

Or can tmpfiles be used to store files persistently? My problem is that I don't think I'd want those user-changed configs to be removed even is the module is disabled. If I could patch this app to read configs from, say, $XDG_CONFIG_HOME I would, but me no smart enough to patch binaries to this degree. /var is better than the default /usr but yes it is incredible that they even fathomed to make /usr/lib a user-writable configuration directory :)))
So yeah, don't think tmpfiles is good, these files are by no means temporary

Copy link
Author

Choose a reason for hiding this comment

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

things created via systemd-tmpfiles are not necessarily temporary, despite the name.

So what would the added benefit be then? Can it copy a set of given files instead of this little scripty thingy?

Copy link
Author

Choose a reason for hiding this comment

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

I'm just not familiar with systemd-tmpfiles that's why I'm asking:)

Nikita and others added 4 commits February 12, 2024 03:49
Co-authored-by: nuko <host@nuko.city>
Co-authored-by: nuko <host@nuko.city>
Co-authored-by: éclairevoyant <848000+eclairevoyant@users.noreply.github.com>
Co-authored-by: éclairevoyant <848000+eclairevoyant@users.noreply.github.com>
@h7x4 h7x4 changed the title Xppen pentablet xppen: init package with nixos module Feb 12, 2024
@ghost ghost force-pushed the xppen-pentablet branch from e56a4b4 to d4a8dd8 Compare February 13, 2024 09:28
@ghost ghost requested a review from eclairevoyant February 14, 2024 13:07
@eclairevoyant eclairevoyant dismissed their stale review April 15, 2024 14:24

stale review

@damiankorcz
Copy link

damiankorcz commented Sep 23, 2024

Hey,
I have an XP-Pen Deco LW. This model works wired and wireless (Bluetooth - comes with a receiver) so I can test both scenarios. By default in KDE 6, it get detected and moves the cursor (wired or wireless). I would like to help get this PR updated and merged. I've tried to build it and it appears to build fine although when running the PenTablet executable I get the following:

[damian@nixos-desktop:~/Git/nixpkgs/result/bin]$ ./PenTablet
QFont::fromString: Invalid description 'Noto Sans,10,-1,5,400,0,0,0,0,0,0,0,0,0,0,1'
QLocale::English
QMetaObject::connectSlotsByName: No matching signal for on_horizontalSlider_tablet_light_valueChanged(int)
QObject::connect: Incompatible sender/receiver arguments
        QDesktopWidget::primaryScreenChanged() --> MainWindow::onDeskScreenChanged(int)
QLayout: Attempting to add QLayout "" to MainWindow "MainWindow", which already has a layout
Function  EnumDisplay nScreenCount: 4
Device.AllScreen "(0,0,4640,2880)"
Device.Screen 1 : "1(1440,1440,2560,1440)"
Device.Screen 2 : "2(0,320,1440,2560)"
Device.Screen 3 : "3(1440,0,2560,1440)"
Device.Screen 4 : "4(4000,2400,640,480)"
-------type: 1
hid_open_path Failed!
Cannot mix incompatible Qt library (5.15.14) with this library (5.15.12)
Aborted (core dumped)

Let me know what you want me to try further. Also, it appears the version is still 3.4.9 but they add a suffix with YYMMDD e.g. currently it's on XPPenLinux3.4.9-240607. So might need to amend how the versions are defined.

@ghost
Copy link
Author

ghost commented Sep 26, 2024

Hey @damiankorcz, I have completely forgotten about this PR. Unfortunately I don't have much time to fix and update it myself, but I think you can suggest changes and fixes, I'll add them. I can still push some effort into getting this PR merged :)

@gepbird
Copy link
Contributor

gepbird commented Oct 14, 2024

A few months ago drivers with major version 4 got released. However some tablets are not compatible with v4, so we should definitely keep the current v3 driver.

Based on the previous v3 deco and g430 drivers, I packaged a working v4 driver in #347984 (tested by @HangedFool, I don't have a hardware for it), however it is not as polished as this PR but should be a decent starting point.

It would be nice to add the v4 package in this PR, but I totally understand if you don't have the capacity for it, it can be done later. At least we should make the v3 package in this PR future proof by renaming it to xppen_3 to avoid making aliases in the future that might confuse/annoy users.

@gepbird
Copy link
Contributor

gepbird commented Oct 14, 2024

Cannot mix incompatible Qt library (5.15.14) with this library (5.15.12)

@damiankorcz This is most likely not an issue with the the xppen package in this PR, but rather with Qt packages on NixOS in general.

Personally I don't have this issue and the GUI opens, but this is what I found after doing some research:

I haven't found a clear answer, but if you really want to test this PR you could try temporarily uninstalling your qt apps (also delete them with nix-collect-garbage) or updating your system to unstable (I assume you're on nixos-24.05 from the 5.15.12 Qt version). Or at this point setting up a nixos-unstable vm seems to be easier.

Copy link
Contributor

@gepbird gepbird left a comment

Choose a reason for hiding this comment

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

Hi, I don't want to be intrusive, but if you don't have time to work on this I'd be happy to take it over if it's fine with you (of course I'd credit you for all the work you've done over the past months :) ).

Is there anything else blocking this PR?

, libGL
}:
stdenv.mkDerivation {
pname = "xppen";
Copy link
Contributor

Choose a reason for hiding this comment

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

See #285660 (comment) for explanation

Suggested change
pname = "xppen";
pname = "xppen_3";

{
options.programs.xppen = {
enable = lib.mkEnableOption "XPPen PenTablet application";
package = lib.mkPackageOption pkgs "xppen" { };
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
package = lib.mkPackageOption pkgs "xppen" { };
package = lib.mkPackageOption pkgs "xppen_3" { };

@wegank wegank added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jan 2, 2025
@nixpkgs-ci nixpkgs-ci bot removed the 12.first-time contribution This PR is the author's first one; please be gentle! label Jun 25, 2025
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 Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (new) This PR adds a module in `nixos/` 8.has: module (update) This PR changes an existing module in `nixos/` 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 11.by: package-maintainer This PR was created by a maintainer of all the package it changes.

Projects

None yet

Development

Successfully merging this pull request may close these issues.