Update dependencies exported by nav2_tasks.#474
Conversation
|
I've edited the PR description to (hopefully) be a bit more detailed and clear about what the problem is and how this tries to solve it. |
|
@nuclearsandwich - Thanks for the explanation. Coincidentally, our Travis build is failing for a different reason, so I'm trying to get that one fixed before merging this one. Should be very soon. |
|
Adding @mjeronimo since he is also a maintainer |
|
@nuclearsandwich - can you rebase and re-push now that Travis is passing again on master? I want to make sure my fix and your fix both work before merging |
Exporting dependencies is not required in all situations. The documentation escapes me right now but exporting dependencies is used for: * Dependencies whose C/C++ types will be present in your package's * Dependencies whose interfaces will be encountered by your package's dependents. * Build dependencies of code generated by your package which will be built during a downstream package's build.
nuclearsandwich
force-pushed
the
remove-ament-export-dependencies
branch
from
5de3145 to
a4938a9
Compare
mkhansenbot
left a comment
mkhansenbot
left a comment
There was a problem hiding this comment.
See my other comment regarding rebasing
ghost
mentioned this pull request
orduno
left a comment
orduno
left a comment
There was a problem hiding this comment.
Not exporting at least one of the dependencies causes a build error on nav2_bt_navigator. See my comment for a suggestion.
|
|
||
| ament_export_include_directories(include) | ||
| ament_export_libraries(${library_name}) | ||
| ament_export_dependencies(${dependencies}) |
There was a problem hiding this comment.
@nuclearsandwich Thanks for pointing this out. We do need to export at least tf2_geometry_msgs. It is a dependency of spin_action.hpp and is required downstream when building nav2_bt_navigator. A better approach for now:
ament_export_dependencies(tf2_geometry_msgs)
And as you suggested, add it to the package.xml:
build_export_depend(tf2_geometry_msgs)
There was a problem hiding this comment.
I'll implement this now
There was a problem hiding this comment.
@orduno why don't you just add tf2_geometry_msgs to the package xml in bt navigator rather than making it the tasks package's problem?
There was a problem hiding this comment.
why don't you just add
tf2_geometry_msgsto the package xml in bt navigator rather than making it the tasks package's problem?
If this package is publishing messages provided by another package, such that any package which consumes this one will need those messages, I think exporting the dependency here is a valid thing to do.
There was a problem hiding this comment.
Ok, @orduno can you do an exhaustive search to see if there's any others we're missing?
There was a problem hiding this comment.
@nuclearsandwich should we more generally be removing these ament export dependencies? They're broadly used right now https://github.com/ros-planning/navigation2/search?q=ament_export_dependencies&unscoped_q=ament_export_dependencies
There was a problem hiding this comment.
should we more generally be removing these ament export dependencies? They're broadly used right now https://github.com/ros-planning/navigation2/search?q=ament_export_dependencies&unscoped_q=ament_export_dependencies
Good catch! Each of those should definitely be looked at, ideally by someone familiar with the package to determine whether exported packages are actually needed by downstream or not. Those that aren't can be removed and those that are can be audited to make sure there's a corresponding build_export_depend in the package.xml.
There was a problem hiding this comment.
Yeah. We kind of took a blunt hammer approach and exported everything in many cases, and apparently didn't do it right (need the build_export_depend). But I don't think we should remove it altogether.
Without the ament_export_dependencies line, somebody will be forced to manually figure out all the build failures they'll get whenever they include a header from one of our packages.
There was a problem hiding this comment.
I'm not sure we're saying to remove them altogether as much as audit them and make sure we've resolved this issue as made famous by rosidl
nuclearsandwich
changed the title
ghost
merged commit 
4 participants
Basic Info
Description of contribution in a few bullet points
This change attempts to address build failures of packages which depend on nav2_tasks.
The nav2_tasks CMake currently exports dependencies that are not declared as
build_export_dependin its package.xml. I'm not familiar with the nav2_tasks package but a cursory inspection suggests it doesn't need to be exporting these dependencies.Exporting dependencies is not required in all situations.
The documentation escapes me right now but exporting dependencies is
used for:
dependents.
built during a downstream package's build.
I removed this export because I couldn't see at a glance any of the above cases for any dependency of nav2_tasks. But the final decision is best left to the maintainers. Any packages that do need to be exported need an accompanying
build_export_dependin the package.xml manifest.Future work that may be required in bullet points