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

Remove rcl_logging_log4cxx. #78

Merged
merged 1 commit into from
Sep 14, 2021
Merged

Conversation

clalancette
Copy link
Contributor

We don't ever use it, and having it around causes a few problems:

  1. We have an unnecessary, and unneeded dependency on log4cxx in the
    core.
  2. We can't currently build against libc++ with log4cxx.
  3. Log4cxx causes some absolute paths to be in the delivered packages
    on Windows.

Signed-off-by: Chris Lalancette clalancette@openrobotics.org

@ivanpauno FYI, this should get rid of one more of the problems pointed out in ros2/rclcpp#1688

We don't ever use it, and having it around causes a few problems:

1.  We have an unnecessary, and unneeded dependency on log4cxx in the
core.
2.  We can't currently build against libc++ with log4cxx.
3.  Log4cxx causes some absolute paths to be in the delivered packages
on Windows.

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

Full CI:

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

Copy link
Member

@ivanpauno ivanpauno left a comment

Choose a reason for hiding this comment

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

Sounds fine to me

@clalancette
Copy link
Contributor Author

The other good thing about doing this is that we'll be able to get rid of the log4cxx dependency.

Before I go ahead with this, any objections @cottsay or @nuclearsandwich ?

@clalancette
Copy link
Contributor Author

Even though the full CI was a month ago, nothing substantial has changed here. So I'm just going to rely on that and the approval from @ivanpauno, and go ahead and merge this. There is probably some follow-up work in the documentation and the CI jobs that needs to be done, which I'll look at next.

@clalancette clalancette merged commit 7846c05 into master Sep 14, 2021
@delete-merged-branch delete-merged-branch bot deleted the clalancette/remove-log4cxx branch September 14, 2021 19:14
clalancette added a commit to clalancette/ros2_documentation that referenced this pull request Sep 14, 2021
It is no longer required as of ros2/rcl_logging#78

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
clalancette added a commit to ros2/ros2_documentation that referenced this pull request Sep 14, 2021
It is no longer required as of ros2/rcl_logging#78

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
@christophebedard christophebedard mentioned this pull request Mar 1, 2024
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