Use runtime install prefix instead of compile time install prefix.
Needs ReviewPublic

Authored by habacker on Sep 6 2017, 1:36 PM.

Details

Diff Detail

Repository
R303 KInit
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
habacker created this revision.Sep 6 2017, 1:36 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptSep 6 2017, 1:36 PM
Restricted Application added a subscriber: Frameworks. · View Herald Transcript
dfaure added a subscriber: dfaure.Sep 10 2017, 1:30 PM
dfaure added inline comments.
src/kdeinit/kinit_win.cpp
205

That's the Qt install prefix. I guess it matches your KF5 install prefix for this patch to work, but it doesn't seem like a universal solution.

Maybe this can use QStandardPaths instead?

Does QStandardPaths provide a parameter to retrieve the runtime binary install location ?
For example if kdeinit5.exe is located in <install-root>/bin it should return <install-root> or if is installed in <install-root> and there is no 'bin' dir in path it should return <install-root> ?
With QLibraryInfo::location() I can set the prefix in the related qt.conf to what I want as shown by the following example.

Install layout without 'bin' sub dir (kdeinit5.exe is located in the <install-root> dir)

  • no qt.conf required
  • or qt.conf contains

[Paths]
Prefix = .
Binaries = .

Install layout with 'bin' sub dir (qt.conf and kdeinit5.exe are located in '<install-root>/bin' dir)

[Paths]
Prefix = ..
Binaries = bin

BTW: From the QStandardPaths documentation it looks that <APPDIR> is the nearest guess with a required patch to use <APPDIR>/.. if the executable is located in a bin subdir and to use <APPDIR> otherwise. Unfortunally I do not see any StandardLocation which would provide this.

dfaure edited edge metadata.Sep 10 2017, 3:39 PM

If all you need is <APPDIR>, you can use QCoreApplication::applicationDirPath().

If all you need is <APPDIR>, you can use QCoreApplication::applicationDirPath().

Did a further look into this stuff and found https://cgit.kde.org/kinit.git/commit/src/kdeinit/kinit_win.cpp?id=6b0ddac715475d7ed36a15f180c755f0f978b7cf. In KDE times there were KStandardDirs::installPath("kdedir") which returns the runtime install prefix. Is a comparable method available in KF5 ?

From your initial reference to QStandardPaths I assume that an universal solution at the time KStandardDirs::installPath("kdedir") has been ported to QStandardPaths was something like adding QStandardPaths::InstallLocation, but seems to be to late now. (TODO for Qt6 ?)

Searching my incomplete KF5 sources for CMAKE_INSTALL_PREFIX show several references to the runtime install prefix (there may be more references)

find -name '*.cpp' -exec grep -Hn CMAKE_INSTALL_PREFIX {} \;
./kconfig/src/kconf_update/kconf_update.cpp:766: path = CMAKE_INSTALL_PREFIX "/" LIB_INSTALL_DIR "/kconf_update_bin/" + script;
./kdelibs4support/src/kdecore/kstandarddirs.cpp:370: return QFile::decodeName(CMAKE_INSTALL_PREFIX "/") + relPath;
./kdelibs4support/src/kdecore/kstandarddirs.cpp:1196: typeInstallPath = QFile::decodeName(CMAKE_INSTALL_PREFIX "/") + QLatin1String("share/applications/");
./kdelibs4support/kde-config.cpp:88: printResult(QFile::decodeName(CMAKE_INSTALL_PREFIX));
./kdelibs4support/autotests/kstandarddirstest.cpp:52: if (kconfig_compilerLocation.startsWith(CMAKE_INSTALL_PREFIX)) {
./kdelibs4support/autotests/kstandarddirstest.cpp:141: return QFile::exists(CMAKE_INSTALL_PREFIX "/bin/kf5-config");
./kdelibs4support/autotests/kstandarddirstest.cpp:441: const QString kdeDataApps = KStandardDirs::realPath(CMAKE_INSTALL_PREFIX "/share/applications/");
./kdelibs4support/autotests/kstandarddirstest.cpp:484: const QString kdeDataApps = KStandardDirs::realPath(CMAKE_INSTALL_PREFIX "/share/applications/");
./sonnet/src/plugins/aspell/aspellclient.cpp:36: // A generated config-aspell.h (or config-kspell.h, if shared) should be added then, to define CMAKE_INSTALL_PREFIX
./sonnet/src/plugins/aspell/aspellclient.cpp:37: return QLatin1String(CMAKE_INSTALL_PREFIX ASPELL_DATA_ROOT);
./kinit/src/kdeinit/kinit.cpp:631: CMAKE_INSTALL_PREFIX "/" LIB_INSTALL_DIR "/");
./kinit/src/kdeinit/kinit.cpp:1541: QString path = QFile::decodeName(CMAKE_INSTALL_PREFIX "/" LIB_INSTALL_DIR "/") + lib;
./kxmlgui/src/kbugreport.cpp:553: command = QFile::decodeName(CMAKE_INSTALL_PREFIX "/" KF5_LIBEXEC_INSTALL_DIR "/ksendbugmail");

