Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

libwacom: 0.33 -> 1.1 #69182

Closed
wants to merge 1 commit into from
Closed

libwacom: 0.33 -> 1.1 #69182

wants to merge 1 commit into from

Conversation

jchv
Copy link
Contributor

@jchv jchv commented Sep 21, 2019

Despite the version jump, as far as I can tell there are no incompatible or even large changes; just one new API and some bugfixes/added devices. This release also installs udev rules by default, which will aid in fixing #52490.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • 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 nix-review --run "nix-review wip" (was previously checked, but it turns out I ran the wrong command, currently rerunning.)
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

I've ran the gnome3 NixOS test and I am using this build locally with SwayWM without issue.

Copy link
Contributor

@worldofpeace worldofpeace left a comment

Choose a reason for hiding this comment

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

I'm seeing upstream uses the meson build system, can you port to using this?

I know for sure we'll need to disable the tests.

mesonFlags = [
  "-Dtests=false"
];

Can you also add the udev rules to the wacom module?
I believe it should be as simple as adding this package at

services.udev.packages = [ pkgs.xf86_input_wacom ];

@jchv
Copy link
Contributor Author

jchv commented Sep 21, 2019

I'm seeing upstream uses the meson build system, can you port to using this?

I know for sure we'll need to disable the tests.

mesonFlags = [
  "-Dtests=false"
];

Sure! I also noticed this and am excited to see it, I just didn't know that it would be preferable to switch already, since it's still new. I'll do this now.

Can you also add the udev rules to the wacom module?
I believe it should be as simple as adding this package at

services.udev.packages = [ pkgs.xf86_input_wacom ];

I was thinking about doing this kind of thing in a followup PR. As for why I don't just add it there, I have some reservations: This is an X11 specific file, but these udev rules are just as important if you are using Wayland. Would it make sense to perhaps have a config.hardware.wacom config option instead that includes the udev rules? Not sure exactly how it should be structured in relation to config.services.xserver.wacom.

@jchv
Copy link
Contributor Author

jchv commented Sep 21, 2019

Ported to use Meson.

@worldofpeace
Copy link
Contributor

Sure! I also noticed this and am excited to see it, I just didn't know that it would be preferable to switch already, since it's still new. I'll do this now.

🎆 Meson 😄

I was thinking about doing this kind of thing in a followup PR. As for why I don't just add it there, I have some reservations: This is an X11 specific file, but these udev rules are just as important if you are using Wayland. Would it make sense to perhaps have a config.hardware.wacom config option instead that includes the udev rules? Not sure exactly how it should be structured in relation to config.services.xserver.wacom.

It appears to be organized at modules/services/x11/hardware/wacom.nix.
I agree it would be a good followup PR, it appears the module is outdated and predated wayland's relevance. I'd say a hardware option would make sense, though I probably need to refresh my memory on this component.

@worldofpeace
Copy link
Contributor

worldofpeace commented Sep 21, 2019

So here's the diff between the .pc files with meson

--- /nix/store/ljvlm1b2fyqci1kli0ljbfmr2mizs97m-libwacom-0.33/lib/pkgconfig/libwacom.pc	1969-12-31 19:00:01.000000000 -0500
+++ /nix/store/m5izrmjgx82b60i8a9sfmfb1lhki84ga-libwacom-1.1-dev/lib/pkgconfig/libwacom.pc	1969-12-31 19:00:01.000000000 -0500
@@ -1,11 +1,9 @@
-prefix=/nix/store/ljvlm1b2fyqci1kli0ljbfmr2mizs97m-libwacom-0.33
-exec_prefix=${prefix}
-libdir=${exec_prefix}/lib
-includedir=${prefix}/include
+prefix=/nix/store/i75ix7wz0p0sk0948sfkax74vpkpxh10-libwacom-1.1
+libdir=${prefix}/lib
+includedir=/nix/store/m5izrmjgx82b60i8a9sfmfb1lhki84ga-libwacom-1.1-dev/include
 
-Name: libwacom
+Name: Libwacom
 Description: Wacom model feature query library
-Version: 0.33
-Requires.private: glib-2.0
-Cflags: -I${includedir}/libwacom-1.0
+Version: 1.1
 Libs: -L${libdir} -lwacom
+Cflags: -I${includedir}/libwacom-1.0

No use of Requires.private?

@jchv
Copy link
Contributor Author

jchv commented Sep 21, 2019

Interesting. My best guess is its an artifact of switching to Meson.

I'm currently running nix-review and I don't think the lack of Requires.private is causing any real side-effects, and in fact I'm kind of curious what kinds of side-effects that could cause. Checking the pkg-config docs right now.

May be worth filing a bug upstream if there's real benefit to having Requires.private...

@jchv
Copy link
Contributor Author

jchv commented Sep 21, 2019

I'm filing a bug upstream. It looks like this difference between autotools and meson is not intended. The current autotools build still contains the Requires.private.

@worldofpeace
Copy link
Contributor

worldofpeace commented Sep 21, 2019

We actual patch away the main thing with Requires.private
https://bugs.freedesktop.org/show_bug.cgi?id=4738

I'm filing a bug upstream. It looks like this difference between autotools and meson is not intended. The current autotools build still contains the Requires.private.

Thanks, I made a PR also.
It also appears the udev rules are needed by libinput which uses libwacom

So I guess those rules can get unconditionally added to the libinput module, or only when the wacom option is turned on.

@worldofpeace
Copy link
Contributor

Merged to staging in b54065a. Don't think the requires.private thing will cause an issue for us.
Thanks @jchv.

@jchv jchv deleted the libwacom-11 branch September 23, 2019 02:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants