Skip to content

Conversation

@traversaro
Copy link
Contributor

Description

Since CMake 3.14, the install(TARGETS signature had default install destination that work well, without requiring for the user the need to set any boilerplate (see https://cmake.org/cmake/help/v3.14/command/install.html#installing-targets). As since #969 the minimum version of CMake is 3.16, we can remove all the DESTINATION arguments from the library install(TARGETS call, and the libraries will still be installed in the correct location.

Note that the install(TARGETS call that install the node executable will still keep its DESTINATION arguments, as in that case the DESTINATION is non-standard.

Is this user-facing behavior change?

The generated CMakeLists.txt will change from containing:

install(
  TARGETS <cpp_library_name>
  EXPORT export_<project_name>
  ARCHIVE DESTINATION lib
  LIBRARY DESTINATION lib
  RUNTIME DESTINATION bin)

to:

install(
  TARGETS <cpp_library_name>
  EXPORT export_<project_name>
)

while keeping the same behavior, reducing the generated boilerplate.

Did you use Generative AI?

No.

Additional Information

Triggered by ros-misc-utilities/apriltag_detector#7 .

Copy link
Collaborator

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

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

lgtm but can we also apply the same change to

ARCHIVE DESTINATION lib
LIBRARY DESTINATION lib
RUNTIME DESTINATION bin
?

@traversaro
Copy link
Contributor Author

lgtm but can we also apply the same change to

ARCHIVE DESTINATION lib
LIBRARY DESTINATION lib
RUNTIME DESTINATION bin

?

That file still have a minimum CMake version of 3.8, see

cmake_minimum_required(VERSION 3.8)
. If that can also be raised to 3.16, then I think it make sense to do the same change there.

@fujitatomoya
Copy link
Collaborator

Ah I see then we can go ahead with this change for now.

note: i think i am gonna have to to align the minimum CMake version for all packages together, and then apply this same change with them.

@fujitatomoya
Copy link
Collaborator

Pulls: #1056
Gist: https://gist.githubusercontent.com/fujitatomoya/e32bd71a2bffdf25c30f19a52be5e838/raw/c3728ee3998630fa4f2f752dff592022b6f530ae/ros2.repos
BUILD args: --packages-above-and-dependencies ros2pkg
TEST args: --packages-above ros2pkg
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/16217

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

@ahcorde ahcorde merged commit 6652d53 into ros2:rolling Jun 16, 2025
2 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants