Review Request 114336: Install all includes in a KF5 dir

[Aurélien Gâteau]

Le mardi 10 décembre 2013 10:02:18 Stephen Kelly a écrit :
> 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.

That's sort of what I am doing now :)

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

For a 'K'-prefixed framework, one would use it like this:

  ecm_library_set_include_prefix(KFoo KF5)

and then install headers with:

  install(FILES Foo.h DESTINATION ${KFoo_INCLUDE_INSTALL_DIR})


For a non-prefixed framework, one would use it like this:

  ecm_library_set_include_prefix(Bar KF5)

and then install headers with:

  install(FILES Bar.h DESTINATION ${Bar_INCLUDE_INSTALL_DIR}/Bar)

ecm_library_set_include_prefix() is not called with KF5/Bar because in the 
case of a non-prefixed framework, we want users to include headers with

  #include <Bar/Bar.h>

So INSTALL_INTERFACE must contain include/KF5, not include/KF5/Bar.


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

Sounds good, we will be able to simplify the macro at this point then.

Aurélien