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.

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

Logging interface #41

Merged
merged 6 commits into from
Jun 9, 2020
Merged

Logging interface #41

merged 6 commits into from
Jun 9, 2020

Conversation

clalancette
Copy link
Contributor

@clalancette clalancette commented Jun 8, 2020

As discussed in several places, the current way that the rcl_logging API is working is quite odd. The API is currently being defined in rcl, implemented in rcl_logging, and then used in rcl. This essentially creates a circular dependency that we are breaking by copying a bunch of code around.

Instead of that, what this PR (and the counterpart in ros2/rcl#676) does is to define an rcl_logging_interface package. This package only installs and provides a header file. Then the logging backends depend on this interface and implement it. Finally, rcl also depends on the interface, and calls into whatever backend is setup as the default. This breaks the circular dependency.

@rotu @wjwwood @cottsay @hidmic FYI.

This has just the header files that the implementations need
to provide, and that the callers can call.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
@clalancette
Copy link
Contributor Author

CI:

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

Copy link
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

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
@clalancette clalancette marked this pull request as ready for review June 8, 2020 20:01
Copy link
Member

@cottsay cottsay left a comment

Choose a reason for hiding this comment

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

Thanks, Chris, this is great. Do we need to add a quality declaration since this new package will be in the "critical path"?

rcl_logging_noop/CMakeLists.txt Show resolved Hide resolved
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
@clalancette
Copy link
Contributor Author

Thanks, Chris, this is great. Do we need to add a quality declaration since this new package will be in the "critical path"?

Yeah, good point. Added in 655ee72.

@clalancette
Copy link
Contributor Author

All right, smoke-test CI looked good. Here is a full CI to make sure nothing wonky happens:

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

@clalancette
Copy link
Contributor Author

All right, Linux warnings are expected and in the nightlies.

macOS failures are all in the nightlies except one. But given that it is a failure in a service, I think that is probably flaky.

Windows is hard to tell, but there are 4 additional tests that failed here vs. the nightlies. All of those were in services as well, so I think we can chalk that up to flakiness.

Overall, it looks like this set of PRs doesn't have much impact (which is good!). Going ahead and merging, thanks for the reviews.

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.

None yet

3 participants