-
Notifications
You must be signed in to change notification settings - Fork 36
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
Conversation
This has just the header files that the implementations need to provide, and that the callers can call. Signed-off-by: Chris Lalancette <[email protected]>
Signed-off-by: Chris Lalancette <[email protected]>
Signed-off-by: Chris Lalancette <[email protected]>
Signed-off-by: Chris Lalancette <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM
rcl_logging_interface/include/rcl_logging_interface/visibility_control.h
Outdated
Show resolved
Hide resolved
Signed-off-by: Chris Lalancette <[email protected]>
There was a problem hiding this 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"?
Signed-off-by: Chris Lalancette <[email protected]>
Yeah, good point. Added in 655ee72. |
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. |
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.