D7706: Use runtime install prefix instead of compile time install prefix.

[Ralf Habacker]

habacker marked an inline comment as done.
habacker added a comment.


  In D7706#181225 , @dfaure wrote:
  
  > 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 ?

REPOSITORY
  R303 KInit

REVISION DETAIL
  https://phabricator.kde.org/D7706

To: habacker, dfaure, ltoscano, bcooksley
Cc: dfaure, #frameworks, michaelh, ngraham, bruns

D7706: Use runtime install prefix instead of compile time install prefix.

[David Faure]

dfaure added a comment.


  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.

INLINE COMMENTS

> habacker wrote in kinit_win.cpp:205
> not in KF5, see comments in this bug

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

REPOSITORY
  R303 KInit

REVISION DETAIL
  https://phabricator.kde.org/D7706

To: habacker, dfaure, ltoscano, bcooksley
Cc: dfaure, #frameworks

D7706: Use runtime install prefix instead of compile time install prefix.

[Ralf Habacker]

habacker marked an inline comment as done.
habacker added inline comments.

INLINE COMMENTS

> dfaure wrote in 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?

not in KF5, see comments in this bug

REPOSITORY
  R303 KInit

REVISION DETAIL
  https://phabricator.kde.org/D7706

To: habacker, dfaure, ltoscano, bcooksley
Cc: dfaure, #frameworks

D7706: Use runtime install prefix instead of compile time install prefix.

[Ben Cooksley]

bcooksley added a comment.


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

REPOSITORY
  R303 KInit

REVISION DETAIL
  https://phabricator.kde.org/D7706

To: habacker, dfaure, ltoscano, bcooksley
Cc: dfaure, #frameworks

D7706: Use runtime install prefix instead of compile time install prefix.

[Ralf Habacker]

habacker added reviewers: ltoscano, bcooksley.

REPOSITORY
  R303 KInit

REVISION DETAIL
  https://phabricator.kde.org/D7706

To: habacker, dfaure, ltoscano, bcooksley
Cc: dfaure, #frameworks

D7706: Use runtime install prefix instead of compile time install prefix.

[Ralf Habacker]

habacker added a comment.


  > If all you need is , 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 ?

REPOSITORY
  R303 KInit

REVISION DETAIL
  https://phabricator.kde.org/D7706

To: habacker, dfaure
Cc: dfaure, #frameworks

D7706: Use runtime install prefix instead of compile time install prefix.

[David Faure]

dfaure added a comment.


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

REPOSITORY
  R303 KInit

REVISION DETAIL
  https://phabricator.kde.org/D7706

To: habacker, dfaure
Cc: dfaure, #frameworks

D7706: Use runtime install prefix instead of compile time install prefix.

[Ralf Habacker]

habacker added a reviewer: dfaure.

REPOSITORY
  R303 KInit

REVISION DETAIL
  https://phabricator.kde.org/D7706

To: habacker, dfaure
Cc: dfaure, #frameworks

D7706: Use runtime install prefix instead of compile time install prefix.

[Ralf Habacker]

habacker added a comment.


  Does QStandardPaths provide a parameter to retrieve the runtime binary 
install location ? 
  For example if kdeinit5.exe is located in /bin it should return 
 or if is installed in  and there is no 'bin' dir 
in path it should return  ?
  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  
 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 
'/bin' dir)
  
  [Paths]
  Prefix = ..
  Binaries = bin
  
  BTW: From the QStandardPaths documentation it looks that  is the 
nearest guess with a required patch to use /..  if the executable is 
located in a  bin subdir and to use  otherwise. Unfortunally I do not 
see any StandardLocation which would provide this.

REPOSITORY
  R303 KInit

REVISION DETAIL
  https://phabricator.kde.org/D7706

To: habacker
Cc: dfaure, #frameworks

D7706: Use runtime install prefix instead of compile time install prefix.

[David Faure]

dfaure added inline comments.

INLINE COMMENTS

> kinit_win.cpp:205
> +{
> +return QLibraryInfo::location(QLibraryInfo::PrefixPath);
> +}

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?

REPOSITORY
  R303 KInit

REVISION DETAIL
  https://phabricator.kde.org/D7706

To: habacker
Cc: dfaure, #frameworks

D7706: Use runtime install prefix instead of compile time install prefix.

[Ralf Habacker]

habacker created this revision.
Restricted Application added a project: Frameworks.
Restricted Application added a subscriber: Frameworks.

REPOSITORY
  R303 KInit

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D7706

AFFECTED FILES
  src/kdeinit/kinit_win.cpp

To: habacker
Cc: #frameworks