Convert init/shutdown to pybind11 by sloretz · Pull Request #715 · ros2/rclpy

sloretz · 2021-03-16T01:30:59Z

Part of #665

hidmic

Looking good!

rclpy/rclpy/context.py

hidmic · 2021-03-16T17:24:14Z

rclpy/src/rclpy/init.cpp

+  {
+    rcl_ret_t ret = rcl_init_options_fini(&rcl_options_);
+    if (RCL_RET_OK != ret) {
+      fprintf(


@sloretz FYI there's RCUTILS_SAFE_FWRITE_TO_STDERR too.

Using rcutils stderr macro in 66a7131

hidmic · 2021-03-16T17:25:01Z

rclpy/src/rclpy/init.cpp

+    }
+  }
+
+  rcl_init_options_t rcl_options_;


@sloretz same nits as for SerializedMessage in #712

Made InitOptions a struct in 007b970

hidmic · 2021-03-16T17:34:30Z

rclpy/src/rclpy/init.cpp

+  rcl_allocator_t allocator = rcl_get_default_allocator();
+  InitOptions init_options(allocator);


@sloretz nit:

Suggested change

rcl_allocator_t allocator = rcl_get_default_allocator();

InitOptions init_options(allocator);

InitOptions init_options(rcl_get_default_allocator());

removed temporary allocator in 25e8e22

hidmic · 2021-03-16T17:35:07Z

rclpy/src/rclpy/init.cpp

+  }
+
+  rcl_ret_t ret = rcl_shutdown(context);
+  if (ret != RCL_RET_OK) {


@sloretz nit: flip operands

Flipped the operands in dc06621

hidmic · 2021-03-16T17:40:28Z

rclpy/src/rclpy_common/src/exceptions.cpp

+
+  if (unparsed_ros_args_count < 0) {
+    throw std::runtime_error("failed to count unparsed arguments");
+  } else if (0 == unparsed_ros_args_count) {


@sloretz nit^N: that else is redundant, the previous if clause will leave the scope if it ever evaluates to true.

removed else in ec35c46

sloretz · 2021-03-18T16:13:25Z

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

Linux
Linux-aarch64
macOS
Windows

hidmic · 2021-03-18T19:09:30Z

rclpy/src/rclpy_common/include/rclpy_common/exceptions.hpp

+private:
+  using std::runtime_error::runtime_error;
+
+  friend void throw_if_unparsed_ros_args(py::list, const rcl_arguments_t &);


@sloretz uh, why the privacy?

IIUC private with using std::runtime_error::runtime_error forces one to use throw_if_unused_ros_args() so the exception text is always formatted the same. The access specifier has no effect on the friend declaration.

I ended up removing the friendship and pybind11 use from rclpy_common (with Windows Debug compatibility in mind) in d224f19

hidmic · 2021-03-18T19:10:22Z

MacOS seems to be having issues with some tests.

sloretz · 2021-03-18T20:52:48Z

Testing if adding visibility macro before friend declaration satifies windows:

sloretz · 2021-03-18T22:50:24Z

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

Linux
Linux-aarch64
macOS
Windows

sloretz · 2021-03-22T17:54:18Z

CI OSX to see if test failures are fixed by d224f19

CI Windows debug out of paranoia

sloretz · 2021-03-22T17:59:35Z

Ah, I'm more confident that 021e795 will fix the OSX test failure.

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

Linux
Linux-aarch64
macOS
Windows
Windows Debug

sloretz · 2021-03-22T19:12:21Z

Windows debug again with 3df8fbc

Windows Debug

sloretz · 2021-03-22T21:12:34Z

Windows debug warning should be fixed by #731

hidmic · 2021-03-22T21:38:48Z

~~I'll get this one in.~~ Oops, not mine.

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

sloretz · 2021-03-22T21:55:56Z

Rebased to fix conflicts - no changes

hidmic

LGTM!

rclpy/src/rclpy/init.hpp

hidmic · 2021-03-22T22:06:06Z

rclpy/src/rclpy/init.cpp

+        "failed to fini rcl_init_options_t in destructor:");
+      RCUTILS_SAFE_FWRITE_TO_STDERR(rcl_get_error_string().str);
+      RCUTILS_SAFE_FWRITE_TO_STDERR("\n");
+      rcl_reset_error();


@sloretz meta, nothing to do here: one cool alternative to printing to stderr I saw in the code base is to use Python warnings. pybind11 doesn't provide a nice wrapper though.

sloretz · 2021-03-22T22:11:03Z

One last CI run

Linux
Linux-aarch64
macOS
Windows

sloretz self-assigned this Mar 16, 2021

sloretz mentioned this pull request Mar 16, 2021

Use Pybind11 for rclpy Python Bindings #665

Closed

34 tasks

hidmic reviewed Mar 16, 2021

View reviewed changes

sloretz force-pushed the pybind11_init branch 2 times, most recently from 66a7131 to 3dfce00 Compare March 18, 2021 16:11

hidmic approved these changes Mar 18, 2021

View reviewed changes

sloretz force-pushed the pybind11_init branch from 3dfce00 to b6e09e8 Compare March 18, 2021 20:48

sloretz force-pushed the pybind11_init branch from 1b7758f to d224f19 Compare March 22, 2021 17:51

sloretz added 13 commits March 22, 2021 14:49

Convert init/shutdown to pybind11

4323332

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

Remove old code

c6fba30

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

Add visibility macro

556fbc5

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

Make InitOptions a struct

461dfec

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

remove temporary allocator variable

73359d2

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

Swap conditional order

a54f6c5

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

Remove redundant else

92a9176

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

Use rcutils macro for stderr writing

38df6bd

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

Check for too many arguments

c3a1ffb

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

Try visibility macro to satisfy windows

5770613

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

Removed pybind11 usage from rclpy_common

38a4953

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

UnknownROSArgsError inherits from RuntimeError

dc563e5

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

Undo changes to exceptions.cpp

d0f7882

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

sloretz force-pushed the pybind11_init branch from 3df8fbc to d0f7882 Compare March 22, 2021 21:55

hidmic approved these changes Mar 22, 2021

View reviewed changes

sloretz merged commit a09ac14 into master Mar 22, 2021

delete-merged-branch bot deleted the pybind11_init branch March 22, 2021 23:37

hidmic mentioned this pull request Mar 29, 2021

Make sure only non-empty std::vector of arguments are indexed #740

Merged

		rcl_allocator_t allocator = rcl_get_default_allocator();
		InitOptions init_options(allocator);

	rcl_allocator_t allocator = rcl_get_default_allocator();
	InitOptions init_options(allocator);
	InitOptions init_options(rcl_get_default_allocator());

Conversation

sloretz commented Mar 16, 2021

Uh oh!

hidmic left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sloretz commented Mar 18, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hidmic commented Mar 18, 2021

Uh oh!

sloretz commented Mar 18, 2021

Uh oh!

sloretz commented Mar 18, 2021

Uh oh!

sloretz commented Mar 22, 2021

Uh oh!

sloretz commented Mar 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sloretz commented Mar 22, 2021

Uh oh!

sloretz commented Mar 22, 2021

Uh oh!

hidmic commented Mar 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sloretz commented Mar 22, 2021

Uh oh!

hidmic left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sloretz commented Mar 22, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

sloretz commented Mar 22, 2021 •

edited

Loading

hidmic commented Mar 22, 2021 •

edited

Loading