build(cmake): Use GNUInstallDirs to install data and lib directories #1817

tgurr · 2023-11-04T20:03:59Z

Description

GNUInstallDirs (https://cmake.org/cmake/help/v3.18/module/GNUInstallDirs.html) is a CMake module which provides similar params as autotools allowing to adjust where files are installed.

On multiarch and/or cross layouts the prefix might be something like /usr/${arch} but arch independent files should still go into /usr/share.

Screenshot

Issues Fixed or Closed

Type of Change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Dependency update (updates to dependencies)
Documentation update (changes to documentation)
Repository update (changes to repository files, e.g. .github/...)

Checklist

My code follows the style guidelines of this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have added or updated the in code docstring/documentation-blocks for new or existing methods/components

Branch Updates

LizardByte requires that branches be up-to-date before merging. This means that after any PR is merged, this branch
must be updated before it can be merged. You must also
Allow edits from maintainers.

I want maintainers to keep my branch updated

tgurr · 2023-11-04T20:07:48Z

Please note I couldn't test building/running the flatpak package as I only do distribution packaging, please be sure to test the change there as well.

ReenigneArcher · 2023-11-04T20:11:47Z

I don't see this as being better or worse than what we have right now. It's just different; therefore, I'm going to close this.

tgurr · 2023-11-04T20:15:53Z

@ReenigneArcher Please reopen as this actually fixes being able to install this on distributions like Exherbo which have such a said layout and we need to manually patch this wrong behaviour downstream right now:

https://gitlab.exherbo.org/exherbo/net/-/blob/ee333903564a57dba5b1b88c4d7e29171dc5d458/packages/net-remote/sunshine/sunshine-0.21.0.exheres-0#L185

Also see this being fixed for many other projects in the same way (and not done by Exherbo devs so there is actually a need for this not only for Exherbo) just one quick recent example:

paullouisageneau/libdatachannel@f1aa6c8 which mentiones Debian as well in regards to LIBDIR: paullouisageneau/libdatachannel#975

Conan-Kudo · 2023-11-13T11:31:45Z

I fixed it in libdatachannel because otherwise it was broken on Fedora. That's also the case here.

ReenigneArcher

Please also fix conflicts.

ReenigneArcher · 2023-12-30T15:36:23Z

packaging/linux/flatpak/dev.lizardbyte.sunshine.yml

@@ -317,6 +317,7 @@ modules:
      - cd ${FLATPAK_BUILDER_BUILDDIR} && npm install
    config-opts:
      - -DCMAKE_BUILD_TYPE=Release
+      - -DCMAKE_INSTALL_DATAROOTDIR=/app/share


I think this needs to be added to the following locations, unless the default is okay. Please confirm.

Sunshine/.github/workflows/CI.yml

Lines 402 to 412 in 11d4723

cmake -DCMAKE_BUILD_TYPE=Release \

-DCMAKE_INSTALL_PREFIX=/usr \

-DSUNSHINE_ASSETS_DIR=share/sunshine \

-DSUNSHINE_EXECUTABLE_PATH=/usr/bin/sunshine \

-DSUNSHINE_ENABLE_WAYLAND=ON \

-DSUNSHINE_ENABLE_X11=ON \

-DSUNSHINE_ENABLE_DRM=ON \

-DSUNSHINE_ENABLE_CUDA=ON \

${{ matrix.EXTRA_ARGS }} \

..

make -j ${nproc}

Sunshine/packaging/linux/Arch/PKGBUILD

Lines 80 to 86 in 11d4723

cmake \

-S "$pkgname" \

-B build \

-Wno-dev \

-D CMAKE_INSTALL_PREFIX=/usr \

-D SUNSHINE_EXECUTABLE_PATH=/usr/bin/sunshine \

-D SUNSHINE_ASSETS_DIR="share/sunshine"

Sunshine/docker/debian-bookworm.dockerfile

Lines 105 to 115 in 11d4723

cmake \

-DCMAKE_CUDA_COMPILER:PATH=/build/cuda/bin/nvcc \

-DCMAKE_BUILD_TYPE=Release \

-DCMAKE_INSTALL_PREFIX=/usr \

-DSUNSHINE_ASSETS_DIR=share/sunshine \

-DSUNSHINE_EXECUTABLE_PATH=/usr/bin/sunshine \

-DSUNSHINE_ENABLE_WAYLAND=ON \

-DSUNSHINE_ENABLE_X11=ON \

-DSUNSHINE_ENABLE_DRM=ON \

-DSUNSHINE_ENABLE_CUDA=ON \

/build/sunshine

Sunshine/docker/debian-bullseye.dockerfile

Lines 119 to 129 in 11d4723

cmake \

-DCMAKE_CUDA_COMPILER:PATH=/build/cuda/bin/nvcc \

-DCMAKE_BUILD_TYPE=Release \

-DCMAKE_INSTALL_PREFIX=/usr \

-DSUNSHINE_ASSETS_DIR=share/sunshine \

-DSUNSHINE_EXECUTABLE_PATH=/usr/bin/sunshine \

-DSUNSHINE_ENABLE_WAYLAND=ON \

-DSUNSHINE_ENABLE_X11=ON \

-DSUNSHINE_ENABLE_DRM=ON \

-DSUNSHINE_ENABLE_CUDA=ON \

/build/sunshine

Sunshine/docker/fedora-38.dockerfile

Lines 106 to 115 in 11d4723

cmake \

-DCMAKE_BUILD_TYPE=Release \

-DCMAKE_INSTALL_PREFIX=/usr \

-DSUNSHINE_ASSETS_DIR=share/sunshine \

-DSUNSHINE_EXECUTABLE_PATH=/usr/bin/sunshine \

-DSUNSHINE_ENABLE_WAYLAND=ON \

-DSUNSHINE_ENABLE_X11=ON \

-DSUNSHINE_ENABLE_DRM=ON \

-DSUNSHINE_ENABLE_CUDA=OFF \

/build/sunshine

Sunshine/docker/fedora-39.dockerfile

Lines 106 to 115 in 11d4723

cmake \

-DCMAKE_BUILD_TYPE=Release \

-DCMAKE_INSTALL_PREFIX=/usr \

-DSUNSHINE_ASSETS_DIR=share/sunshine \

-DSUNSHINE_EXECUTABLE_PATH=/usr/bin/sunshine \

-DSUNSHINE_ENABLE_WAYLAND=ON \

-DSUNSHINE_ENABLE_X11=ON \

-DSUNSHINE_ENABLE_DRM=ON \

-DSUNSHINE_ENABLE_CUDA=OFF \

/build/sunshine

Sunshine/docker/ubuntu-20.04.dockerfile

Lines 155 to 165 in 11d4723

cmake \

-DCMAKE_CUDA_COMPILER:PATH=/build/cuda/bin/nvcc \

-DCMAKE_BUILD_TYPE=Release \

-DCMAKE_INSTALL_PREFIX=/usr \

-DSUNSHINE_ASSETS_DIR=share/sunshine \

-DSUNSHINE_EXECUTABLE_PATH=/usr/bin/sunshine \

-DSUNSHINE_ENABLE_WAYLAND=ON \

-DSUNSHINE_ENABLE_X11=ON \

-DSUNSHINE_ENABLE_DRM=ON \

-DSUNSHINE_ENABLE_CUDA=ON \

/build/sunshine

Sunshine/docker/ubuntu-22.04.dockerfile

Lines 120 to 130 in 11d4723

cmake \

-DCMAKE_CUDA_COMPILER:PATH=/build/cuda/bin/nvcc \

-DCMAKE_BUILD_TYPE=Release \

-DCMAKE_INSTALL_PREFIX=/usr \

-DSUNSHINE_ASSETS_DIR=share/sunshine \

-DSUNSHINE_EXECUTABLE_PATH=/usr/bin/sunshine \

-DSUNSHINE_ENABLE_WAYLAND=ON \

-DSUNSHINE_ENABLE_X11=ON \

-DSUNSHINE_ENABLE_DRM=ON \

-DSUNSHINE_ENABLE_CUDA=ON \

/build/sunshine

For regular Linux distributions the defaults which CMake with the use of GNUInstallDirs figures out should be okay, as thats main purpose of it. (See also the comment from @Conan-Kudo above in regards to lib vs lib32/lib64 handling, which for example we don't have on Exherbo as we set different arch-specific prefixes instead.)

Thinking about it it's probably also not required for the flatpak as well as it sets -DCMAKE_INSTALL_PREFIX=/app so stuff should properly end up being installed there. Pushed again to remove the line.

I probably concluded this wrong as on Exherbo we do indeed have to set DATAROOTDIR specifically as we need the arch-independent stuff to not land under our (arch-dependent) prefix.

Probably at the top of ./cmake/packaging/unix.cmake

@tgurr @ReenigneArcher special_package_configuration.cmake is include before unix.cmake been include in CMakeLists.txt, therefore GNUInstallDirs is not enabled when configure desktop file, the @CMAKE_INSTALL_DATAROOTDIR@ in .desktop files is invalid and empty string, this broken both regular and flatpak version of the desktop file in v0.22.0 release.

Sunshine/packaging/linux/sunshine.desktop

Line 15 in faeeb7e

Exec=gio launch @CMAKE_INSTALL_DATAROOTDIR@/applications/sunshine_terminal.desktop

but when I try to add include(GNUInstallDirs) to special_package_configuration.cmake and generate the desktop file again,

Exec=gio launch share/applications/sunshine_terminal.desktop

the /usr/ in path is missing, a cmake bug? my distro is archlinux.
so how to fix this, revert the change for these desktop file, add -DCMAKE_INSTALL_DATAROOTDIR="/usr/share" in PKGBUILD and anywhere needed, or wait cmake fix @CMAKE_INSTALL_DATAROOTDIR@ in configure_file()?

Adding -DCMAKE_INSTALL_DATAROOTDIR:PATH=/usr/share where needed appears to be the correct approach to me, e.g. on Exherbo we have to do that anyways because we need to install arch-independent stuff into /usr and arch-dependent stuff into prefix e.g. /usr/x86_64-pc-linux-gnu like bin, lib and so on which was the reason for this MR in the first place. Unfortunate this doesn't work automatically for configure_file() but I can see no reason why @CMAKE_INSTALL_PREFIX@ would work but @CMAKE_INSTALL_DATAROOTDIR@ would not, according the the PKGBUILD @CMAKE_INSTALL_PREFIX@ was and is also manually set: https://github.com/LizardByte/Sunshine/blob/nightly/packaging/linux/Arch/PKGBUILD#L65 which might explain why things worked in the first place and brings me back to my suggestion of passing -DCMAKE_INSTALL_DATAROOTDIR:PATH=/usr/share when/where needed. Hope you all agree.

@tgurr wait according to cmake doc you post above, variable CMAKE_INSTALL_DATAROOT's value should really just share.

CMAKE_INSTALL_FULL_<dir>

The absolute path generated from the corresponding CMAKE_INSTALL_<dir> value. If the value is not already an absolute path, an absolute path is constructed typically by prepending the value of the CMAKE_INSTALL_PREFIX variable. However, there are some special cases as documented below.

you need CMAKE_INSTALL_FULL_DATAROOT for value that auto prepend CMAKE_INSTALL_PREFIX in to CMAKE_INSTALL_DATAROOT, maybe you can make a new MR for update this and add include(GNUInstallDirs) in special_package_configuration.cmake?

@detiam Do I get you right that you mean:

-Exec=gio launch @CMAKE_INSTALL_DATAROOTDIR@/applications/sunshine_terminal.desktop +Exec=gio launch @CMAKE_INSTALL_FULL_DATAROOTDIR@/applications/sunshine_terminal.desktop

I lack the possibility to test your usecase and also don't want to steal the credits from you so if you'd prepare an MR instead I'd happiliy test my usecase to see if something breaks for me (which I don't expect, in the worst case we just set the different variable manually for our build). If you still want to me to prepare an MR instead please advise.

Please open a new PR (or issue) to help resolve this. Conversation on merged/closed PRs is going to get lost and forgotten about.

@detiam Do I get you right that you mean:

-Exec=gio launch @CMAKE_INSTALL_DATAROOTDIR@/applications/sunshine_terminal.desktop +Exec=gio launch @CMAKE_INSTALL_FULL_DATAROOTDIR@/applications/sunshine_terminal.desktop

I lack the possibility to test your usecase and also don't want to steal the credits from you so if you'd prepare an MR instead I'd happiliy test my usecase to see if something breaks for me (which I don't expect, in the worst case we just set the different variable manually for our build). If you still want to me to prepare an MR instead please advise.

