Consider updating to ament_export_targets (Modern CMake)

[Ryanf55]

Current Behavior

BehaviorTree.CPP has support for ament (YAY!), but it's using the old-style macros that were for before CMake 3, in that they set variables instead of target properties. They may be deprecated soon: ament/ament_cmake#365

Desired Behavior

When you call find_package in the ROS ecosystem, it's becoming more common practice to use target_link_libraries and link to an exported namespaced target. This is not yet possible.

• Update this section of ament_build.cmake to :
  • Remove the line with ament_export_include_directories
  • Remove the line with ament_export_libraries
  • Add a call to ament_export_targets
• Update documentation on recommended usage with target_link_libraries as an alternative to ament_target_dependencies, perhaps by appending a "Usage with CMake" Guide somewhere to the docs
• Add a CI task that verifies the changes to the ROS build don't break consumers
• Patch this into a release, and release binaries for rolling, iron and humble, since it is backward compatible.

Why? Productivity! If you spell behaviortree-cpp wrong when you call target_link_libraries, you get include errors instead of a nice "This target doesn't exist error". Exported namespace targets catch bugs at configure time rather than build time.

Outside help

I can do all the work up to the release part, but do not plan to do it until you approve these changes. Please let me know if you agree with the approach presented, and I will get started.

Reference

https://docs.ros.org/en/humble/How-To-Guides/Ament-CMake-Documentation.html#installing

[facontidavide]

I will happily accept your contributions.

[Ryanf55]

I ran into a blocker, sorry. Issue linked above. The decision to use BT:: as the exported namespace for conan builds is not supported yet in ament_export_targets. ament_export_targets hard codes the project name as the namespace, which is more common convention.

In order to keep the usage semantics the same (BT::BT::behaviortree_cpp everywhere), this issue will need to be put on hold till upstream has a recommendation.

If we rushed this in with a different namespace, then you'd have a support/documentation fiasco and downstream portable code would be annoying to write to account for both target names.

ROS users should continue to use ament_target_dependencies for the time being, despite it not supporting PRIVATE linkage.

I've submitted an PR upstream. Once that goes in, we can revisit this issue.

[asasine]

I found a related problem here that causes linking to fail if there are two installations of this library: one in system (e.g., /opt/ros/{version} via ros-{version}-behaviortree-cpp and one cloned in your workspace). Because ament_export_dependencies(ament_index_cpp) comes before ament_export_include_directories(include), the resulting ${behaviortree_cpp_INCLUDE_DIRS} has /opt/ros/{version}/include before /path/to/local/install/behaviortree_cpp/include, so downstream users of the library link to symbols in /opt/ros/{version}/include instead of /path/to/local/install/behaviortree_cpp/include.

BehaviorTree.CPP/cmake/ament_build.cmake

Lines 21 to 37 in 789ce6e

	ament_export_dependencies(ament_index_cpp)
	set( BTCPP_LIB_DESTINATION lib )
	set( BTCPP_INCLUDE_DESTINATION include )
	set( BTCPP_BIN_DESTINATION bin )
	mark_as_advanced(
	BTCPP_EXTRA_LIBRARIES
	BTCPP_LIB_DESTINATION
	BTCPP_INCLUDE_DESTINATION
	BTCPP_BIN_DESTINATION )
	macro(export_btcpp_package)
	ament_export_include_directories(include)
	ament_export_libraries(${BTCPP_LIBRARY})
	ament_package()
	endmacro()

Does this get resolved with a move to ament_export_targets that also removes ament_export_include_directories, or does it remain? I'm thinking that it fixes it, because the exported target should have /path/to/local/install/behaviortree_cpp/include before /opt/ros/{version}/include in its interface include directories, but I'm unsure.

[Ryanf55]

I will test for this scenaio, and probably can add a CI check for it. It should be fixed.

[facontidavide]

@Ryanf55 if we can fix this the next week, it would be ideal, I want to release 4.6

[Ryanf55]

@Ryanf55 if we can fix this the next week, it would be ideal, I want to release 4.6

Yes. It merged to rolling last week, but CI failed on the iron/humble backport for unrelated reasons. After a re-run, it should be good (in theory). I don't have permissions for a re-run.

[Ryanf55]

Hello!

The PR to humble just merged. The ament_cmake maintainers have to

• Spin a release, which makes binaries available in testing
• Then, there needs to be another "sync" in humble. Because we just had one, it's going to be a month.

Thus, please don't hold up your release, unless you have a way to make all your consumers build ament_cmake from source :)

[facontidavide]

can you create a PR, please, it is not clear to me what need to be done and I am busy with other stuff.
Thanks 😄