Use unique name for exported targets and avoid exporting binary targets

[luca-della-vedova]

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)

[mxgrey]

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]

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

[luca-della-vedova]

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

I'd propose to fully transition out of it once there is an official notice on the ROS 2 side, I noticed that it's still not 100% official yet