Fix and simplify cmake package config #596
Conversation
CMakeLists.txt
Outdated
| if(BUILD_SHARED_LIBS) | ||
| add_library(${Libname} SHARED) | ||
| else() | ||
| add_library(${Libname} STATIC) | ||
| endif() | ||
| add_library(${Libname}) |
There was a problem hiding this comment.
This was just redundant as add_library already does this:
If no is given the default is STATIC or SHARED based on the value of the BUILD_SHARED_LIBS variable.
| target_include_directories(${Libname} | ||
| PUBLIC | ||
| $<BUILD_INTERFACE:${PROJECT_SOURCE_DIR}/include> | ||
| $<INSTALL_INTERFACE:include>) |
There was a problem hiding this comment.
The included directory is different for installed target
examples/CMakeLists.txt
Outdated
| @@ -4,7 +4,7 @@ if(${CMAKE_SOURCE_DIR} STREQUAL ${CMAKE_CURRENT_SOURCE_DIR}) | |||
| project(zenohpico_examples LANGUAGES C) | |||
| include(../cmake/helpers.cmake) | |||
| set_default_build_type(Release) | |||
| configure_include_project(ZENOHPICO zenohpico zenohpico::static ".." zenohpico "https://github.com/eclipse-zenoh/zenoh-pico" "") | |||
| configure_include_project(ZENOHPICO zenohpico zenohpico ".." zenohpico "https://github.com/eclipse-zenoh/zenoh-pico" "") | |||
There was a problem hiding this comment.
This actually fixes the build if building the examples as the root project. When including the project using add_subdirectory or FetchContent the library target will be named zenohpico.
The only small problem is that the target name for a find_packaged library will be zenohpico::zenohpico and this macro does not take the target namespace into account.
|
Hello @bjsowa and thanks for your contribution, we'll look into it after the end of our release cycle so probably next week. |
|
PR missing one of the required labels: breaking-change bug dependencies documentation enhancement new feature internal |
DenisBiryukov91
added
enhancement
|
@bjsowa We still want to expose variables corresponding to enabled/disabled features of pico (this is needed mostly for cpp to decide which tests/examples to build) and the alias zenohpico::lib. I made corresponding pr to your branch. |
…ake_package_config keep feature defines in PackageConfig.cmake.in
| @@ -4,7 +4,7 @@ if(${CMAKE_SOURCE_DIR} STREQUAL ${CMAKE_CURRENT_SOURCE_DIR}) | |||
| project(zenohpico_examples LANGUAGES C) | |||
| include(../cmake/helpers.cmake) | |||
| set_default_build_type(Release) | |||
| configure_include_project(ZENOHPICO zenohpico zenohpico::static ".." zenohpico "https://github.com/eclipse-zenoh/zenoh-pico" "") | |||
| configure_include_project(ZENOHPICO zenohpico zenohpico::lib ".." zenohpico "https://github.com/eclipse-zenoh/zenoh-pico" "") | |||
There was a problem hiding this comment.
This should now work regardless of whether zenohpico is included using find_package or add_subdirectory as both define zenohpico::lib target
| add_library(${Libname} STATIC) | ||
| endif() | ||
| add_library(${Libname}) | ||
| add_library(zenohpico::lib ALIAS ${Libname}) |
There was a problem hiding this comment.
@DenisBiryukov91 I assume there was no reason to put the alias definition inside add_definition function. Let me know if I'm wrong.
There was a problem hiding this comment.
Yes, sorry, this was a typo.
The current package config template seems to be copied from zenoh-c and does not really work as the CMakeLists does not create STATICLIB or DYLIB variables.
The solution I propose is much simpler. It just exports the
zenohpicotarget which contains the public include directories and compile definitions, instead of creating an imported library target.Dependent projects can just
find_packagethis project and linkzenohpico::zenohpicotarget to their executable.