Review Request 129273: Remove /KF5 include directory injection

[Aleix Pol Gonzalez]

-----------------------------------------------------------
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
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20161028/24d52e95/attachment.html>