-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Install target generates a clean CMake package (+ Catkin dependency is now optional) #215
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
This commit allow the user to export the library that is compatible witha find_package() call.
install() call on serial library was missing a proper ARCHIVE parameter, causing Unix build to fail.
|
why not to squash all commit to one? |
CMakeLists.txt
Outdated
|
|
||
| ## Add serial library | ||
| add_library(${PROJECT_NAME} ${serial_SRCS}) | ||
| add_library(${PROJECT_NAME} STATIC ${serial_SRCS}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why forcing static?
better would be to use -DBUILD_SHARED_LIBS=OFF (see https://cmake.org/cmake/help/v3.0/variable/BUILD_SHARED_LIBS.html)
|
Can this pull request be fixed and merged? Having Catkin dependency as optional is a long time request |
|
|
||
| ## Include headers | ||
| include_directories(include) | ||
| target_include_directories(${PROJECT_NAME} PUBLIC $<BUILD_INTERFACE:${CMAKE_SOURCE_DIR}/include>) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| target_include_directories(${PROJECT_NAME} PUBLIC $<BUILD_INTERFACE:${CMAKE_SOURCE_DIR}/include>) | |
| target_include_directories(${PROJECT_NAME} PUBLIC $<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/include>) |
The use of CMAKE_CURRENT_SOURCE_DIR is problematic when the project is used inside a bigger project via add_subdirectory , for example when using the FetchContent CMake module. Use of CMAKE_CURRENT_SOURCE_DIR should remove any ambiguity.
Install target now generate a clean CMake package :
libfolder that contains the static library (debug and release)includefolder that contains the *.hcmakefolder that contains all the cmake related files (exported targets + version file)This allow the package to be found by a
find_package()call if you setserial_DIRvar to the path ofcmakefolder.This PR also disable Catkin dependency by default, you can restore it by setting
USE_CATKINoption to ON.Also the option
BUILD_SAMPLEallow you to build or not the sample (default is ON).This is somewhat #133 is trying to do while keeping Catkin dependency as an option if needed.