Is ament_target_dependencies() still necessary?

sloretz commented 3 years ago

I was going to open an issue to add the PRIVATE keyword to ament_taret_dependencies(), but instead I'm wondering if it is still necessary now that ROS 2 packages offer modern CMake targets.

A lot of code uses ament_target_dependencies() to make a target in one package depend on everything offered by another package, whether that uses old-style standard CMake variables or modern CMake targets.

ament_target_dependencies(${PROJECT_NAME}
  rcl_logging_interface
  rcpputils
  rcutils
  spdlog
)

ament_target_dependencies() propagates include directories, compiler definitions, compiler flags, etc. However, target_link_libraries() with modern CMake targets should do all of that too. As far as I can tell, I can use target_link_library() calls with the targets that are now being exported from those packages.

target_link_libraries(${PROJECT_NAME} PRIVATE
  rcpputils::rcpputils
  rcutils::rcutils
  spdlog::spdlog)
target_link_libraries(${PROJECT_NAME} PUBLIC
  rcl_logging_interface::rcl_logging_interface)

If I wanted to link against every target in an ament_cmake package, it looks like I could do:

target_link_libraries(${PROJECT_NAME} PRIVATE
  ${rcpputils_TARGETS}
  ${rcutils_TARGETS}
  spdlog::spdlog)
target_link_libraries(${PROJECT_NAME} PUBLIC
  ${rcl_logging_interface_TARGETS})

If ament_target_dependencies() is around just for convenience, it seems like the standard CMake way is equally convenient. What other value does ament_target_dependencies() provide? Can it be deprecated?

dirk-thomas commented 3 years ago

Can it be deprecated?

Do you implicitly want to deprecate packages which use old-style standard CMake variable and instead only support packages which use modern CMake targets?

hidmic commented 3 years ago

Dirk has a point. At the same time, transitioning towards modern CMake would be good.

Perhaps, we can keep ament_target_dependencies() around but add a deprecation notice. That means we won't be able to actually remove this until H-Turtle, but at least package authors will have an incentive to clean things up starting with Galactic. WDYT?

dirk-thomas commented 3 years ago

I think any transition needs to happen from the bottom up (similar to the transition from rosbuild to catkin in ROS 1). That being said deprecating ament_target_dependencies() sounds unrealistic to me.

Maybe an option would be to add a deprecation warning when the function is being used only with dependencies which do export modern CMake targets at which point the function could become a no-op. That way the deprecation warnings happen incrementally and its usage can be removed in each package as soon as it is not necessary anymore.

hidmic commented 3 years ago

Maybe an option would be to add a deprecation warning when the function is being used only with dependencies which do export modern CMake targets at which point the function could become a no-op. That way the deprecation warnings happen incrementally and its usage can be removed in each package as soon as it is not necessary anymore.

That's an option, but wouldn't it create package clusters that can stick to the old ways by virtue of being inactive? Though I guess we have to deal with that in the long run. Maybe we should better document which one's the right way ™ of doing things before even considering deprecating the older (but vastly more common) ones.

dirk-thomas commented 3 years ago

wouldn't it create package clusters that can stick to the old ways by virtue of being inactive?

There would always be at least one package at the bottom of the dependency chain which would get the deprecation warning. That package would need to be updated in order to allow downstream packages to transition to the modern style.

Obviously this incremental approach could take a long time. I just doubt that you can force all packages to transition within a single release cycle.

sloretz commented 3 years ago

Do you implicitly want to deprecate packages which use old-style standard CMake variable and instead only support packages which use modern CMake targets?

Hmm I guess that is the implication. Switching to target_link_libraries() does require modern CMake targets for all one's dependencies.

That being said deprecating ament_target_dependencies() sounds unrealistic to me.

What about something softer than a deprecation than - like recommending using target_link_libraries() if possible in the documentation of ament_target_dependencies() and using that in ROS 2 examples?

I think any transition needs to happen from the bottom up (similar to the transition from rosbuild to catkin in ROS 1).

I think it's safe to update packages in the middle of a chain as long as it's dependencies using standard CMake variables aren't publicly exposed, so that they don't need to be propagated downstream.

Maybe an option would be to add a deprecation warning when the function is being used only with dependencies which do export modern CMake targets at which point the function could become a no-op. That way the deprecation warnings happen incrementally and its usage can be removed in each package as soon as it is not necessary anymore.

That sounds like it might be useful as an experiment to see how much remaining standard variable usage there is.

dirk-thomas commented 3 years ago

I think it's safe to update packages in the middle of a chain as long as it's dependencies using standard CMake variables aren't publicly exposed, so that they don't need to be propagated downstream.

How does that work - a package not exposing standard CMake variables (and no CMake targets I assume) but somehow being used by that "package in the middle"?

sloretz commented 3 years ago

How does that work - a package not exposing standard CMake variables (and no CMake targets I assume) but somehow being used by that "package in the middle"?

Hmm I don't understand the question - I wonder if we're talking about different cases. What case has a package neither providing CMake variables nor CMake targets?

