search

ros2 / rcl_logging

Logging implementations for ROS 2.
Apache License 2.0
18 stars 34 forks source link

Allow configuring logging directory through environment variables #53

Closed christophebedard closed 3 years ago

christophebedard commented 3 years ago

This allows configuring the logging directory through environment variables using the following logic:

Since this function is meant to be used by all logging implementations, I put it in rcl_logging_interface. I also added a test.

Closes #50

Requires https://github.com/ros2/rcpputils/pull/98

christophebedard commented 3 years ago

Since this function is meant to be used by all logging implementations, I put it in rcl_logging_interface.

But maybe it shouldn't be RCL_LOGGING_INTERFACE_PUBLIC/in the header directly?

christophebedard commented 3 years ago

@ros-pull-request-builder retest this please

christophebedard commented 3 years ago

oh, build.ros.org doesn't build everything from source.

christophebedard commented 3 years ago

@jacobperron could you take a look?

ivanpauno commented 3 years ago

By the way, the PR checker isn't passing.

ivanpauno commented 3 years ago

After addressing comments here, we should also update the logging tutorial: https://index.ros.org/doc/ros2/Tutorials/Logging-and-logger-configuration/.

christophebedard commented 3 years ago

By the way, the PR checker isn't passing.

yeah, it requires ros2/rcpputils#98

christophebedard commented 3 years ago

I think I've adressed all of your comments @ivanpauno @wjwwood!

I also added another simple test (inspired by https://github.com/ros2/launch/pull/460#discussion_r496923624) in a88529569551f9ec20ea93f46154bbb2d8350686

christophebedard commented 3 years ago

rcutils_format_string has an implicit 2048 max length, for some strange reason (see here).

I assumed 2048 was enough, but yeah there's no point in restricting it. I changed it in 49fdcd8b410e20087dcccd8eabd6853579233610

oh I missed your edit

christophebedard commented 3 years ago

@ros-pull-request-builder retest this please

christophebedard commented 3 years ago

Hope you don't mind, here's CI for this PR + ros2/launch#460 with --packages-above[-and-dependencies] rcl_logging_interface rcl_logging_log4cxx rcl_logging_noop rcl_logging_spdlog launch launch_xml launch_yaml:

christophebedard commented 3 years ago

like @ivanpauno pointed out previously, the tests aren't passing on Windows because they're using / as a path separator in environment variables. I'm just wondering which solution is more appropriate:

  1. change the tests to use the right path separator in the env var values, e.g. by #ifdefing or calling rcutils_to_native_path, or
  2. calling rcutils_to_native_path at the end of rcl_logging_get_logging_directory (although this would still require a bit of option 1)

Note that, in the Logging and logger configuration tutorial, I used forward slashes for Windows, so it wouldn't currently work with option 1: https://index.ros.org/doc/ros2/Tutorials/Logging-and-logger-configuration/#logging-directory-configuration

christophebedard commented 3 years ago

I ended up going with option 2 (+ a bit of option 1, as mentioned above) because it's more resilient: c8aae9e1b9ca47f5927b0f065fbd4e12e279d364

I've also made the same modification in ros2/launch#460. Let me know what you think. I've tested on both Ubuntu and Windows. Here's a CI rebuild:

christophebedard commented 3 years ago

I missed the MSBuild warning in the first CI run. Should be fixed in 1a4f0e01ab9b425c4c2db7a0cf44e792599b664d.

Windows rebuild:

clalancette commented 3 years ago

CI:

ivanpauno commented 3 years ago

Merging together with https://github.com/ros2/launch/pull/460, thanks @christophebedard for the contribution!

christophebedard commented 3 years ago

thank you for the review @ivanpauno et al. :smile:

tgreier commented 3 years ago

I would like to backport this PR to Foxy with https://github.com/ros2/launch/pull/460. However, the rcl_logging_interface was never merged into Foxy. I would need to backport quite a bit to get to this PR. I would first need to backport https://github.com/ros2/rcl_logging/pull/41 and then move forward. Do you think that it is ok to do that?

ivanpauno commented 3 years ago

I would like to backport this PR to Foxy with ros2/launch#460. However, the rcl_logging_interface was never merged into Foxy. I would need to backport quite a bit to get to this PR. I would first need to backport #41 and then move forward. Do you think that it is ok to do that?

cc @jacobperron

tgreier commented 3 years ago

@jacobperron @ivanpauno I backported (locally) https://github.com/ros2/rcl_logging/pull/41 and https://github.com/ros2/rcl_logging/pull/53 and I now get the functionality that I want from https://github.com/ros2/launch/pull/460. Is this acceptable to do or are there additional PRs/commits that I should backport between #41 and #53? I will need at least #51 and #52.