search

ros2 / rcl_logging

Logging implementations for ROS 2.
Apache License 2.0
20 stars 36 forks source link

Logging interface #41

Closed clalancette closed 4 years ago

clalancette commented 4 years ago

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.

clalancette commented 4 years ago

CI:

clalancette commented 4 years ago

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 commented 4 years ago

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

clalancette commented 4 years ago

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.