I was talking about a package using standard CMake variables from another package, but not needing to make that info available to downstream packages. Say alice and bob export CMake targets, while charlie has some info only standard CMake variables. Say alice depends on bob and bob depends on charlie. However, bob only uses charlie internally. Bob might use ament_target_dependencies(bob charlie) and get info from CMake variables, but alice should be able to target_link_libraries(... bob::bob) because alice didn't need any info from charlies CMake variables in order to build.

dirk-thomas commented 3 years ago

I was talking about a package using standard CMake variables from another package, but not needing to make that info available to downstream packages.

Such a case means that there is no need to export anything.

Bob might use ament_target_dependencies(bob charlie) and get info from CMake variables

Using ament_target_dependencies() for that seems wrong since it doesn't offer an equivalent of target_link_libraries(... PRIVATE ...) (assuming it is used on target which is exported itself - in your case bob::bob). ament_target_dependencies() has only the semantics of PUBLIC and INTERFACE so it always exports information which seems undesired if charlie is an internal dependency.

sloretz commented 3 years ago

I patched ament_target_dependencies() locally to output some data and made a script to get an idea of how much work this ticket would be:

The number of uses of ament_target_dependencies() that could be changed to target_link_libraries() is pretty large (there is a big chunk that could be gotten rid of by editing a few CMake macros that call ament_target_dependencies() internally). That work should be tedious but trivial. The total list of projects who's old-style CMake variables get used via ament_target_dependencies() is pretty short, but exporting CMake targets from those might be more involved.

rosidl_generator_py
rttest
fastrtps
rmw_fastrtps_cpp
tf2_kdl
rosbag2_storage_default_plugins
rviz_assimp_vendor
rmw_cyclonedds_cpp
tlsf_cpp
tlsf
tinyxml2_vendor
rmw_fastrtps_dynamic_cpp
eigen3_cmake_module
OpenCV
Log4cxx
PythonExtra
Eigen3

fred-apex-ai commented 3 years ago

now that ROS 2 packages offer modern CMake targets.

@sloretz What do I have to do to export the targets? I guess call ament_export_targets and follow the tutorial. But how to combine this with ament_auto_package()? If I call ament_export_targets before ament_auto_package, I get an error

CMake Error: INSTALL(EXPORT) given unknown export "my_target"

If I call it afterwards, it complains it should be called before

  ament_export_targets() must be called before ament_package()

alsora commented 3 years ago

Hi, is there still interest on this work? I agree that target_link_libraries should be the recommended way for handling dependencies rather than a ROS-specific utility with its limitations.

wep21 commented 2 years ago

@sloretz Is it better to replace ament_target_dependencies with target_link_libraries in ament_cmake_auto? https://github.com/ament/ament_cmake/blob/3fc4db9dc728bcade5f02bb191ce20c8d3e62511/ament_cmake_auto/cmake/ament_auto_add_library.cmake#L80

Ryanf55 commented 1 year ago

For messages, I can't find any method that makes sense, other than this really long target name buried in the cmake export. I wouldn't consider that intuitive, and it's not documented, but it works.

Using ament_target_dependencies, with bar_package that is just a message package (it calls rosidl_generate_interfaces).

ament_target_dependencies(
  foo_node
  "bar_package"
)

If you switch to raw cmake, no luck. You get include errors, because includes aren't propogated by the same name. Maybe that's a target, maybe not. I'd have to add debug logic to check it's a target.

target_link_libraries(
  foo_node
  PRIVATE
  "bar_package"
)

If you switch to modern cmake alias targets, which error out if the target doesn't exist, intuitively, I'd expect a bar_package::bar_package target, but it doesn't exist.

target_link_libraries(
  foo_node
  bar_package::bar_package
)

  Target "foo_node" links to target "bar_package::bar_package" but the target
  was not found.  Perhaps a find_package() call is missing for an IMPORTED
  target, or an ALIAS target is missing?

Through some trial and error, this works.

target_link_libraries(
  foo_node
  PRIVATE
  bar_package::bar_package__rosidl_generator_cpp
)

Are the ROS maintainers actually expecting users wanting to use modern CMake to use those long target names?

If you want to play around yourself, see my example repo here: https://github.com/Ryanf55/ryan_ros_mvp/tree/bug-cmake-target-link-libraries-msgs

clalancette commented 1 year ago

target_link_libraries(
  foo_node
  PRIVATE
  bar_package::bar_package__rosidl_generator_cpp
)

Thanks, I didn't know about that one. The other workaround which works (and which we've been using in the core as we've been slowly converting it to modern cmake targets) is ${bar_package_TARGETS}, which also works.

Are the ROS maintainers actually expecting users wanting to use modern CMake to use those long target names?

We really do want to deprecate (and eventually remove) ament_target_dependencies in favor of modern cmake targets. And we really do want that target to be msg_package::msg_package, but we've had some problems making it work in the past. We've really just lacked time to look back into it and make it nicer, but any help here is appreciated.

Ryanf55 commented 1 year ago

