Review Request 129273: Remove /KF5 include directory injection

[Harald Sitter]

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

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