Skip to content

Enabling the build for rcl_logging_log4cxx#3

Merged
mjcarroll merged 9 commits intoros2:masterfrom
nburek:master
Feb 26, 2019
Merged

Enabling the build for rcl_logging_log4cxx#3
mjcarroll merged 9 commits intoros2:masterfrom
nburek:master

Conversation

@nburek
Copy link
Copy Markdown
Contributor

@nburek nburek commented Dec 21, 2018 •
edited by mjcarroll
Loading

This review includes all the fixes needed to build and run the rcl_logging_log4cxx package on Linux, Mac, and Windows 10. In order for this to work in the build system we will need use brew to install log4cxx on Mac and use the choco package in the related PR in order to install log4cxx on Windows.

There is a known issue with the Windows build that causes log4cxx to crash when compiled as a Release build, but works if it is compiled in Debug mode. As a temporary work around we force the Windows build of rcl_logging_log4cxx to use Debug mode.

Requires ros2/choco-packages#7

@ross-desmond ross-desmond mentioned this pull request Dec 21, 2018
Merged
@nburek nburek mentioned this pull request Dec 21, 2018
Closed
@nburek
Copy link
Copy Markdown
Contributor Author

nburek commented Jan 16, 2019

It'd be great if we could get this and the related choco package change merged in for the next ROS2 Crystal patch (ros2/ros2#647). What are the next steps for this PR? Do you need my help with anything?

@cottsay cottsay added the ready label Jan 17, 2019
@nuclearsandwich nuclearsandwich mentioned this pull request Feb 5, 2019
Closed
@nuclearsandwich nuclearsandwich mentioned this pull request Feb 15, 2019
Closed
10 tasks
@mjcarroll
Copy link
Copy Markdown
Member

mjcarroll commented Feb 19, 2019 •
edited
Loading

With the choco package in, this should work now?

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

mjcarroll pushed a commit to ros2/ci that referenced this pull request Feb 19, 2019
Add liblog4cxx-dev
9f8e54d 
Needed in support of: ros2/rcl_logging#3
This was referenced Feb 19, 2019
Closed
Closed
@mjcarroll
Copy link
Copy Markdown
Member

mjcarroll commented Feb 19, 2019

After installing choco packages on Windows (via ros2/build_farmer#164)

  • Windows Build Status

@mjcarroll
Copy link
Copy Markdown
Member

mjcarroll commented Feb 19, 2019 •
edited
Loading

Rebuild with log4cxx installed on macOS and Windows, subset of packages.

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

@mjcarroll
Copy link
Copy Markdown
Member

mjcarroll commented Feb 19, 2019

Alright @nburek looks mostly good, with a few warnings on Windows CI. Mind getting those cleaned up?

@mjcarroll mjcarroll mentioned this pull request Feb 19, 2019
Merged
@mjcarroll
Copy link
Copy Markdown
Member

mjcarroll commented Feb 19, 2019

Also, can you add a PR for installation documentation (https://github.com/ros2/ros2_documentation/tree/master/source/Installation) to show that

  1. We need to install the log4cxx choco package on Windows source/binary
  2. We need to install log4cxx via brew on macOS.

@nburek
Copy link
Copy Markdown
Contributor Author

nburek commented Feb 19, 2019

Someone from our team is starting to fix the windows warning and I'm working on adding the documentation to the install instructions.

Will update as soon as we're ready. Thanks.

@nburek nburek mentioned this pull request Feb 20, 2019
Merged
@nburek
Copy link
Copy Markdown
Contributor Author

nburek commented Feb 20, 2019

Documentation updated here: ros2/ros2_documentation#127

We are still working on cleaning up the warnings.

@juanrh
Copy link
Copy Markdown

juanrh commented Feb 21, 2019

All windows warnings should be solved now. I have disabled C4275 and C4251 because, as discussed here

I recommend avoiding this in the first place - putting STL types in your DLL's interface forces you to play by the STL's rules (specifically, you can't mix different major versions of VC, and your IDL settings must match). However, there is a workaround. C4251 is essentially noise and can be silenced...

and those warnings happen on Log4cxx's code. I have added a comment about that in CMakeLists.txt next to where those warnings are disabled.

@nburek
Copy link
Copy Markdown
Contributor Author

nburek commented Feb 21, 2019

This will need the CI build run again to show the Windows warnings were fixed before merge.

Thanks.

mjcarroll pushed a commit to ros2/ci that referenced this pull request Feb 21, 2019
Add liblog4cxx-dev
14b7295 
Needed in support of: ros2/rcl_logging#3
@mjcarroll
Copy link
Copy Markdown
Member

mjcarroll commented Feb 21, 2019 •
edited
Loading

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

mjcarroll pushed a commit to ros2/ci that referenced this pull request Feb 21, 2019
Add liblog4cxx-dev
0067350 
Needed in support of: ros2/rcl_logging#3
mjcarroll pushed a commit to ros2/ci that referenced this pull request Feb 21, 2019
Add liblog4cxx-dev
519f19d 
Needed in support of: ros2/rcl_logging#3
mjcarroll pushed a commit to ros2/ci that referenced this pull request Feb 26, 2019
Add log4cxx for rcl_logging_log4cxx (#247)
664672e 
* Wrap build and test dependencies

* Add liblog4cxx-dev

Needed in support of: ros2/rcl_logging#3
@mjcarroll mjcarroll merged commit efeebc9 into ros2:master Feb 26, 2019
@mjcarroll mjcarroll removed the ready label Feb 26, 2019
bhatsach added a commit to bhatsach/rcl_logging that referenced this pull request Feb 27, 2019
@bhatsach
Revert "Enabling the build for rcl_logging_log4cxx (ros2#3)"
fe85881 
This reverts commit efeebc9.
@nuclearsandwich nuclearsandwich mentioned this pull request Mar 8, 2019
Merged
dirk-thomas
dirk-thomas reviewed Apr 17, 2020
View reviewed changes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@dirk-thomas dirk-thomas dirk-thomas left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@mjcarroll mjcarroll

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@nburek @mjcarroll @juanrh @dirk-thomas @cottsay @yyu