so I would say yes in KF5 there is the need to have a common function to get the runtime install prefix [1] instead of implementing it on every mentioned place [2].

Using QCoreApplication::applicationDirPath() falls in category [2] while using QLibraryInfo::location falls into category[1] because from my experience every KF5 application on Windows needs a qt.conf for finding correct pathes.
Unfortunally using QLibraryInfo::location on linux would change install prefix which may not work. This indicates that a replacement for KStandardDirs::installPath("kdedir") is required and leads to the question how the function should be named and where to place it ? In KDE4 times is was in kdecore . For KF5 should it be placed in kcoreaddons ?

This add kcoreaddons as a runtime dependency to all libraries where the above mentioned code locations belongs to. To avoid the runtime dependency it would be possible to define the function as public inline method in kcoreaddons and to add kcoreaddons as development only dependency tp related libraries. The implementation may use QLibraryInfo::location() on Windows and CMAKE_INSTALL_PREFIX on linux.

Any objections or a better solution ?

This is also a duplicate as mentioned on the other reviews?

habacker marked an inline comment as done.Dec 18 2017, 10:21 AM
habacker added inline comments.
src/kdeinit/kinit_win.cpp
205

not in KF5, see comments in this bug

QStandardPaths::InstallLocation cannot ever exist, if you mean by that "the install location for KF5" or "the install location for a KF5-based application". How would that work? There could be (at least) 3 different install locations, at least on Unix: one for Qt, one for KF5, and one for the app. Qt only knows about the first one, currently.

But OK, this code here is Windows-specific, and there we control the install layout, so if Qt, KF5 and KF5-based apps will always be installed into the same prefix, then this patch is OK. Are we sure this will always be the case though?

The rest of your comment is about CMAKE_INSTALL_PREFIX usage everywhere. I am very opposed to adding a dependency on kcoreaddons everywhere, even a header-only dependency, it breaks the whole point of independently reusable frameworks for commercial application developers out there. However many frameworks already depend on kcoreaddons so we could use a kcoreaddons method there, and duplicate it in the other ones -- after all we're talking about a 5 liner with #ifdef / return / #else / return / #endif.

There is a completely different solution: adding to Qt a method that takes a function address and returns the full path to the binary (shared lib or executable) containing that function. Volker provided us with an implementation for this: https://github.com/KDAB/GammaRay/blob/master/common/selflocator.cpp
Then any framework could easily find where it is installed, by calling this with the address of one of its own functions.
This would be a much cleaner solution, because it wouldn't rely on Windows setups to install everything into the same dir as Qt, like this patch does.

src/kdeinit/kinit_win.cpp
205

Which bug? you mean this review request? I don't see a bug reference.

habacker marked an inline comment as done.Apr 9 2018, 8:06 AM

There is a completely different solution: adding to Qt a method that takes a function address and returns the full path to the binary (shared lib or executable) containing that function.
This would be a much cleaner solution, because it wouldn't rely on Windows setups to install everything into the same dir as Qt, like this patch does.

This has its advantage - How long would it take for this feature to be available in Qt in the case someone submits a related feature request now ?