Allow configuring logging directory through environment variables

[christophebedard]

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

• Use $ROS_LOG_DIR if ROS_LOG_DIR is set and not empty.
• Otherwise, use $ROS_HOME/log, using ~/.ros for ROS_HOME if not set or if empty.

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 ros2/rcpputils#98

[christophebedard]

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]

@ros-pull-request-builder retest this please

[christophebedard]

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

[christophebedard]

@jacobperron could you take a look?

[ivanpauno]

I have left a bunch of comment here and there, but this seems to be going in the right direction.
Thanks for working on this @christophebedard !

[ivanpauno]

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

[ivanpauno]

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

[christophebedard]

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

yeah, it requires ros2/rcpputils#98

[christophebedard]

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 49fdcd8

oh I missed your edit

[christophebedard]

@ros-pull-request-builder retest this please

[christophebedard]

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:

• Linux
• Linux-aarch64
• macOS
• Windows

[christophebedard]

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]

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

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:

• Linux
• Linux-aarch64
• macOS
• Windows

[christophebedard]

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

Windows rebuild:

• Windows

[clalancette]

All right, this is starting to look better. I still found a few more memory leaks to clean up.

[clalancette]

This looks good to me now. Thanks for the iterations.

I'll run CI on it next.

[clalancette]

CI:

• Linux
• Linux-aarch64
• macOS
• Windows

[ivanpauno]

Merging together with ros2/launch#460, thanks @christophebedard for the contribution!

[christophebedard]

thank you for the review @ivanpauno et al. 😄

[tgreier]

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?

[ivanpauno]

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]

@jacobperron @ivanpauno
I backported (locally) #41 and #53 and I now get the functionality that I want from ros2/launch#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.