Differential D26245

Set SYSCONFDIR to /etc when CMAKE_INSTALL_SYSCONFDIR is etc relative to /usr
Needs ReviewPublic
Actions

Authored by pwojcik on Dec 27 2019, 5:53 PM.

Details

Reviewers
kossebau
alexmerry
Summary

Marble built with -DCMAKE_INSTALL_PREFIX=/usr installs its data into /usr/etc/xdg/marble.knsrc, what is wrong.
This is because KDEInstallDirs do not handle special case of CMAKE_INSTALL_SYSCONFDIR=etc with CMAKE_INSTALL_PREFIX=/usr.
I am not sure where CMAKE_INSTALL_SYSCONFDIR is set, but certainly not from commandline. Probably from GNUInstallDirs.
GNUInstallDirs already handle special cases: let's use it.

This change complements https://cgit.kde.org/extra-cmake-modules.git/commit/?id=e92da33 and make behavior as documented, i.e. :

SYSCONFDIR

read-only single-machine data (etc, or /etc if CMAKE_INSTALL_PREFIX is /usr)

Diff Detail

Repository
R240 Extra CMake Modules
Lint
Lint Skipped
Unit
Unit Tests Skipped
pwojcik created this revision.Dec 27 2019, 5:53 PM
Restricted Application added projects: Frameworks, Build System. · View Herald TranscriptDec 27 2019, 5:53 PM
Restricted Application added subscribers: kde-buildsystem, kde-frameworks-devel. · View Herald Transcript
pwojcik requested review of this revision.Dec 27 2019, 5:53 PM
pwojcik retitled this revision from Set SYSCONFDIR to /etc when CMAKE_INSTALL_PREFIX is /usr AND to Set SYSCONFDIR to /etc when CMAKE_INSTALL_SYSCONFDIR is etc relative to /usr.Dec 27 2019, 6:06 PM
pwojcik edited the summary of this revision. (Show Details)
pwojcik added reviewers: kossebau, alexmerry.
apol added a subscriber: apol.Dec 28 2019, 1:12 AM

Not saying that this patch is wrong, would have to look into it more closely.

But for knsrc files you want to use KDE_INSTALL_KNSRCDIR anyway, which will fall into /usr.

kde-modules/KDEInstallDirs.cmake
376

Why's /opt special?

pwojcik added inline comments.Dec 28 2019, 10:29 AM
kde-modules/KDEInstallDirs.cmake
376

/usr is single special case, but any subdirectory of /opt/ is special by GNUInstallDirs.cmake, following Filesystem Hierarchy Standard.

kossebau added a comment.Dec 28 2019, 2:04 PM

(Just remember that using KDE_INSTALL_KNSRCDIR though needs at least KNewStuffCore from KF 5.57 (hint was missing in API dox, proposing D26248 to fix that).)

Myself also need to reserve time to look closer at it, hoping to sneak in in the next days or week 1 of 2020

Please see to add a unit test also covering this, so this special case is covered.

kde-modules/KDEInstallDirs.cmake
376

Ideally gets a small explaining comment in the code, as this is not directly obvious.

pwojcik updated this revision to Diff 72404.Dec 30 2019, 5:36 PM

Corrected logic, so it is is in line with GNUInstallDirs. Explained in comment. Added tests.

pwojcik added a comment.Feb 4 2020, 6:15 PM

I want to remind about topic.

Revision Contents
Changeset List

Diff 72404

View Options

kde-modules/KDEInstallDirs.cmake

Loading...
View Options

tests/CMakeLists.txt

Loading...
View Options

tests/KDEInstallDirsTest/vars_in_sync/CMakeLists.txt

Loading...