Skip to content

Conversation

@tgreier
Copy link

@tgreier tgreier commented Dec 21, 2020

Backport to Foxy the following PRs:

* Add in a new rcl_logging_interface package.

This has just the header files that the implementations need
to provide, and that the callers can call.  Also port log4cxx,
noop, and spdlog backends to use this new interface.

Signed-off-by: Chris Lalancette <[email protected]>
Signed-off-by: tgreier <[email protected]>
* Used current_path function from rcpputils

Signed-off-by: ahcorde <[email protected]>

* Removed fs namespace

Signed-off-by: ahcorde <[email protected]>
Signed-off-by: tgreier <[email protected]>
Signed-off-by: Alejandro Hernández Cordero <[email protected]>
Signed-off-by: tgreier <[email protected]>
@jacobperron
Copy link
Member

jacobperron commented Dec 21, 2020

This looks like a large change. It would be better to incrementally open up separate backports for each PR referenced.

Also, we should make sure that anything we backport does not break things for existing users. For example, we can't remove public headers, since we'll break any downstream code currently including them. It might be possible to workaround this issue by including the equivalent header from rcl_logging_interface from inside the old headers, if all of the function signatures are the same.

Correct me if I'm wrong, but at a glance, it looks like it should be possible to backport #53 without the other three referenced PRs. It's not as simple as cherry-picking the change, but presumably we could add equivalent API to the existing logging packages in Foxy.

@tgreier
Copy link
Author

tgreier commented Dec 21, 2020

Ok, I can kill this PR and take the approach of adding the functionality to Foxy without deleting any existing public headers (no cherry-picking).

@jacobperron
Copy link
Member

Ok, I can kill this PR and take the approach of adding the functionality to Foxy without deleting any existing public headers (no cherry-picking).

Sounds good. I'll go ahead and close this one then.

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.

2 participants