Skip to content

Convert logging mutex functions to pybind11#735

Merged
sloretz merged 6 commits intomasterfrom
pybind11_logging_mutex_funcs
Mar 29, 2021
Merged

Convert logging mutex functions to pybind11#735
sloretz merged 6 commits intomasterfrom
pybind11_logging_mutex_funcs

Conversation

@sloretz
Copy link
Copy Markdown
Contributor

@sloretz sloretz commented Mar 24, 2021

Part of #665

Converts these functions to pybind11

  • rclpy_logging_configure
  • rclpy_logging_fini
  • rclpy_create_node

@sloretz sloretz self-assigned this Mar 24, 2021
@sloretz
Copy link
Copy Markdown
Contributor Author

sloretz commented Mar 24, 2021

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

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

@sloretz sloretz changed the title Pybind11 logging mutex funcs Convert logging mutex functions to pybind11 Mar 24, 2021
@sloretz sloretz mentioned this pull request Mar 24, 2021
Closed
34 tasks
@sloretz sloretz force-pushed the pybind11_logging_mutex_funcs branch from 4fa053c to 4a901ce Compare March 24, 2021 21:12
hidmic
hidmic reviewed Mar 24, 2021
View reviewed changes
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 !

rclpy/src/rclpy/logging.cpp
#include "logging.hpp"

extern "C"
{
Copy link
Copy Markdown
Contributor

@hidmic hidmic Mar 24, 2021

Choose a reason for hiding this comment

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

@sloretz meta: why the extern "C" block? AFAIK it only affects the function symbol, C and C++ are ABI compatible.

Copy link
Copy Markdown
Contributor

@hidmic hidmic Mar 24, 2021

Choose a reason for hiding this comment

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

On a related note, we should probably ensure symbol locality (i.e. static, anonymous namespace).

Copy link
Copy Markdown
Contributor Author

@sloretz sloretz Mar 24, 2021

Choose a reason for hiding this comment

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

Oops, I mistakenly thought extern "C" changed the calling convention, but it does not. Removed extern C stuff and made appropriate functions static in 570b63d

rclpy/src/rclpy/node.cpp
}

if (RCL_RET_BAD_ALLOC == ret) {
throw std::bad_alloc();
Copy link
Copy Markdown
Contributor

@hidmic hidmic Mar 24, 2021

Choose a reason for hiding this comment

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

@sloretz consider doing rcl_reset_error() before throwing.

Copy link
Copy Markdown
Contributor Author

@sloretz sloretz Mar 24, 2021

Choose a reason for hiding this comment

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

Reset the rcl error in 336ca4f

@sloretz
Copy link
Copy Markdown
Contributor Author

sloretz commented Mar 24, 2021

CI with feedback addressed

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

@sloretz sloretz force-pushed the pybind11_logging_mutex_funcs branch from 570b63d to fbe3fda Compare March 25, 2021 17:35
@sloretz
Copy link
Copy Markdown
Contributor Author

sloretz commented Mar 25, 2021

Rebased, and running windows CI to hopefully get a line number number for the warning

Windows: Build Status

@sloretz
Copy link
Copy Markdown
Contributor Author

sloretz commented Mar 25, 2021

Windows CI with hopefully a fix for the warning: Build Status

hidmic
hidmic approved these changes Mar 26, 2021
View reviewed changes
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.

LGTM !

rclpy/src/rclpy/logging.cpp Outdated
try {
rclpy::LoggingGuard scoped_logging_guard;
rcl_logging_multiple_output_handler(location, severity, name, timestamp, format, args);
} catch (std::exception & ex) {
Copy link
Copy Markdown
Contributor

@hidmic hidmic Mar 26, 2021

Choose a reason for hiding this comment

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

@sloretz nit: could be made const?

Copy link
Copy Markdown
Contributor Author

@sloretz sloretz Mar 29, 2021

Choose a reason for hiding this comment

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

Made exception catching const in 6dee6f8

sloretz added 5 commits March 29, 2021 09:29
@sloretz
Convert logging_configure/_fini and create_node to pybind11
bb96de7 
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
@sloretz
Reset error after rcl alloc failure
a8c5fa0 
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
@sloretz
make _rclpy_destroy_node static
a798aeb 
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
@sloretz
Remove extern C stuff
4745b3e 
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
@sloretz
Fix windows signed/unsigned warning
1f2ab6b 
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
@sloretz sloretz force-pushed the pybind11_logging_mutex_funcs branch from b9d6b31 to 1f2ab6b Compare March 29, 2021 16:29
@sloretz
Const exception
6dee6f8 
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
@sloretz
Copy link
Copy Markdown
Contributor Author

sloretz commented Mar 29, 2021

Rebased, CI (build: --packages-above-and-dependencies rclpy test: --packages-above rclpy)

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

@sloretz
Copy link
Copy Markdown
Contributor Author

sloretz commented Mar 29, 2021

CMake warning is in cyclondedds, not this PR. Merging 🎉

@sloretz sloretz merged commit 28bcd82 into master Mar 29, 2021
@delete-merged-branch delete-merged-branch bot deleted the pybind11_logging_mutex_funcs branch March 29, 2021 20:32
@hidmic hidmic mentioned this pull request Mar 29, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@hidmic hidmic hidmic approved these changes

Assignees

@sloretz sloretz

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@sloretz @hidmic