Install UDev rules to the proper location by default #4114
Conversation
|
I'll leave this open for a little to give others a chance to comment, but it looks good to me. |
|
Ugh, why does the "Close and comment" button exist?! |
|
@dvzrv could you take a look at this? |
|
@Be-ing I just noticed a comment in the UDev file saying it has to be named |
|
LGTM! :) |
There was a problem hiding this comment.
Ubuntu build fails, because the udev file was moved. Please also update the other locations where the file is used:
$ grep --exclude-dir .git --exclude-dir build -HrnF 'res/linux/mixxx-usb-uaccess.rules' .
./res/linux/mixxx.metainfo.xml:40: <!-- Keep these IDs in sync with res/linux/mixxx-usb-uaccess.rules -->
./packaging/CPackDebUploadPPA.cmake:82:file(COPY ${CPACK_TOPLEVEL_DIRECTORY}/${CPACK_PACKAGE_FILE_NAME}/res/linux/mixxx-usb-uaccess.rules
./packaging/CPackDebInstall.cmake:36:file(COPY ${CPACK_DEBIAN_SOURCE_DIR}/res/linux/mixxx-usb-uaccess.rules
|
@daschuer Can you test this? The new location works fine for me, but I remember we had a lengthy discussion about the different paths for udev files and which one is correct (for reference: |
There was a problem hiding this comment.
The default settings shall be optimized for home builds.
The current version uses
/usr/local/share/mixxx/udev/rules.d
This PR uses
/usr/local/lib/udev/rules.d
On Ubuntu I have no /usr/local/lib/udev/rules.d and I don't like to get one by default.
The issue is that without external help we cannot find the right place without messing around with the user system. That's why we have decided it #3458
To install it to the neutral ground of /usr/local/share/mixxx/udev/rules.d and let the user decide.
According to @Holzhaus
At least on Arch it's pretty standard to install it to share and let users copy it themselves. Also, that way we make sure the rules file is included in binary archives (cpack -G TGZ).
I also think it is a bad idea to move and rename a file in a stable branch. This will break every packaging script which is already adopted.
| \"If you are installing Mixxx from source for your own use, copy\n\" | ||
| \"mixxx-usb-uaccess.rules to /etc/udev/rules.d/ and run:\n\" | ||
| \" udevadm control --reload-rules && udevadm trigger\n\" | ||
| \"as root to load the rules.\n\" | ||
| \"If you are building a package for a distribution, the correct directory for\n\" |
There was a problem hiding this comment.
Because the intent of this PR is to install it to the right place in the first place. At that point this comment is unnecessary, it'll already be in the right place.
There was a problem hiding this comment.
Once we install it at the right place, we need to rewrite this message. Maybe it can be omitted entirely and only be issues as a fatal message if the detection of the correct install directory fails.
For that case we may take the directory including file name as a cmake option.
CMakeLists.txt
Outdated
| ) | ||
| install(CODE " | ||
| message(STATUS \"Important Note: Installation of udev rules\n\" | ||
| \"The udev rule file for USB HID and Bulk controller permissions will be\n\" | ||
| \"installed to:\n\" | ||
| \" ${CMAKE_INSTALL_PREFIX}/${MIXXX_INSTALL_DATADIR}/udev/rules.d.\n\" | ||
| \" ${CMAKE_INSTALL_LIBDIR}/udev/rules.d.\n\" | ||
| \"If you are installing Mixxx from source for your own use, copy\n\" | ||
| \"mixxx-usb-uaccess.rules to /etc/udev/rules.d/ and run:\n\" |
There was a problem hiding this comment.
| \"mixxx-usb-uaccess.rules to /etc/udev/rules.d/ and run:\n\" | |
| \"69-mixxx-usb-uaccess.rules to /etc/udev/rules.d/ and run:\n\" |
Hm, but who are the users of that spec? Downstream distributions? They usually package an upstream their own way. |
|
If on Ubuntu it doesn't install to |
The comment in the CMakeLists.txt talks about some proper location the
file has to be installed too, but itself installs it to a wrong
directory. Rather than telling people to turn the option off and install
the file to the right directory manually, install it directly to the
correct directory instead. ${CMAKE_INSTALL_LIBDIR} will expand to
whatever distros require it to be _assuming_ they set their
-DCMAKE_INSTALL_PREFIX and -DCMAKE_INSTALL_LIBDIR correctly
Also while we're at it, make sure it's loaded before 70-uaccess.rules
rather than just having a comment mention that in the udev file itself
Hm, that's odd. From where does ubuntu load udev rules?
As an idea: How about letting the user provide a location for it themselves via an option to cmake then? That would solve this problem generically and could default to a "contrib" (i.e. inactive) location.
It is not standard and I would actually prefer the udev rule to end up in the correct location (without me having to manually move it into place). Those are either not udev rules or are in place due to upstream deciding to install them to a "contrib" directory (e.g. lirc). There are of course also cases where we specifically decide to install the udev rule to the correct location (e.g. solaar).
I would not worry about that too much, as downstreams can adapt to such a change. Eventually I guess (for me) it boils down to: |
Yes. Before we had this udev rule in the repository we got lots of support requests on our forum about users unable to access HID controllers and we had to direct them how to install a udev rule. We still get this sometimes because distribution packages are not all installing it correctly.
I agree. Installing to |
CMake sets the default prefix to /usr/local but distributions explicitly set it to /usr. |
|
Another option could be not installing this file with CMake and explicitly telling packagers in our release notes what to do (#4115). The current warning message has not been effective as we intended. This discussion has only come up because I specifically asked packagers to check the udev rule gets installed. |
I mean, that's a option, but there is no reason not to just install this file with CMake. Seriously, if you do your packaging right (so seemingly now how the Ubuntu packaging is done currently) then this PR will let CMake install the UDev file to the right directory on all distributions. There is no need to do it any other way. |
|
I think it would be better to check if the CMAKE_INSTALL_PREFIX is /usr and warn about the udev rule file if it is not. No need to implement that in this PR though. |
|
I am not certain what are we trying to solve.
So in the case of Alpine Linux you can Problem solved. With this patch, we clutter the user directory in some cases and break packaging scripts that have already adopted that approach. I am not saying that we cannot improve the situation. For instance we can allow to set the installation path via a cmake option. But It is better tho keep the default like this, to not risk installation issues in point releases where I expect that the package maintainer will only change the commit hash and the check sum. |
From what I can tell this PR is attempting to install a udev rule to the correct location automatically. A lot of applications do it, e.g. upower using From what I understand it can either be done with automatic detection (e.g. pkgconf) or using an option with a default (e.g. install to
But it does. The default vendor location is
I am not sure what you mean by that. Are you referring to the ordering of udev rule files?
There really are only three (common) targets from what I understand:
IIUC the use of
This can be done, but also reads like unneeded manual steps.
Whereas it can just be
If you mean
If the file location changes and the packager chose to move the file, the build will fail and the packager can easily adopt as packages are usually built in a clean environment (and depending on distribution also offers tooling around detection of changed files, etc.). There is not a lot to it. |
|
Thank you @dvzrv that sounds reasonable. So there is some work and testing left here. |
|
I think a more generic file name would be appropriate, maybe rename from |
|
Itvs just for hid, right? How about |
|
No it is not just for HID. It is also for those odd old controllers that Mixxx accesses USB Bulk endpoints through libusb. |
|
@daschuer merge? |
Packagers are telling us they want this so let's listen to them. |
|
Ugh, the close and comment button... why does it exist?! |
|
The current state brakes existing package scripts and makes the situation worse for local builds. I am happy to merge it if it works like outlined by @dvzrv |
|
This is why we need a section for packagers in our release notes. |
|
This PR is a good idea and better then every written Text that can be ignored. |
|
let's not hold off 2.3.1 by this |
|
What is not complete here? This already works. |
|
I have outlined the issues in my review comments. |
| DESTINATION | ||
| "${MIXXX_INSTALL_DATADIR}/udev/rules.d" | ||
| "${CMAKE_INSTALL_LIBDIR}/udev/rules.d" |
There was a problem hiding this comment.
By default this is "/usr/local/lib/udev/rules.d" which is wrong at least for Ubuntu
There was a problem hiding this comment.
So what is the problem? It did not install to the right place before either if the prefix was /usr/local
There was a problem hiding this comment.
Before it was installed as data to the standard arch directory under a mixxx folder. I did not consider this as clutter.
Please refer to the original decision process. #3458
There was a problem hiding this comment.
The original decision was bad, as packagers have told us. This exists for their convenience. Please listen to them.
There was a problem hiding this comment.
It makes no difference whether it is installed to one wrong location or another. Neither has an effect.
There was a problem hiding this comment.
Note that Ubuntu will install it to the correct place if it's packaging sets -DCMAKE_INSTALL_PREFIX=/. If it doesn't right now, that's wrong anyhow.
There was a problem hiding this comment.
There was a problem hiding this comment.
That's autoconf and not relevant for a CMake project. Besides, you should be able to overwrite whatever defaults it sets as a packager.
There was a problem hiding this comment.
Oh right, wrong link:
https://github.com/Debian/debhelper/blob/eb6e510ef52430c6edb96b28879fa4f64fbcafdb/Debian/Debhelper/Buildsystem/cmake.pm#L15
Besides, you should be able to overwrite whatever defaults it sets as a packager.
Yes, but the point of the whole PR is to avoid manual steps, right?
There was a problem hiding this comment.
I find configuring CMake correctly by adding an argument to the command you're calling any way better than executing an entirely new command 😉
But it seems Ubuntu installs things in places I didn't expect, so I guess the pkg-config method is the way to go. Closing this PR for now.
No, not really because this would require root permissions to install even if the user has write permission to CMAKE_INSTALL_PREFIX. |
Rejecting PRs because they don't solve every existing problem is not productive. That is how we get giant PRs and burn out developers. |
That is not correct. In a build environment (or even when only building for the user's target system) the required packages to build (and introspect that system) are expected to be installed. One of them should be pkgconf/pkg-config. We can simply rely on FindPkgConfig's pkg_get_variable, e.g. (if I got it right): This way we can always use |
The PR is not rejected, but still being worked on. It does not help to get upset about it either. Sometimes PRs take a little to get right. :) @PureTryOut If you have time to look into any of my above suggestions, I'd be happy to test them! :) |
The comment in the CMakeLists.txt talks about some proper location the
file has to be installed too, but itself installs it to a wrong
directory. Rather than telling people to turn the option off and install
the file to the right directory manually, install it directly to the
correct directory instead. ${CMAKE_INSTALL_LIBDIR} will expand to
whatever distros require it to be
Also while we're at it, make sure it's loaded before 70-uaccess.rules
rather than just having a comment mention that in the udev file itself