Use pkg config udevdir #4126

daschuer · 2021-07-20T08:48:07Z

This PR implement the suggestions by @dvzrv #4114 to make use of pk-config to install the udev rules file in the correct location.

daschuer · 2021-07-20T08:49:30Z

This is not yet tested widely. Please let confirm that it does not break existing packaging and does the right thing.

Holzhaus · 2021-07-20T08:55:57Z

CMakeLists.txt

+  if(INSTALL_USER_UDEV_RULES)
+    set(MIXXX_UDEVDIR "${MIXXX_INSTALL_DATADIR}/udev")
+    if (CMAKE_INSTALL_PREFIX STREQUAL "/usr")
+      message(FATAL_ERROR "Bla")


Ups, a debug left over.

rebased out

Holzhaus · 2021-07-20T08:58:06Z

CMakeLists.txt

+          if(PKGCONFIG_UDEVDIR)
+	    file(TO_CMAKE_PATH "${PKGCONFIG_UDEVDIR}" MIXXX_UDEVDIR)
+          endif()


Are you sure that PKGCONFIG_UDEVDIR is not an absolute path outside CMAKE_INSTALL_PREFIX? Mixxx mustn't install anything outside the chosen prefix under any circumstances.

It does not:

-- Installing: /home/be/local/share/mixxx/udev/rules.d/mixxx-usb-uaccess.rules

And this is certainly the case on all systems? So returning a relative path instead of an absolute one is guaranteed by pkg-config?

I don't know.

Are you sure that PKGCONFIG_UDEVDIR is not an absolute path outside CMAKE_INSTALL_PREFIX? Mixxx mustn't install anything outside the chosen prefix under any circumstances.

In case of udevdir it is an exception. The rules files have to be installed at the common folder returned by pkg-config IF Mixxx is installed in the system location "/usr". This should happens only in the packaging case.

In all other case the udevdir is useless and the rules file is installed under our fall back location .../share/mixxx/udev/rules.d/mixxx-usb-uaccess.rules as before.

This should make all hacks in the packaging scripts obsolete.

Do we know a distro where this assumption is void?

That would be wrong for Ubuntu sorry

What would be wrong? Have you read the special case section at: https://cmake.org/cmake/help/v3.15/module/GNUInstallDirs.html?highlight=gnuinstalldirs#special-cases

The following values of CMAKE_INSTALL_PREFIX are special:

/
For <dir> other than the SYSCONFDIR, LOCALSTATEDIR and RUNSTATEDIR, the value of CMAKE_INSTALL_<dir> is prefixed with usr/ if it is not user-specified as an absolute path. For example, the INCLUDEDIR value include becomes usr/include. This is required by the GNU Coding Standards, which state:
When building the complete GNU system, the prefix will be empty and /usr will be a symbolic link to /.

Ah Ok, so we need to check how debuild uses CMake

What does PC return for udevdir on arch?

$ pkgconf --variable=udevdir udev /usr/lib/udev

Arch does not use /lib or /lib64 (it is symlinked to /usr/lib), as well as we also don't do /usr/sbin or /bin or /sbin (it is symlinked to /usr/bin): https://www.freedesktop.org/wiki/Software/systemd/TheCaseForTheUsrMerge/

Cool, so it should work for all major distros at least

…g config udevdir

dvzrv · 2021-07-21T06:02:01Z

CMakeLists.txt

-    ")
+  if(INSTALL_USER_UDEV_RULES)
+    set(MIXXX_UDEVDIR "${MIXXX_INSTALL_DATADIR}/udev")
+    if (CMAKE_INSTALL_PREFIX STREQUAL "/usr")


I am not sure this will work on e.g. nixOS, as they have a very different directory structure as packages are installed to the /nix directory (but I also don't really know how they handle udev rule files, better ask someone who knows!).

Can we not check whether CMAKE_INSTALL_PREFIX does not match /usr/local instead?

The install prefix can also be
~/.local or /opt or wherever the user thinks it is correct.
So it is IMHO more reasonable to check for the system path that corospods to the system udev dir.
We can extend the list of suitable packaging install prefixes on demand.

daschuer · 2021-07-21T16:17:40Z

debuild uses prefix /usr and requires the udevdir at /lib/udev
https://github.com/Debian/debhelper/blob/eb6e510ef52430c6edb96b28879fa4f64fbcafdb/Debian/Debhelper/Buildsystem/autoconf.pm#L34

If we want to install the rules automatically to the final location, we cannot enforce the rule "no file outside the install prefix" at the same time. Since pkg-config is telling us to violate the rule, it should be OK to follow.

Interestingly CMAKE_INSTALL_PREFIX "/" and "/usr" are treated as the same location:
https://cmake.org/cmake/help/latest/module/GNUInstallDirs.html#special-cases
There are locations listed outside the /usr prefix which are used anyway.

So we can argue that we don't even voialate the rule, because "/usr" is actually "/" which allows any location.

I have added a comment, with some hints.

Be-ing · 2021-07-22T00:55:53Z

CMakeLists.txt

+    if (MIXXX_UDEVDIR STREQUAL "${MIXXX_INSTALL_DATADIR}/udev")
+      install(
+        FILES
+          "${CMAKE_CURRENT_SOURCE_DIR}/res/linux/mixxx-usb-uaccess.rules"


Suggested change

"${CMAKE_CURRENT_SOURCE_DIR}/res/linux/mixxx-usb-uaccess.rules"

"${CMAKE_CURRENT_SOURCE_DIR}/packaging/linux/69-mixxx-dj-hardware.rules"

This file needs to be renamed to have the correct priority.

This is the path where we install the file to the data directory. I have not renamed it to not break existing packaging scripts. To use the file the user can move and rename it as desired.

That defeats the purpose. The packager should not have to do anything besides the CMake install step.

Here we are in the branch that is not for packagers. In the packager's branch the file is renamed.

This is unnecessarily complicated. Please simply rename the file. Packagers have already said that is not a problem. At most it is changing a single line of code for them.

This place is not for packagers!

Can't we just rename it in main? I think it's not really that important to change in 2.3 and packagers have already written packaging scripts with the old name for 2.3.0, so let's not break existing scripts in a point release.

Why is this file even installed to somewhere it has no effect? I think we should just show a warning that the file has not been installed.

It makes sense to have this file as part of the installer and move it yourself. FWIW it's actually a bit unexpected that just having mixxx installed messes around with your system permissions instead of copying the file yourself, but in this special case it's not an issue because the security implications of giving all users permission to access HID DJ controllers are small :D

daschuer · 2021-07-25T10:03:23Z

This is IMHO in a mergeable state. @Holzhaus please have a look.

Holzhaus

A simple test seems to work, make DESTDIR=/tmp/foo install seems to work correctly.

I'm still unsure if we actually need the pkgconfig magic instead of letting the packager take care of it. But I don't want to bikeshed over this. Would be good to get confirmation from @dvzrv and @PureTryOut though.

CMakeLists.txt

Holzhaus · 2021-07-25T13:02:20Z

CMakeLists.txt

+    if (MIXXX_UDEVDIR STREQUAL "${MIXXX_INSTALL_DATADIR}/udev")
+      install(
+        FILES
+          "${CMAKE_CURRENT_SOURCE_DIR}/res/linux/mixxx-usb-uaccess.rules"


It makes sense to have this file as part of the installer and move it yourself. FWIW it's actually a bit unexpected that just having mixxx installed messes around with your system permissions instead of copying the file yourself, but in this special case it's not an issue because the security implications of giving all users permission to access HID DJ controllers are small :D

Holzhaus · 2021-07-25T13:05:23Z

CMakeLists.txt

-    ")
+  if(INSTALL_USER_UDEV_RULES)
+    set(MIXXX_UDEVDIR "${MIXXX_INSTALL_DATADIR}/udev")
+    if (CMAKE_INSTALL_PREFIX STREQUAL "/usr" OR CMAKE_INSTALL_PREFIX STREQUAL "/" )


just an idea, maybe it's not better:

Suggested change

if (CMAKE_INSTALL_PREFIX STREQUAL "/usr" OR CMAKE_INSTALL_PREFIX STREQUAL "/" )

if (CMAKE_INSTALL_PREFIX IN_LIST "/;/usr" )

PureTryOut · 2021-07-25T15:31:29Z

Uh, I'm still unsure why #4114 didn't do the right thing and why this would be better. In the end both PR's achieve the same result (assuming packaging is done correctly, which it doesn't seem to be for Ubuntu currently as it doesn't set -DCMAKE_INSTALL_PREFIX to /usr) but this seems a way more complicated way to do it.

Why is making sure -DCMAKE_INSTALL_PREFIX is set correctly in packaging and then using ${CMAKE_INSTALL_LIBDIR}/udev/rules.d wrong? Why would this more complicated pkg-config stuff be preferable?

I'm still unsure if we actually need the pkgconfig magic instead of letting the packager take care of it.

I'm definitely against packagers having to do a mv <file> <new location> when CMake can install it to the right place in the first place.

Co-authored-by: Jan Holthuis <holthuis.jan@googlemail.com>

daschuer · 2021-07-25T15:51:17Z

#4114 does not work for all, because it guesses the right udev location. This one does only install it when it is certain to be correct. That is an improvement.

In case of Ubuntu we use a manual install step using dh_installudev

mixxx/packaging/debian/rules

Line 10 in bee21d8

dh_installudev --name=mixxx-usb-uaccess --priority 69

With this PR the packager can omit this step.

Why is making sure -DCMAKE_INSTALL_PREFIX is set correctly in packaging and then using ${CMAKE_INSTALL_LIBDIR}/udev/rules.d wrong? Why would this more complicated pkg-config stuff be preferable?

Two reasons:

With debuilder toolcahin CMAKE_INSTALL_PREFIX is set to /usr, but the correct install location is /lib/udev
If CMAKE_INSTALL_LIBDIR is not / or /usr the install location is wrong.

PureTryOut · 2021-07-25T16:13:33Z

With debuilder toolcahin CMAKE_INSTALL_PREFIX is set to /usr, but the correct install location is /lib/udev

So set it to / instead? Or does Ubuntu install some things to /usr and some things to /? That would be weird but I guess a valid reason not to use #4114. If it does not, this PR just seems a more complicated way to achieve the same result.

daschuer · 2021-07-25T17:18:50Z

Yes right, on Ubuntu the default install prefix is /usr while the udev dir is /lib/udev.

PureTryOut · 2021-07-25T19:10:59Z

Weird... But ok, valid point then.

daschuer · 2021-07-27T19:47:07Z

@Holzhaus merge?

github-actions bot added the build label Jul 20, 2021

Holzhaus requested changes Jul 20, 2021

View reviewed changes

When install at the system path /usr install the rules file at the pk…

7ea35d7

…g config udevdir

daschuer force-pushed the udev branch from e659766 to 7ea35d7 Compare July 20, 2021 23:51

dvzrv reviewed Jul 21, 2021

View reviewed changes

Also treat CMAKE_INSTALL_PREFIX=/ as system path

395c78d

Be-ing reviewed Jul 22, 2021

View reviewed changes

daschuer requested a review from Holzhaus July 25, 2021 10:03

Holzhaus approved these changes Jul 25, 2021

View reviewed changes

Improve comment

858a5ab

Co-authored-by: Jan Holthuis <holthuis.jan@googlemail.com>

uklotzde added this to the 2.3.1 milestone Jul 27, 2021

Holzhaus merged commit ccfb894 into mixxxdj:2.3 Jul 29, 2021

daschuer deleted the udev branch August 1, 2021 11:04

	"${CMAKE_CURRENT_SOURCE_DIR}/res/linux/mixxx-usb-uaccess.rules"
	"${CMAKE_CURRENT_SOURCE_DIR}/packaging/linux/69-mixxx-dj-hardware.rules"

	if (CMAKE_INSTALL_PREFIX STREQUAL "/usr" OR CMAKE_INSTALL_PREFIX STREQUAL "/" )
	if (CMAKE_INSTALL_PREFIX IN_LIST "/;/usr" )

Use pkg config udevdir #4126

Use pkg config udevdir #4126

Conversation

daschuer commented Jul 20, 2021

daschuer commented Jul 20, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Holzhaus Jul 20, 2021 • 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

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

daschuer commented Jul 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Be-ing Jul 22, 2021 • 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

Holzhaus Jul 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daschuer commented Jul 25, 2021

Holzhaus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PureTryOut commented Jul 25, 2021

daschuer commented Jul 25, 2021

PureTryOut commented Jul 25, 2021

daschuer commented Jul 25, 2021

PureTryOut commented Jul 25, 2021

daschuer commented Jul 27, 2021

Holzhaus Jul 20, 2021 •

edited

Loading

daschuer commented Jul 21, 2021 •

edited

Loading

Be-ing Jul 22, 2021 •

edited

Loading

Holzhaus Jul 22, 2021 •

edited

Loading