----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/129273/ -----------------------------------------------------------
Review request for KDE Frameworks and Aleix Pol Gonzalez. Repository: kwindowsystem Description ------- This is ancient code that is outright wrong most of the time and at best just incredibly unnecessary. It is also not present in the great majority of frameworks due to this. Its wrongness comes from the fact that it hardcodes the installation path, which breaks relocatability of the KF5 tree as it will always attempt to find the include dir $PREFIX/KF5 (e.g. /usr/include/KF5), which may or may not exist given that the tree was relocated. Worse yet, in a cross-building scenario we maybe for example build on ARM and install to /usr but for cross building take the entire ARM tree and shift it into /arm/usr/. If we then crossbuild on that tree the bogus include list in this framework will make sure that we always search in /usr/include/KF5 and thus potentially load a !ARM header simply because the relevant ARM header was not installed etc.. Similarly of course a build in $HOME can pick up /usr/include/KF5 headers because the home ones are missing, causing unexpected results. This happens whenever the KDE_INSTALL_INCLUDEDIR_KF5 var is absolute, which it usually is. On top of all that the premise of the code in question is flawed. It seeks to add $PREFIX/$KF5INCLUDES to the search paths (e.g. /usr/include/KF5). This is unnecessary because the target itself is properly installed via cmake's install(TARGETS ... EXPORT ...) function [1]. This function has smart functionality built in which will add the passed INCLUDES destination to the INTERFACE_INCLUDE_DIRECTORIES property of the targets (i.e. what the useless code wants to do) [2]. So what happens is that we install the target to the KF5 locations, which has "include/KF5" as INCLUDES location, thus causing the correct path to be added to the includes list of the Targets.cmake file. In particular thanks to more internal magic in cmake it will do so with automatically resolved root paths such that the installed tree is relocatable and able to relatively find the other KF5/* headers. So it does what the code in question wants to do, just correctly. Since cmake automatically takes care of injecting $prefix/include/KF5 we can simply get rid of the wrong custom inejection code. This makes the generated cmake file find the correct include/KF5/ directory and stops it from always expecting a /usr/include/KF5/ directory to be present. [1] https://cmake.org/cmake/help/v3.0/command/install.html [2] https://cmake.org/cmake/help/v3.0/command/install.html > The INCLUDES DESTINATION specifies a list of directories which will be > added to the INTERFACE_INCLUDE_DIRECTORIES target property of the > <targets> when exported by the install(EXPORT) command. Diffs ----- src/CMakeLists.txt 1598b4f80d1b7db7b28af5bc876d5c1091bcabb3 Diff: https://git.reviewboard.kde.org/r/129273/diff/ Testing ------- Targets.cmake contents without patch: ``` set_target_properties(KF5::WindowSystem PROPERTIES INTERFACE_INCLUDE_DIRECTORIES "${_IMPORT_PREFIX}/include/KF5/KWindowSystem;/usr/include;/usr/include;/usr/include/KF5;${_IMPORT_PREFIX}/include/KF5" INTERFACE_LINK_LIBRARIES "Qt5::Widgets" ) ``` Targets.cmake contents with patch: ``` set_target_properties(KF5::WindowSystem PROPERTIES INTERFACE_INCLUDE_DIRECTORIES "${_IMPORT_PREFIX}/include/KF5/KWindowSystem;/usr/include;/usr/include;${_IMPORT_PREFIX}/include/KF5" INTERFACE_LINK_LIBRARIES "Qt5::Widgets" ) ``` FTR: the static /usr/include thingies come from manually pulling in X11 paths, which is technically still wrong but out of scope and somewhat harder to resolve correctly. Thanks, Harald Sitter