Skip to content

Converting last of _rclpy.c to pybind11#738

Merged
gbalke merged 14 commits intoros2:masterfrom
gbalke:wrapping_up_pybind11
Mar 30, 2021
Merged

Converting last of _rclpy.c to pybind11#738
gbalke merged 14 commits intoros2:masterfrom
gbalke:wrapping_up_pybind11

Conversation

@gbalke
Copy link
Copy Markdown
Contributor

@gbalke gbalke commented Mar 25, 2021

Part of #665 covering:

rclpy_get_rmw_implementation_identifier
rclpy_assert_liveliness
rclpy_remove_ros_args

Greg Balke added 3 commits March 25, 2021 11:43
Signed-off-by: Greg Balke <greg@openrobotics.org>
Signed-off-by: Greg Balke <greg@openrobotics.org>
Signed-off-by: Greg Balke <greg@openrobotics.org>
@gbalke gbalke requested review from hidmic and sloretz March 25, 2021 20:09
@gbalke gbalke self-assigned this Mar 25, 2021
@gbalke
Copy link
Copy Markdown
Contributor Author

gbalke commented Mar 25, 2021

CI up to rclpy:

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

@gbalke gbalke mentioned this pull request Mar 26, 2021
34 tasks
Signed-off-by: Greg Balke <greg@openrobotics.org>
@gbalke gbalke force-pushed the wrapping_up_pybind11 branch from 52df391 to d413c1f Compare March 29, 2021 19:11
Greg Balke added 3 commits March 29, 2021 12:14
Signed-off-by: Greg Balke <greg@openrobotics.org>
Signed-off-by: Greg Balke <greg@openrobotics.org>
Signed-off-by: Greg Balke <greg@openrobotics.org>
@gbalke gbalke requested review from ahcorde, hidmic and sloretz March 29, 2021 19:26
@gbalke
Copy link
Copy Markdown
Contributor Author

gbalke commented Mar 29, 2021

CI up to rclpy:

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

Copy link
Copy Markdown
Contributor

@hidmic hidmic left a comment

Choose a reason for hiding this comment

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

Overall LGTM

Greg Balke added 3 commits March 29, 2021 13:50
Signed-off-by: Greg Balke <greg@openrobotics.org>
Signed-off-by: Greg Balke <greg@openrobotics.org>
Signed-off-by: Greg Balke <greg@openrobotics.org>
@gbalke
Copy link
Copy Markdown
Contributor Author

gbalke commented Mar 29, 2021

@sloretz I merged and cleaned out _rclpy.c but am not sure exactly how to remove the plumbing that attaches to the file itself cleanly. I'll leave that for a separate PR unless you want to give me pointers 😄.

I attempted to just delete the file and remove it from CMakeLists.txt but got some test errors 😢.

@gbalke
Copy link
Copy Markdown
Contributor Author

gbalke commented Mar 29, 2021

CI up to rclpy:

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

@sloretz
Copy link
Copy Markdown
Contributor

sloretz commented Mar 29, 2021

I attempted to just delete the file and remove it from CMakeLists.txt but got some test errors cry.

In addition to deleting the file and removing it from the CMakeLists.txt, I think this change to rclpy/impl/implementation_singleton.py would be enough.

--- <unnamed>
+++ <unnamed>
@@ -2,26 +2,9 @@
 from rpyutils import import_c_library
 package = 'rclpy'
 
-rclpy_implementation = import_c_library('._rclpy', package)
+rclpy_implementation = import_c_library('._rclpy_pybind11', package)
 rclpy_action_implementation = import_c_library('._rclpy_action', package)
 rclpy_logging_implementation = import_c_library('._rclpy_logging', package)
 rclpy_signal_handler_implementation = import_c_library('._rclpy_signal_handler', package)
 rclpy_handle_implementation = import_c_library('._rclpy_handle', package)
 rclpy_pycapsule_implementation = import_c_library('._rclpy_pycapsule', package)
-
-
-# Temporary code for converting giant _rclpy module to pybind11
-def _combine_split_modules():
-    global rclpy_implementation
-    module = import_c_library('._rclpy_pybind11', package)
-    for attr in dir(module):
-        thing = getattr(module, attr)
-        if attr.startswith('rclpy_'):
-            # It's a wrapped C function
-            setattr(rclpy_implementation, attr, thing)
-        elif isinstance(thing, type):
-            # It's a custom type
-            setattr(rclpy_implementation, attr, thing)
-        elif '_' != attr[0] and attr.upper() == attr:
-            # It's a module constant
-            setattr(rclpy_implementation, attr, thing)

@sloretz
Copy link
Copy Markdown
Contributor

sloretz commented Mar 29, 2021

and deleting these two as well

_combine_split_modules()
del _combine_split_modules

Signed-off-by: Greg Balke <greg@openrobotics.org>
Signed-off-by: Greg Balke <greg@openrobotics.org>
@gbalke
Copy link
Copy Markdown
Contributor Author

gbalke commented Mar 29, 2021

CI up to rclpy:

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

@hidmic
Copy link
Copy Markdown
Contributor

hidmic commented Mar 29, 2021

Pending green CI !

Co-authored-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Greg Balke <greg@openrobotics.org>
@gbalke gbalke force-pushed the wrapping_up_pybind11 branch from 4e06d0a to 515b284 Compare March 30, 2021 17:22
Signed-off-by: Greg Balke <greg@openrobotics.org>
@gbalke gbalke force-pushed the wrapping_up_pybind11 branch from 368a095 to 1c0d1c4 Compare March 30, 2021 17:32
@gbalke
Copy link
Copy Markdown
Contributor Author

gbalke commented Mar 30, 2021

CI up to rclpy:

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

@gbalke gbalke merged commit aac41b5 into ros2:master Mar 30, 2021
@gbalke gbalke deleted the wrapping_up_pybind11 branch March 30, 2021 18:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants