-
Notifications
You must be signed in to change notification settings - Fork 225
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
Make rclpy dependencies explicit #906
Conversation
#include "rcl/error_handling.h" | ||
#include "rcpputils/scope_exit.hpp" | ||
#include <rcpputils/scope_exit.hpp> |
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.
What is the general preference / policy for include tags with respect to quotes marks and angle brackets ?
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.
I'm not sure we have a style rule for it, but I think a good rule is if it's in a different package use <>
, if it's in the same package use ""
.
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.
Can we split these into say 2 PRs ? Might make it easier to review individually.
It looks like these changes are inside code in a |
That's fair. I waffled on this anyway. I'll remove all of that change from this PR (which should simplify it quite a bit as well). |
Also rearrange the includes in some files so that they are consistent with the rest. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
We make calls to it, so we really should have a dependency on it. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
b9ed8c9
to
f0a8eb9
Compare
I've now removed that last patch. I'm going to run CI on the lot here to see what happens. |
@ros-pull-request-builder retest this please |
CI is all green, and approved, so I'm going to go ahead and merge this in as a fix. |
While working on some other things in rclpy, I noticed that rclpy didn't properly declare all of its dependencies. In particular, it depends on
rosidl_runtime_c
andrmw
, but neither of these were explicitly called out in the package.xml or CMakeLists.txt.To fix this, there are 3 patches in this series:
All 3 patches are completely independent, and could all be taken separately. But given that I noticed them all together, I figured I would open one big PR for it. Let me know if you'd like me to split it up.