Skip to content
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

Use unique name for exported targets and avoid exporting binary targets #396

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

Conversation

luca-della-vedova
Copy link
Member

@luca-della-vedova luca-della-vedova commented Dec 3, 2024

Bug fix

Fixed bug

The libraries in rmf_ros2 cannot be linked to by using ament_target_dependencies, forcing downstream users to use target_link_libraries instead.

Fix applied

Based on the guidelines in the ament_cmake docs, specifically the sample library:

install(
  TARGETS my_library
  EXPORT export_${PROJECT_NAME}
  LIBRARY DESTINATION lib
  ARCHIVE DESTINATION lib
  RUNTIME DESTINATION bin
)

ament_export_targets(export_${PROJECT_NAME} HAS_LIBRARY_TARGET)
ament_export_dependencies(some_dependency)

The exported target has a unique name rather than the library name, this is also explicitly suggested in a later paragraph that says the following:

The EXPORT notation of the install call requires additional attention: It installs the CMake files for the my_library target. It must be named exactly the same as the argument in ament_export_targets. To ensure that it can be used via ament_target_dependencies, it should not be named exactly the same as the library name, but instead should have a prefix like export_ (as shown above).

Furthermore, we were exporting binaries and targets and downstream users using ament_target_dependencies would get cryptic CMake failures such as:

CMake Error at /opt/ros/jazzy/share/ament_cmake_target_dependencies/cmake/ament_target_dependencies.cmake:146 (target_link_libraries):
  Target "[...]" links to:

    rmf_fleet_adapter::read_only

  but the target was not found.  Possible reasons include:

    * There is a typo in the target name.
    * A find_package call is missing for an IMPORTED target.
    * An ALIAS target is missing.

Call Stack (most recent call first):
  CMakeLists.txt:28 (ament_target_dependencies)


CMake Generate step failed.  Build files cannot be regenerated correctly.

So I split the installation step to make sure we only export targets for the library and not for the executable, again following the documentation.

These changes made it possible to compile downstream applications that link to rmf_fleet_adapter (I haven't tested the other packages but applied the same fix for consistency)

Signed-off-by: Luca Della Vedova <[email protected]>
@luca-della-vedova luca-della-vedova marked this pull request as draft December 3, 2024 10:24
Signed-off-by: Luca Della Vedova <[email protected]>
@luca-della-vedova luca-della-vedova marked this pull request as ready for review December 3, 2024 10:39
@luca-della-vedova luca-della-vedova changed the title Use unique name for exported targets Use unique name for exported targets and avoid exporting binary targets Dec 3, 2024
Copy link
Contributor

@mxgrey mxgrey left a comment

Choose a reason for hiding this comment

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

Thanks for the detailed explanation and the fixes.

This seems like an odd idiosyncrasy of ament to me (.. I thought cmake normally gives the export the same name as the target..?), but I'm sure there's a perfectly logical reason for it that I'm failing to appreciate because my understanding of ament_cmake isn't deep enough.

I'm not sure if there's any risk of this being disruptive for anyone who isn't using ament_target_dependencies. I think we'll need to make sure we don't backport this since I don't know if it could break downstream build systems.

@Yadunund
Copy link
Member

Yadunund commented Dec 5, 2024

One thing to note is that ament_target_dependencies is actually being deprecated to encourage the use of target_link_libraries.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Inbox
Development

Successfully merging this pull request may close these issues.

3 participants