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

Adding new cli parameters for configuring the logging. #327

Merged
merged 12 commits into from Dec 7, 2018

Conversation

Projects
None yet
6 participants
@nburek
Copy link
Contributor

nburek commented Nov 13, 2018

Summary

This change is part of a set of changes for new logging features. These changes include the following features:

rcutils

  • Slight refactor to the buffer used in logging to make it more generic and reusable
  • A new output handler that allows multiple output handlers to be set (default is still only the console output function). This includes the following three output handlers: 1) The console output handler 2) The external logging lib output handler 3) The rosout output handler
  • Change to the output format to accept only the log string instead of the format and vargs. The log will be formatted before being sent to the output so that there will be a consistent format across all the outputs
  • Added an interface for an external logging library, an output handler function for sending logs to this library, and cmake changes so that the external logging lib can be specified (default is rc_logging_log4cxx)

rcl

  • Added new command line parameters for disabling various logging output handlers
  • Added new command line parameter for passing in a config file to the external logging library
  • Called the new configuration function in the rcutils logging to configure the logging during the ROS init process

rclcpp

  • Fixing a broken unit test

rc_logging_log4cxx

  • An external logging implementation that uses log4cxx under the hood
  • Sets up a default FileAppender logger to forward logs to files
  • Uses the process Id as the filename by default

Remaining Feature Work

  • The rosout handlers is stubbed out in rcutils right now, but more work will need to be done to actually support sending logs to a rosout topic on a node. This will likely include moving the rosout handler up to rcl so that it can use the node interfaces to publish.

Links

https://discourse.ros.org/t/ros2-logging/6469
https://github.com/nburek/rc_logging_log4cxx

Pull requests for change

rclcpp: ros2/rclcpp#582
rcl: #327
rcutils: ros2/rcutils#127

@wjwwood
Copy link
Member

wjwwood left a comment

I have one blocking question about pointer arithmetic and some nitpick's, but otherwise lgtm! I'll get on the reviews for the related pr's before running CI.

Show resolved Hide resolved rcl/src/rcl/arguments.c Outdated
Show resolved Hide resolved rcl/src/rcl/arguments.c Outdated
Show resolved Hide resolved rcl/src/rcl/arguments.c Outdated
Show resolved Hide resolved rcl/src/rcl/arguments.c Outdated
Show resolved Hide resolved rcl/src/rcl/arguments.c
Show resolved Hide resolved rcl/src/rcl/arguments_impl.h
Show resolved Hide resolved rcl/src/rcl/rcl.c Outdated
@tfoote

This comment has been minimized.

Copy link
Contributor

tfoote commented Nov 21, 2018

Running CI

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

This comment has been minimized.

Copy link
Member

wjwwood commented Nov 21, 2018

@tfoote this will definitely fail CI due to style changes, and I suspect the compiler failures are due to upstream changes which will require this to be rebased to fix.

@nburek

This comment has been minimized.

Copy link
Contributor

nburek commented Nov 26, 2018

I have one blocking question about pointer arithmetic and some nitpick's, but otherwise lgtm! I'll get on the reviews for the related pr's before running CI.

Thanks for the feedback. I missed the linter failures, so I will correct those and make sure all the other tests are passing before updating. I'll also rebase to make sure I'm up to date on API changes. I'll also address all of the documentation requests above too.

@nburek nburek force-pushed the nburek:logging branch from 6b46a00 to 2914917 Nov 28, 2018

@nburek

This comment has been minimized.

Copy link
Contributor

nburek commented Nov 28, 2018

I ran ament_uncrustify and had it fix all the formatting for the whole repository. I addressed all the formatting issues as well and all unit tests were passing locally on a Docker Linux image.

The major change in this revision was to move the external logger, rosout, and multiple output log handlers into rcl instead of having them in rcutils. This was in response to the comments on the rcutils review suggesting that they not remain in rcutils. If you have a different place you'd prefer these to live I'd be more than happy to move them there instead.

Show resolved Hide resolved rcl/src/rcl/graph.c Outdated
@nburek

This comment has been minimized.

Copy link
Contributor

nburek commented Nov 28, 2018

It looks like the automated build for this is failing because of the new package that I added not being available. Currently I've just posted this package in a repository on my personal account (here: https://github.com/nburek/rc_logging_log4cxx) because I think it should live in a new repository under the ros2 organization. Let me know how you would like to handle creating/reviewing this code.

@nburek nburek force-pushed the nburek:logging branch 2 times, most recently from 059f8e1 to ee53228 Nov 28, 2018

@nburek

This comment has been minimized.

Copy link
Contributor

nburek commented Dec 4, 2018

Rebased on top of master and migrated changes made to rcl.c into init.c

@mjcarroll

This comment has been minimized.

Copy link
Contributor

mjcarroll commented Dec 5, 2018

CI with all the rebases:

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

This comment has been minimized.

Copy link
Contributor

mjcarroll commented Dec 5, 2018

CI Failures because I didn't include rc_logging_log4cxx, rerunning:

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

This comment has been minimized.

Copy link
Contributor

mjcarroll commented Dec 5, 2018

It's going to fail due to log4cxx dependency not being on the CI at the moment.

@mjcarroll

This comment has been minimized.

Copy link
Contributor

mjcarroll commented Dec 5, 2018

