Review Request 114336: Install all includes in a KF5 dir

[Stephen Kelly]

Aurélien Gâteau wrote:

> Le lundi 9 décembre 2013 21:56:52 Stephen Kelly a écrit :
>> Aurélien Gâteau wrote:
>> > target_include_directories(${library} INTERFACE
>> > "$<INSTALL_INTERFACE:
>> > ${CMAKE_INSTALL_PREFIX}/${${library}_INCLUDE_INSTALL_DIR}>"
>> > )
>> 
>> Note that this makes the resulting package non-relocatable. If using
>> CMake 3.0.0, you can specify a relative path here. With 2.8.12, you
>> should use $<INSTALL_PREFIX> instead of ${CMAKE_INSTALL_PREFIX} so that
>> the relative path stays relative.
> 
> Good to know, I must admit I cargo-culted the if(IS_ABSOLUTE) snippet from
> existing code (for example tier1/kwidgetaddons/src/CMakeLists.txt uses
> it). 

:/

Ill-advised code propagates from cargo-culting. It is a bad idea. If you 
don't understand, ask. ECM also has another problem I was trying to help mck 
see on IRC. He started with what he thought was the solution and didn't 
understand the problem at hand. I filed 

 http://public.kitware.com/Bug/view.php?id=14634

and I agree with Kent. I'm not sure it remains an issue after the split, but 
it might be.

> Since we are tied to CMake 2.8.12, the macro would be:
> 
> macro ecm_library_set_include_prefix(library include_prefix)
>   set(${library}_INCLUDE_INSTALL_DIR
>   ${INCLUDE_INSTALL_DIR}/${include_prefix})
> 
>   if(IS_ABSOLUTE "${INCLUDE_INSTALL_DIR}")
>     target_include_directories(${library} INTERFACE
>       "$<INSTALL_INTERFACE:${${library}_INCLUDE_INSTALL_DIR}>"
>     )
>   else()
>     target_include_directories(${library} INTERFACE
>       "$<INSTALL_INTERFACE:
>        $<INSTALL_PREFIX>/${${library}_INCLUDE_INSTALL_DIR}>"
>     )
>   endif()
> endmacro()
> 
> Is this correct?

What about the name? You specify that when invoking the macro?

The target_include_directories parts of your macro (ie most of it) are 
obsoleted by 

diff --git a/kde-modules/KDEInstallDirs.cmake b/kde-
modules/KDEInstallDirs.cmake
index d94adcf..126185b 100644
--- a/kde-modules/KDEInstallDirs.cmake
+++ b/kde-modules/KDEInstallDirs.cmake
@@ -174,7 +174,7 @@ _set_fancy(DBUS_SYSTEM_SERVICES_INSTALL_DIR 
"${SHARE_INSTALL_PREFIX}/dbus-1/syst
 set(INSTALL_TARGETS_DEFAULT_ARGS  RUNTIME DESTINATION "${BIN_INSTALL_DIR}"
                                   LIBRARY DESTINATION "${LIB_INSTALL_DIR}"
                                   ARCHIVE DESTINATION "${LIB_INSTALL_DIR}" 
COMPONENT Devel
-                                  INCLUDES DESTINATION 
"${INCLUDE_INSTALL_DIR}"
+                                  INCLUDES DESTINATION 
"${INCLUDE_INSTALL_DIR}/KF5/$<TARGET_PROPERTY:NAME>"
 )
 
 
when CMake 3.0.0 is depended on.


Thanks,

Steve.