Skip to content

Convert names_and_types graph APIs to pybind11#717

Merged
sloretz merged 5 commits intomasterfrom
hidmic/pybind-graph-api
Mar 18, 2021
Merged

Convert names_and_types graph APIs to pybind11#717
sloretz merged 5 commits intomasterfrom
hidmic/pybind-graph-api

Conversation

@hidmic
Copy link
Copy Markdown
Contributor

@hidmic hidmic commented Mar 17, 2021

Precisely what the title says. Part of #665. This patch also brings rcpputils as a dependency.

CI up to rclpy:

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

@hidmic hidmic requested review from cottsay, gbalke and sloretz March 17, 2021 15:03
@hidmic hidmic mentioned this pull request Mar 17, 2021
Closed
34 tasks
gbalke
gbalke approved these changes Mar 17, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@gbalke gbalke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Couple comments/questions.

rclpy/src/rclpy/_rclpy_pybind11.cpp Outdated
Comment on lines +243 to +272
m.def(
"rclpy_get_service_names_and_types",
&rclpy::graph_get_service_names_and_types,
"Get all service names and types in the ROS graph.");
m.def(
"rclpy_get_service_names_and_types",
&rclpy::graph_get_service_names_and_types,
"Get all service names and types in the ROS graph.");
Copy link
Copy Markdown
Contributor

@gbalke gbalke Mar 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duplicate?

Copy link
Copy Markdown
Contributor Author

@hidmic hidmic Mar 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed. Removed duplicate in a13f583.

rclpy/src/rclpy_common/include/rclpy_common/common.hpp Outdated
Comment on lines +31 to +33
* Raises ValueError if pywait_set is not a wait set capsule
* Raises RuntimeError if the entity type is not known
*
Copy link
Copy Markdown
Contributor

@gbalke gbalke Mar 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is pywait_set coming from?

Copy link
Copy Markdown
Contributor Author

@hidmic hidmic Mar 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blatant copy & paste. Fixed in a13f583.

sloretz
sloretz requested changes Mar 17, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@sloretz sloretz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like the removal of code from _rclpy.c is missing from the PR, mind adding it?

@hidmic
Copy link
Copy Markdown
Contributor Author

hidmic commented Mar 17, 2021

Running CI up to rclpy:

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

hidmic added 4 commits March 17, 2021 16:02
@hidmic
Convert names_and_types graph APIs to pybind11
5c17aad 
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@hidmic
Address peer review comments
60ca270 
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@hidmic
Use py::make_tuple()
5bf80b5 
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@hidmic
Remove old names_and_types graph APIs implementations
f009f08 
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@hidmic hidmic force-pushed the hidmic/pybind-graph-api branch from 4c66f3d to f009f08 Compare March 17, 2021 19:02
@hidmic
Copy link
Copy Markdown
Contributor Author

hidmic commented Mar 17, 2021

Rebased to solve conflicts.

@hidmic
Copy link
Copy Markdown
Contributor Author

hidmic commented Mar 17, 2021

CI up to rclpy, third's the charm:

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

@sloretz sloretz self-requested a review March 17, 2021 19:05
sloretz
sloretz approved these changes Mar 17, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@sloretz sloretz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One nitpick that I wouldn't block on.

@hidmic
Add missing const
b016dbc 
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@sloretz
Copy link
Copy Markdown
Contributor

sloretz commented Mar 18, 2021

CI with const change (build: --packages-up-to rclpy test: --packages-select rclpy)

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

@sloretz
Copy link
Copy Markdown
Contributor

sloretz commented Mar 18, 2021

@hidmic, It looks like this one is ready to go! I'll merge it so it doesn't need to be rebased again if something else would have gotten merged first.

@sloretz sloretz merged commit 4d76c3a into master Mar 18, 2021
@delete-merged-branch delete-merged-branch bot deleted the hidmic/pybind-graph-api branch March 18, 2021 15:39
@clalancette
Copy link
Copy Markdown
Contributor

clalancette commented Mar 19, 2021 •
edited
Loading

I'm not 100% sure if it was this PR, but Windows debug build is now broken in rclpy: https://ci.ros2.org/view/nightly/job/nightly_win_deb/1931/ . The only two PRs that were merged in rclpy yesterday were this one and #712 . @sloretz @hidmic can you take a look?

@hidmic
Copy link
Copy Markdown
Contributor Author

hidmic commented Mar 19, 2021

It's trying to link to non-debug Python libraries:

LINK : fatal error LNK1104: cannot open file 'python38.lib' [C:\ci\ws\build\rclpy\rclpy_common.vcxproj]

but the CMakeLists.txt does seem to be taking care of not doing that. Has this happened to you before @sloretz ?

@sloretz
Copy link
Copy Markdown
Contributor

sloretz commented Mar 19, 2021

but the CMakeLists.txt does seem to be taking care of not doing that. Has this happened to you before @sloretz ?

Yeah, this seems familiar. There's some odd linker pragmas in the python.h and pybind11.h headers involving the Py_DEBUG macro. I'm guessing if we include <pybind11/pybind11.h> before <python.h> in the other compilation units in rclpy_common, then the issue would be resolved.

@sloretz
Copy link
Copy Markdown
Contributor

sloretz commented Mar 19, 2021

I think we've been getting lucky with the order. I'm guessing the linker pragma set by the last compilation unit is the one that wins :-/

The jobs that passed, the C++ file comes last:

  Building Custom Rule C:/ci/ws/src/ros2/rclpy/rclpy/CMakeLists.txt

  common.c

  handle.c

  exceptions.cpp

     Creating library C:/ci/ws/build/rclpy/Debug/rclpy_common.lib and object C:/ci/ws/build/rclpy/Debug/rclpy_common.exp

The job that failed, a C file came last

 Building Custom Rule C:/ci/ws/src/ros2/rclpy/rclpy/CMakeLists.txt

  common.c

  common.cpp

  exceptions.cpp

  handle.c

LINK : fatal error LNK1104: cannot open file 'python38.lib' [C:\ci\ws\build\rclpy\rclpy_common.vcxproj]

@hidmic
Copy link
Copy Markdown
Contributor Author

hidmic commented Mar 19, 2021

Oh my. Will fix now.

@hidmic hidmic mentioned this pull request Mar 19, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sloretz sloretz sloretz approved these changes

@cottsay cottsay Awaiting requested review from cottsay

+1 more reviewer

@gbalke gbalke gbalke approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@hidmic @sloretz @clalancette @gbalke