Skip to content

feat: add option to export libraries as cmake targets #455

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.

Sign up for GitHub

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

Open
wants to merge 2 commits into
base: rolling
Choose a base branch
from
Open

feat: add option to export libraries as cmake targets #455

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
95be8d8
feat: add option to export libraries as cmake targets
VRichardJP May 29, 2023
d9db853
Merge branch 'rolling' into ament_ex_backport
VRichardJP Jul 23, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions ament_cmake_auto/cmake/ament_auto_add_library.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,10 +69,12 @@ macro(ament_auto_add_library target)
if(EXISTS "${CMAKE_CURRENT_SOURCE_DIR}/include")
if(ARG_INTERFACE)
target_include_directories("${target}" INTERFACE
"${CMAKE_CURRENT_SOURCE_DIR}/include")
$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/include>
$<INSTALL_INTERFACE:include>)
else()
target_include_directories("${target}" PUBLIC
"${CMAKE_CURRENT_SOURCE_DIR}/include")
$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/include>
$<INSTALL_INTERFACE:include>)
endif()
endif()
# link against other libraries of this package
Expand Down
40 changes: 29 additions & 11 deletions ament_cmake_auto/cmake/ament_auto_package.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@
#

macro(ament_auto_package)
cmake_parse_arguments(_ARG_AMENT_AUTO_PACKAGE "INSTALL_TO_PATH" "" "INSTALL_TO_SHARE" ${ARGN})
cmake_parse_arguments(_ARG_AMENT_AUTO_PACKAGE "INSTALL_TO_PATH;EXPORT_LIBRARY_TARGETS" "" "INSTALL_TO_SHARE" ${ARGN})
# passing all unparsed arguments to ament_package()

# export all found build dependencies which are also run dependencies
Expand All @@ -66,22 +66,40 @@ macro(ament_auto_package)
endif()

# export and install all libraries
if(NOT ${PROJECT_NAME}_LIBRARIES STREQUAL "")
set(without_interfaces "")
foreach(library_name ${${PROJECT_NAME}_LIBRARIES})
get_target_property(library_type ${library_name} TYPE)
if(NOT "${library_type}" STREQUAL "INTERFACE_LIBRARY")
list(APPEND without_interfaces ${library_name})
endif()
endforeach()
if (_ARG_AMENT_AUTO_PACKAGE_EXPORT_LIBRARY_TARGETS)
# will export all libraries using modern cmake targets
list(APPEND ${PROJECT_NAME}_TARGETS "${${PROJECT_NAME}_LIBRARIES}")
else()
# old style library export
if(NOT ${PROJECT_NAME}_LIBRARIES STREQUAL "")
set(without_interfaces "")
foreach(library_name ${${PROJECT_NAME}_LIBRARIES})
get_target_property(library_type ${library_name} TYPE)
if(NOT "${library_type}" STREQUAL "INTERFACE_LIBRARY")
list(APPEND without_interfaces ${library_name})
endif()
endforeach()

ament_export_libraries(${without_interfaces})
ament_export_libraries(${without_interfaces})
install(
TARGETS ${without_interfaces}
ARCHIVE DESTINATION lib
LIBRARY DESTINATION lib
RUNTIME DESTINATION bin
)
endif()
endif()

# export all targets for downstream packages
if(NOT "${${PROJECT_NAME}_TARGETS}" STREQUAL "")
install(
TARGETS ${without_interfaces}
TARGETS ${${PROJECT_NAME}_TARGETS}
EXPORT ${PROJECT_NAME}Targets
ARCHIVE DESTINATION lib
LIBRARY DESTINATION lib
RUNTIME DESTINATION bin
)
ament_export_targets(${PROJECT_NAME}Targets)
endif()
Comment on lines +93 to 103
Copy link
Contributor

@wjwwood wjwwood Sep 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@VRichardJP I know you said you don't have the ability to test this and that you've moved on, but if you have the time and context still, does this conditional install need to be outside of the above if-else?

As I read the code, one thing that definitely changed is that we try to install ${${PROJECT_NAME}_TARGETS} even if you don't use EXPORT_LIBRARY_TARGETS. If we were able to move this block up into the if (_ARG_AMENT_AUTO_PACKAGE_EXPORT_LIBRARY_TARGETS) clause, then this change becomes a very safe one (EXPORT_LIBRARY_TARGETS gives you a new behavior and without it you get the old behavior as the default).

But if you think it needs to be outside the clause then I'll have to study it closer. I don't think anything else is using ${${PROJECT_NAME}_TARGETS}, but I need to spend more time on it.

Copy link
Contributor Author

@VRichardJP VRichardJP Sep 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand your point.

As I look back at the code, I think I have implemented it this way because it is simply more future proof. If you consider that more scripts may add targets to ${PROJECT_NAME}_TARGETS in the future (e.g. binaries as target too), and that targets should only be exported once (if any), then I think it makes more sense to keep the target exports separate.

Basically, this PR adds support for target export, and uses that new feature to implement a new export library as target option.


# install all executables
Expand Down