ok I opened a new PR here #2223 :)

CMakeLists.txt

tgurr · 2023-12-31T00:11:02Z

Please also fix conflicts.

Rebased on current nightly.

GNUInstallDirs (https://cmake.org/cmake/help/v3.18/module/GNUInstallDirs.html) is a CMake module which provides similar params as autotools allowing to adjust where files are installed. On multiarch and/or cross layouts the prefix might be something like /usr/${arch} but arch independent files should still go into /usr/share.

…izardByte#1817)

ReenigneArcher closed this Nov 4, 2023

ReenigneArcher reopened this Nov 4, 2023

ReenigneArcher reviewed Dec 30, 2023

View reviewed changes

ReenigneArcher changed the title ~~cmake: Use GNUInstallDirs to install data and lib directories~~ build(cmake): Use GNUInstallDirs to install data and lib directories Jan 1, 2024

ReenigneArcher merged commit faeeb7e into LizardByte:nightly Jan 1, 2024
42 checks passed

tgurr deleted the gnuinstalldirs branch January 1, 2024 05:44

detiam mentioned this pull request Mar 6, 2024

Fix wrong path in desktop file #2223

Merged

11 tasks

KuleRucket pushed a commit to KuleRucket/Sunshine that referenced this pull request Jun 6, 2024

build(cmake): Use GNUInstallDirs to install data and lib directories (L…

c8fc9c0

…izardByte#1817)

e-dong pushed a commit to e-dong/Sunshine that referenced this pull request Jul 26, 2024

build(cmake): Use GNUInstallDirs to install data and lib directories (L…

5165c0e

…izardByte#1817)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build(cmake): Use GNUInstallDirs to install data and lib directories #1817

build(cmake): Use GNUInstallDirs to install data and lib directories #1817

tgurr commented Nov 4, 2023

tgurr commented Nov 4, 2023

ReenigneArcher commented Nov 4, 2023

tgurr commented Nov 4, 2023 •

edited

Loading

Conan-Kudo commented Nov 13, 2023

ReenigneArcher left a comment

ReenigneArcher Dec 30, 2023

tgurr Dec 31, 2023

tgurr Dec 31, 2023 •

edited

Loading

detiam Mar 5, 2024

tgurr Mar 5, 2024

detiam Mar 5, 2024

tgurr Mar 5, 2024

ReenigneArcher Mar 5, 2024

detiam Mar 6, 2024

tgurr commented Dec 31, 2023

	cmake -DCMAKE_BUILD_TYPE=Release \
	-DCMAKE_INSTALL_PREFIX=/usr \
	-DSUNSHINE_ASSETS_DIR=share/sunshine \
	-DSUNSHINE_EXECUTABLE_PATH=/usr/bin/sunshine \
	-DSUNSHINE_ENABLE_WAYLAND=ON \
	-DSUNSHINE_ENABLE_X11=ON \
	-DSUNSHINE_ENABLE_DRM=ON \
	-DSUNSHINE_ENABLE_CUDA=ON \
	${{ matrix.EXTRA_ARGS }} \
	..
	make -j ${nproc}

	cmake \
	-S "$pkgname" \
	-B build \
	-Wno-dev \
	-D CMAKE_INSTALL_PREFIX=/usr \
	-D SUNSHINE_EXECUTABLE_PATH=/usr/bin/sunshine \
	-D SUNSHINE_ASSETS_DIR="share/sunshine"

	cmake \
	-DCMAKE_CUDA_COMPILER:PATH=/build/cuda/bin/nvcc \
	-DCMAKE_BUILD_TYPE=Release \
	-DCMAKE_INSTALL_PREFIX=/usr \
	-DSUNSHINE_ASSETS_DIR=share/sunshine \
	-DSUNSHINE_EXECUTABLE_PATH=/usr/bin/sunshine \
	-DSUNSHINE_ENABLE_WAYLAND=ON \
	-DSUNSHINE_ENABLE_X11=ON \
	-DSUNSHINE_ENABLE_DRM=ON \
	-DSUNSHINE_ENABLE_CUDA=ON \
	/build/sunshine

build(cmake): Use GNUInstallDirs to install data and lib directories #1817

build(cmake): Use GNUInstallDirs to install data and lib directories #1817

Conversation

tgurr commented Nov 4, 2023

Description

Screenshot

Issues Fixed or Closed

Type of Change

Checklist

Branch Updates

tgurr commented Nov 4, 2023

ReenigneArcher commented Nov 4, 2023

tgurr commented Nov 4, 2023 • edited Loading

Conan-Kudo commented Nov 13, 2023

ReenigneArcher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tgurr Dec 31, 2023 • 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

tgurr commented Dec 31, 2023

tgurr commented Nov 4, 2023 •

edited

Loading

tgurr Dec 31, 2023 •

edited

Loading