-
Notifications
You must be signed in to change notification settings - Fork 673
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
Consider updating to ament_export_targets (Modern CMake) #751
Comments
I will happily accept your contributions. |
I ran into a blocker, sorry. Issue linked above. The decision to use In order to keep the usage semantics the same ( 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 I've submitted an PR upstream. Once that goes in, we can revisit this issue. |
I found a related problem here that causes linking to fail if there are two installations of this library: one in system (e.g., BehaviorTree.CPP/cmake/ament_build.cmake Lines 21 to 37 in 789ce6e
Does this get resolved with a move to |
I will test for this scenaio, and probably can add a CI check for it. It should be fixed. |
@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. |
Hello! The PR to
Thus, please don't hold up your release, unless you have a way to make all your consumers build |
can you create a PR, please, it is not clear to me what need to be done and I am busy with other stuff. |
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 usetarget_link_libraries
and link to an exported namespaced target. This is not yet possible.ament_export_include_directories
ament_export_libraries
ament_export_targets
target_link_libraries
as an alternative toament_target_dependencies
, perhaps by appending a "Usage with CMake" Guide somewhere to the docsWhy? 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
The text was updated successfully, but these errors were encountered: