Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Make rclpy dependencies explicit #906

Merged
merged 3 commits into from
Apr 7, 2022
Merged

Make rclpy dependencies explicit #906

merged 3 commits into from
Apr 7, 2022

Conversation

clalancette
Copy link
Contributor

@clalancette clalancette commented Feb 28, 2022 •
edited
Loading

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 and rmw, 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:

  1. Does a pass for include-what-you-use everywhere in the rclpy code.
  2. Adds the new dependencies.
  3. Fixes up documentation problems noticed while doing the above.

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.

adityapande-1995
adityapande-1995 reviewed Mar 17, 2022
View reviewed changes
rclpy/src/rclpy/action_server.cpp
Comment on lines -19 to +29
#include "rcl/error_handling.h"
#include "rcpputils/scope_exit.hpp"
#include <rcpputils/scope_exit.hpp>
Copy link

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 ?

Copy link
Contributor

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 "".

adityapande-1995
adityapande-1995 reviewed Mar 17, 2022
View reviewed changes
Copy link

@adityapande-1995 adityapande-1995 left a 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.

rclpy/src/rclpy/action_client.hpp Outdated Show resolved Hide resolved
@sloretz
Copy link
Contributor

sloretz commented Mar 25, 2022

Changes to fully-qualified object names within the pybind11 code, just to make it clear where things are coming from.

It looks like these changes are inside code in a namespace rclpy {...} block. I feel like that's enough context to have a pretty good intuition for where they come from.

@clalancette
Copy link
Contributor Author

It looks like these changes are inside code in a namespace rclpy {...} block. I feel like that's enough context to have a pretty good intuition for where they come from.

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).

@clalancette
Update rclpy include headers to include-what-you-use.
24bffab 
Also rearrange the includes in some files so that they are
consistent with the rest.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
@clalancette
Add rosidl_runtime_c as an explicit dependency.
38c9909 
We make calls to it, so we really should have a dependency on it.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
@clalancette
Fix up API documentation.
f0a8eb9 
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
@clalancette
Copy link
Contributor Author

I've now removed that last patch. I'm going to run CI on the lot here to see what happens.

@clalancette
Copy link
Contributor Author

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@clalancette
Copy link
Contributor Author

@ros-pull-request-builder retest this please

@clalancette
Copy link
Contributor Author

CI is all green, and approved, so I'm going to go ahead and merge this in as a fix.

@clalancette clalancette merged commit c704619 into master Apr 7, 2022
@clalancette clalancette deleted the clalancette/rclpy-cleanups branch April 7, 2022 17:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sloretz sloretz sloretz left review comments

@adityapande-1995 adityapande-1995 adityapande-1995 approved these changes

Assignees

@adityapande-1995 adityapande-1995

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@clalancette @sloretz @adityapande-1995