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.

Sign up for GitHub

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

Jump to bottom

Use pkg config udevdir #4126

Merged
merged 3 commits into from
Jul 29, 2021
Merged

Use pkg config udevdir #4126

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7ea35d7
When install at the system path /usr install the rules file at the pk…
daschuer Jul 20, 2021
395c78d
Also treat CMAKE_INSTALL_PREFIX=/ as system path
daschuer Jul 21, 2021
858a5ab
Improve comment
daschuer Jul 25, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
73 changes: 52 additions & 21 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1370,27 +1370,58 @@ if(UNIX AND NOT APPLE)
)

option(INSTALL_USER_UDEV_RULES "Install user udev rule file for USB HID and Bulk controllers" ON)
if (INSTALL_USER_UDEV_RULES)
install(
FILES
"${CMAKE_CURRENT_SOURCE_DIR}/res/linux/mixxx-usb-uaccess.rules"
DESTINATION
"${MIXXX_INSTALL_DATADIR}/udev/rules.d"
)
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\"
\"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\"
\"system rules is either /lib/udev/rules.d (e.g. Debian, Fedora) or\n\"
\"/usr/lib/udev/rules.d (e.g. Arch Linux) with an appropriate priority prefix.\n\"
\"Adjust your package script accordingly and set -DINSTALL_USER_UDEV_RULES=OFF\")
")
if(INSTALL_USER_UDEV_RULES)
set(MIXXX_UDEVDIR "${MIXXX_INSTALL_DATADIR}/udev")
if (CMAKE_INSTALL_PREFIX STREQUAL "/usr" OR CMAKE_INSTALL_PREFIX STREQUAL "/" )
Copy link
Member

Choose a reason for hiding this comment

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

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" )

# /usr and / install prefixes at treated by cmake GNUInstallDirs as
# synonym for "system location" in wich case we can lookup the correct udevdir
daschuer marked this conversation as resolved.
Show resolved Hide resolved
# See: https://cmake.org/cmake/help/latest/module/GNUInstallDirs.html#special-cases
find_package(PkgConfig)
if (PKG_CONFIG_FOUND)
pkg_check_modules( PKGCONFIG_UDEV udev)
if (PKGCONFIG_UDEV_FOUND)
execute_process(
COMMAND ${PKG_CONFIG_EXECUTABLE} --variable=udevdir udev
OUTPUT_VARIABLE PKGCONFIG_UDEVDIR
OUTPUT_STRIP_TRAILING_WHITESPACE
)
if(PKGCONFIG_UDEVDIR)
file(TO_CMAKE_PATH "${PKGCONFIG_UDEVDIR}" MIXXX_UDEVDIR)
endif()
Comment on lines +1389 to +1391
Copy link
Member

@Holzhaus Holzhaus Jul 20, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

It does not:

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know.

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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 /.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

What does PC return for udevdir on arch?

Copy link

Choose a reason for hiding this comment

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

$ 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/

Copy link
Member Author

Choose a reason for hiding this comment

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

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

endif()
endif()
endif()
if (MIXXX_UDEVDIR STREQUAL "${MIXXX_INSTALL_DATADIR}/udev")
install(
FILES
"${CMAKE_CURRENT_SOURCE_DIR}/res/linux/mixxx-usb-uaccess.rules"
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
"${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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@Be-ing Be-ing Jul 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

This place is not for packagers!

Copy link
Member

@Holzhaus Holzhaus Jul 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

DESTINATION
"${MIXXX_UDEVDIR}/rules.d"
)
install(CODE "
message(STATUS \"Important Note: Installation of udev rules\n\"
\"The udev rule file for USB HID and Bulk controller permissions have been\n\"
\"installed to:\n\"
\" ${MIXXX_UDEVDIR}/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\"
\" 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\"
\"system rules is either /lib/udev/rules.d (e.g. Debian, Fedora) or\n\"
\"/usr/lib/udev/rules.d (e.g. Arch Linux) with an appropriate priority prefix.\n\"
\"Adjust your package script accordingly and set -DINSTALL_USER_UDEV_RULES=OFF\")
")
else()
install(
FILES
"${CMAKE_CURRENT_SOURCE_DIR}/res/linux/mixxx-usb-uaccess.rules"
DESTINATION
"${MIXXX_UDEVDIR}/rules.d"
RENAME
"69-mixxx-usb-uaccess.rules"
)
endif()
endif()
endif()

Expand Down