target_link_libraries(
  foo_node
  PRIVATE
  bar_package::bar_package__rosidl_generator_cpp
)
Thanks, I didn't know about that one. The other workaround which works (and which we've been using in the core as we've been slowly converting it to modern cmake targets) is ${bar_package_TARGETS}, which also works.

Are the ROS maintainers actually expecting users wanting to use modern CMake to use those long target names?

We really do want to deprecate (and eventually remove) ament_target_dependencies in favor of modern cmake targets. And we really do want that target to be msg_package::msg_package, but we've had some problems making it work in the past. We've really just lacked time to look back into it and make it nicer, but any help here is appreciated.

If you have some references to previous work, that would be great. I can take a look into it.

Ryanf55 commented 1 year ago

I do have concern for how something like ros2 pkg create works, which assumes the target name == project name == library name == package name. That works great for ament_target_dependencies, but there's no way I see it being that easy for code-generation if the user doesn't know what the library's targets are, and ament_target_dependencies is theoretically deprecated.

Consdering getting information about a package after you call find_package(pkg), such as the list of targets, is not available, I don't have a generic way to do this. https://www.reddit.com/r/cmake/comments/ertanr/is_it_possible_to_list_imported_targets_from/ https://gitlab.kitware.com/cmake/cmake/-/issues/17236

Edit: The last post here shows a reasonable solution for getting all the targets: https://discourse.cmake.org/t/cmake-list-of-all-project-targets/1077/16

Ryanf55 commented 1 year ago

@sloretz was the one that mentioned using those targets here, and they are (now) part of the public API.

The namespace export is shown here.

I propose at least documenting them in the tutorials; although the name is long, it provides clear advantages over ${bar_package_TARGETS}, which is NOT one of the required variables exported by CMake. IMHO, it should be, but it's not.

To remove the ALIAS targets, one would have to go through a deprecation process since it's ABI breaking to remove them, so it's not like they will disappear.

alsora commented 1 year ago

As mentioned above, ROS 2 message packages are a little bit tricky to handle.

I want to point out that the solution proposed by @Ryanf55 is not complete. If you link with bar_package::bar_package__rosidl_generator_cpp, you will indeed be able to use the ROS message structure, but you won't be able to create publishers/subscriptions using it. The additional targets are needed for that.

Besides simplifying how to use messages, I think it's also important to easily allow targets to depend only on the message struct definitions (e.g. I may have a library that does some computations using a ROS message as data type, even if it does not have any other dependency on ROS and it does not create nodes, publishers, subscriptions, etc)

Ryanf55 commented 1 year ago

As mentioned above, ROS 2 message packages are a little bit tricky to handle.

I want to point out that the solution proposed by @Ryanf55 is not complete. If you link with bar_package::bar_package__rosidl_generator_cpp, you will indeed be able to use the ROS message structure, but you won't be able to create publishers/subscriptions using it. The additional targets are needed for that.

Besides simplifying how to use messages, I think it's also important to easily allow targets to depend only on the message struct definitions (e.g. I may have a library that does some computations using a ROS message as data type, even if it does not have any other dependency on ROS and it does not create nodes, publishers, subscriptions, etc)

Great info, I just summarized the discussion into an issue in rosidl. Anyone interested in rosidl specifically can resume discussion there since it's only somewhat related to this issue. Thanks!

Ryanf55 commented 8 months ago

I found another blocker for migration to ament_export_targets and target_link_libraries while working on https://github.com/BehaviorTree/BehaviorTree.CPP/issues/751.

ament_export_targets has the namespace for exports hard-coded as the project name. Great convention recommended by the CMake developers, but some packages override it. Perhaps an optional NAMESPACE argument needs to be added to ament_export_targets to override the default of ${PROJECT_NAME} for all the exported targets.

Current behavior for BTCPP

BehaviorTreeCPP supports building a few different ways.

Existing users are recommended to consume it with the following: https://github.com/BehaviorTree/btcpp_sample/blob/1dac26a573df205968da1505ece0ca00fec30884/CMakeLists.txt#L27

PROJECT_NAME: behaviortree_cpp Namespace: BT which comes from here

Unfortunately, ament_export_targets generates the following when called as

macro(export_btcpp_package)
    ament_export_targets(${PROJECT_NAME}Targets HAS_LIBRARY_TARGET)
    ament_export_dependencies(ament_index_cpp)
    ament_package()
endmacro()

# Create imported target behaviortree_cpp::behaviortree_cpp
add_library(behaviortree_cpp::behaviortree_cpp SHARED IMPORTED)

Desired behavior for BTCPP

In order to have the same workflow as existing CMake users, it should do the following:

# Create imported target BT::behaviortree_cpp
add_library(BT::behaviortree_cpp SHARED IMPORTED)

Proposed interface

macro(export_btcpp_package)
    ament_export_targets(${PROJECT_NAME}Targets NAMESPACE BT:: HAS_LIBRARY_TARGET)
    ament_export_dependencies(ament_index_cpp)
    ament_package()
endmacro()

ament / ament_cmake