----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/129273/#review100355 -----------------------------------------------------------
Ship it! Ship It! - Aleix Pol Gonzalez On Oct. 28, 2016, 1:55 p.m., Harald Sitter wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/129273/ > ----------------------------------------------------------- > > (Updated Oct. 28, 2016, 1:55 p.m.) > > > 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 > >