@nburek I commented over on the repo, but I wanted to note it here as well. In order for this to make it in, we'll need a way to satisfy the log4cxx dependency on all of our target platforms. I couldn't find a readily available choco package or vendor package for Windows. ros2/rcl_logging#1

@nburek nburek referenced this pull request Dec 5, 2018

Merged

Publish logs to Rosout #350

@nburek

This comment has been minimized.

Copy link
Contributor

nburek commented Dec 5, 2018

Hey Michael, sorry I missed the log4cxx dependency issue. Since it was available on Linux via rosdep I took for granted that it would be available on other platforms as well. I'm actively working on trying to get this unblocked. If you have examples of other vendor packages I could reference that would be useful.

In the mean time, if it would help unblock these Pull Requests and the new Pull Request I just opened for the rosout topic feature, I can swap out the rc_logger_log4cxx implementation that we're defaulting to with a no-op implementation then follow up with the rc_logger_log4cxx once that's actually working on all platforms.

@nburek

This comment has been minimized.

Copy link
Contributor

nburek commented Dec 5, 2018

Michael, I've added a noop logger to the rc_logger_log4cxx repository and put a COLCON_IGNORE in the rc_logger_log4cxx package. If you guys are okay with that we can move forward with this PR and the rosout PR while we work on solving the dependency issue for Windows and MAC.

@mjcarroll

This comment has been minimized.

Copy link
Contributor

mjcarroll commented Dec 5, 2018

Here are a few of our vendor_ packages that we've had to make in the past. In general, they default to using the system package if available, otherwise they are built using some variant of CMake's ExternalProject: https://github.com/ros2?utf8=%E2%9C%93&q=vendor&type=&language=

@mjcarroll

This comment has been minimized.

Copy link
Contributor

mjcarroll commented Dec 5, 2018

CI with noop:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status
Temporarily switching the default to noop logger ext_lib while system…
… dependency issue is solved for Windows/Mac.
@nburek

This comment has been minimized.

Copy link
Contributor

nburek commented Dec 5, 2018

Latest build failuire on Linux and macOS are because I need to rebase to pick up a change in rcl_actions that's breaking due to a change fields in a struct from another repository.

The Windows failure is because it's failing to import the library exported by rc_logger_noop. I'm just finishing provisioning a Windows machine and should be able to start troubleshooting that as well.

@nburek nburek force-pushed the nburek:logging branch from bc09c0b to 5cddfc2 Dec 5, 2018

@mjcarroll

This comment has been minimized.

Copy link
Contributor

mjcarroll commented Dec 5, 2018

With the rebase:

Linux: Build Status

macOS: Build Status

@nburek

This comment has been minimized.

Copy link
Contributor

nburek commented Dec 5, 2018

I've pushed a slight change to rc_logging_noop (now with the renamed github repository rc_logging) that forces the Windows build to compile rc_logging_noop as a static library as that seemed to be what it wanted. This unblocks the build, but I am still working on getting the unit tests to run as my windows machine doesn't seem to be able to link in fastrtps during runtime.

I've also submitted an additional PR and linked it to this one in order to add nburek/rc_logging to ros2.repos file in ros2/ros2

@nburek

This comment has been minimized.

Copy link
Contributor

nburek commented Dec 5, 2018

Looks like the unstable build on Linux and Mac is caused by two issues:

  1. The xmllint is failing for rcl because I had to increase the package.xml version to 3 in order to add the group_depend and it doesn't seem to know how to deal with a version 3 file.
  2. There are uncrustify issues with rclcpp. Unless I missed something though, these issues appear to be with files that I did not touch in my PR.
Show resolved Hide resolved rcl/package.xml Outdated
@nburek

This comment has been minimized.

Copy link
Contributor

nburek commented Dec 6, 2018

Newest update here fixes the xmllint issue with rcl. We were also able to build rcl with the rc_logging_noop package from the rc_logging repository on Windows and were able to run some quick manually testing that it was working.

@mjcarroll

This comment has been minimized.

Copy link
Contributor

mjcarroll commented Dec 6, 2018

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

wjwwood added some commits Dec 6, 2018

style changes, and one actual fix
Signed-off-by: William Woodall <william@osrfoundation.org>
rename rc_logging* to rcl_logging*
Signed-off-by: William Woodall <william@osrfoundation.org>

mjcarroll and others added some commits Dec 6, 2018

Expand rcl_logging_configure API with allocator
This is to make sure that this API is complete, and we can merge rcl#350 later without breaking the API at a later date.
@nburek

This comment has been minimized.

Copy link
Contributor

nburek commented Dec 6, 2018

Changes based on feedback in ros2/rcutils#127 to pass va_list to output handlers.

@wjwwood
Copy link
Member

wjwwood left a comment

Another few small changes.

Show resolved Hide resolved rcl/src/rcl/logging.c Outdated
Show resolved Hide resolved rcl/src/rcl/logging.c
Show resolved Hide resolved rcl/src/rcl/logging.c Outdated
address review comments
Signed-off-by: William Woodall <william@osrfoundation.org>
@wjwwood

wjwwood approved these changes Dec 6, 2018

@wjwwood wjwwood merged commit 228cd00 into ros2:master Dec 7, 2018

1 check failed

Cpr__rcl__ubuntu_bionic_amd64 Build finished.
Details

@wjwwood wjwwood removed the in review label Dec 7, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment