-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Revamp nav2_util CMakeLists.txt to use modern idioms. #4393
Revamp nav2_util CMakeLists.txt to use modern idioms. #4393
Conversation
This commit does a number of things: 1. Switches to using target_link_libraries everywhere. This gives us finer-grained control over what dependencies are exported to downstream as public, or private. In the particular case of nav2_util, this actually doesn't matter *too* much, but it will help for other packages. 2. Moves the include directory down one level to include/${PROJECT_NAME}, which is best practice in ROS 2 since Humble. 3. Makes sure to export nav2_util as a CMake target, so downstream users of it can use that target. 4. Moves the base_footprint_publisher.hpp header file into the src directory, as it isn't functionality that an external project could use. Signed-off-by: Chris Lalancette <[email protected]>
a390b2c
to
2ae04ed
Compare
@clalancette, your PR has failed to build. Please check CI outputs and resolve issues. |
CI is failing to build because we need a release of bondcpp into Rolling. I'll take a look at getting that done. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Questions, but no issues once bond changes turn over
${tf2_geometry_msgs_TARGETS} | ||
) | ||
target_link_libraries(${library_name} PRIVATE | ||
${bond_TARGETS} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: why is this one & rclcpp below private? When do we do public vs private?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, good question.
So the difference between PUBLIC and PRIVATE is that PUBLIC dependencies are exported to downstream consumers, while PRIVATE ones are not. Marking dependencies as PRIVATE (where they can be) is part of what helps speed up compilation, since the compiler doesn't have to go searching through additional directories for headers and libraries.
As for when to use each, there are separate rules for executables and libraries:
- No downstream can ever compile or link against an executable, so all dependencies can be PRIVATE. This honestly doesn't matter too much, as no downstream will even attempt to link, but making it PRIVATE is just cleaner.
- For libraries, a dependency is PRIVATE if it is only used during compilation of this package. So in a standard ROS package layout, if something is only ever referenced in the
src
directory, then it can be PRIVATE. Anything that is referenced in theinclude
directory needs to be PUBLIC, because in order for a downstream package to compile, it also needs to have access to those includes.
As a concrete example for 2, in nav2_util
the bond
package (which is really a message package, despite the name) is only ever referenced in https://github.com/ros-navigation/navigation2/blob/03ee50a629425bdd2f24463474732b7d9bf6fe95/nav2_util/src/lifecycle_node.cpp , so it can be PRIVATE. On the other hand, geometry_msgs
is referenced in https://github.com/ros-navigation/navigation2/blob/03ee50a629425bdd2f24463474732b7d9bf6fe95/nav2_util/include/nav2_util/geometry_utils.hpp (among other places), so it needs to be PUBLIC (a library that depends on nav2_util
needs to know where to find the geometry_msgs
headers so it can compile).
Does this make sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Makes sense
- So what if the compiled library is used in another package (i.e. lifecycle node is then used in other packages as the base of the action servers)? Seem unintuitive to me that the include in the cpp vs hpp would make that much of a difference, but 👍
is only ever referenced in https://github.com/ros-navigation/navigation2/blob/03ee50a629425bdd2f24463474732b7d9bf6fe95/nav2_util/src/lifecycle_node.cpp
But if that was in the lifecycle_node.hpp
file, then it would need to be public, correct? This is interesting. The convention I was taught early on was to include as much in the headers as possible and use only what you need in the .cpp
files -- but to be fair there's alot of bad conventions people told me early on that I've corrected, so perhaps this is another that I wasn't aware of 🤦
So.. is it worth me at some point doing a review of the stack and moving as much to cpp files to make the dependencies private?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- So what if the compiled library is used in another package (i.e. lifecycle node is then used in other packages as the base of the action servers)? Seem unintuitive to me that the include in the cpp vs hpp would make that much of a difference, but 👍
I'll preface this by saying I don't completely know how this works, so the below is a bit of speculation.
Thinking about it, if you have a symbol that is only ever internal to a library, then no downstream can ever make (direct) reference to it. Thus, the downstream packages don't have to know how to resolve that upstream symbol in order to compile/link. They do have to know how to resolve that symbol at runtime, but I believe that is handled either via RPATH/RUNPATH, or via LD_LIBRARY_PATH.
But if that was in the
lifecycle_node.hpp
file, then it would need to be public, correct?
Yes, exactly.
So.. is it worth me at some point doing a review of the stack and moving as much to cpp files to make the dependencies private?
Moving as much as possible into the .cpp
files will reduce the number of dependencies you need to export to downstream, so yes, I think it is worth doing.
Let me know when bond’s all good for this to turn over. Consider it approved and we can move onto the next one(s) while we’re waiting! |
We've had a rolling sync now, so in theory CI should pass here. If you could kick it off again, we'll see! |
Builds now! But a weird test failed that seems unrelated but also not known as being flaky. I'm rerunning it now and I'll check in the morning. If it happens again, I'll take care of it so you can move on. |
Oh sorry, that test bug was fixed recently, I'm not sure why that wasn't coming to my mind yesterday afternoon. This is just because your branch needs to be rebased to get those updates. LGTM, shipping! |
…#4393) This commit does a number of things: 1. Switches to using target_link_libraries everywhere. This gives us finer-grained control over what dependencies are exported to downstream as public, or private. In the particular case of nav2_util, this actually doesn't matter *too* much, but it will help for other packages. 2. Moves the include directory down one level to include/${PROJECT_NAME}, which is best practice in ROS 2 since Humble. 3. Makes sure to export nav2_util as a CMake target, so downstream users of it can use that target. 4. Moves the base_footprint_publisher.hpp header file into the src directory, as it isn't functionality that an external project could use. Signed-off-by: Chris Lalancette <[email protected]>
…#4393) This commit does a number of things: 1. Switches to using target_link_libraries everywhere. This gives us finer-grained control over what dependencies are exported to downstream as public, or private. In the particular case of nav2_util, this actually doesn't matter *too* much, but it will help for other packages. 2. Moves the include directory down one level to include/${PROJECT_NAME}, which is best practice in ROS 2 since Humble. 3. Makes sure to export nav2_util as a CMake target, so downstream users of it can use that target. 4. Moves the base_footprint_publisher.hpp header file into the src directory, as it isn't functionality that an external project could use. Signed-off-by: Chris Lalancette <[email protected]>
…#4393) This commit does a number of things: 1. Switches to using target_link_libraries everywhere. This gives us finer-grained control over what dependencies are exported to downstream as public, or private. In the particular case of nav2_util, this actually doesn't matter *too* much, but it will help for other packages. 2. Moves the include directory down one level to include/${PROJECT_NAME}, which is best practice in ROS 2 since Humble. 3. Makes sure to export nav2_util as a CMake target, so downstream users of it can use that target. 4. Moves the base_footprint_publisher.hpp header file into the src directory, as it isn't functionality that an external project could use. Signed-off-by: Chris Lalancette <[email protected]>
…#4393) This commit does a number of things: 1. Switches to using target_link_libraries everywhere. This gives us finer-grained control over what dependencies are exported to downstream as public, or private. In the particular case of nav2_util, this actually doesn't matter *too* much, but it will help for other packages. 2. Moves the include directory down one level to include/${PROJECT_NAME}, which is best practice in ROS 2 since Humble. 3. Makes sure to export nav2_util as a CMake target, so downstream users of it can use that target. 4. Moves the base_footprint_publisher.hpp header file into the src directory, as it isn't functionality that an external project could use. Signed-off-by: Chris Lalancette <[email protected]>
Basic Info
Description of contribution in a few bullet points
One thing of note here is that to make these changes, I used https://github.com/clalancette/ros2_pkg_dep_tool to at least guide which dependencies were needed. I didn't actually apply all of the changes suggested by the tool, as it would require quite a bit of changes to the
#include
lines, but at least it was useful in guiding what dependencies were actually needed here.Description of documentation updates required from your changes
None should be required, particularly for this first PR. Once the whole set has gone in, we should probably update the docs to suggest people link against Navigation2 using the CMake targets and
target_link_libraries
, rather thanament_target_dependencies
.Future work that may be required in bullet points
Assuming the basic idea here is accepted, all of the rest of the packages in Navigation2 should undergo a similar treatment. See #4357 for a prototype of what that would look like, though significant changes need to be made there.
For Maintainers: