Changing the default location for log files to be a local directory instead of /var/log/ros on linux due to permission issues.#9
Conversation
|
Please use |
|
@nburek We're going to look at changing this to use |
|
Update: I have a prototype that puts logs in As part of the prototype, I needed an API change to the external logger initialization so I could allocate memory. That is now in as #10 and ros2/rcl#430 . There is also a need to add some rcutils methods to support things like getting the home directory, executable name, etc. in a cross-platform way. But since those are strictly API additions, they can come later. Tomorrow I'll do some additional cleanup on the prototype, test it on Windows and macOS, and then open the PR to rcutils and update this PR with my latest code. |
…nstead of /var/log/ros on linux due to permission issues.
e1aa215 to
dd6848b
Compare
This works on Linux, but still needs to be cleaned up and tested on macOS/Windows. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
dd6848b to
945e966
Compare
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
clalancette
left a comment
There was a problem hiding this comment.
I'm approving, but since I did a bunch of new work here, it's probably best if someone else reviews as well. @nuclearsandwich @wjwwood , mind reviewing this as well?
|
I can't approve since I created the original PR, but the changes look good to me. |
| } | ||
|
|
||
| char log_name_buffer[512] = {0}; | ||
| snprintf(log_name_buffer, sizeof(log_name_buffer), "%s/.ros/log/%s_%i_%" PRId64 ".log", homedir, basec, rcutils_get_pid(), ms_since_epoch); |
There was a problem hiding this comment.
This line looks really long, is there any linting happening in this file?
There was a problem hiding this comment.
That's true. The linter isn't enabled on this package. I'll open an issue to track turning on the linter and dealing with the fallout.
| int64_t ms_since_epoch = RCUTILS_NS_TO_MS(now); | ||
|
|
||
| // Get the program name. | ||
| char *basec = rcutils_get_executable_name(allocator); |
There was a problem hiding this comment.
Just curious what does basec mean? Why not executable_name?
There was a problem hiding this comment.
It is short for "basename copy", but I agree that executable_name would be a better term. I'll do a followup PR tomorrow to rename this.
There was a problem hiding this comment.
I agree that
executable_namewould be a better term. I'll do a followup PR tomorrow to rename this.
I've already got this WIP.
| add_compile_options(-Wall -Wextra -Wpedantic) | ||
| else() | ||
| # Disable ddl-interface warnings for STL from log4cxx code, because we are already | ||
| # Disable ddl-interface warnings for STL from log4cxx code, because we are already |
|
This patch might be responsible for this build failure: https://ci.ros2.org/view/nightly/job/nightly_linux_clang_libcxx/45/consoleFull#console-section-286 Please double check and address the problem asap. Thanks. |
I started looking into this and it appears that the issue is that log4cxx isn't able to link when you compile with libcxx. I suspect this is because log4cxx is exposing std library objects in their API and these are different between the std lib that log4cxx was compiled with and libcxx. |
|
@emersonknapp FYI |
|
I attempted to get log4cxx building with libc++ to see if we could do a build from source for this nightly CI job or switch to using a vendor package for log4cxx, but am running into issues getting the Apache Portable Runtime (APR) to compile with libc++. After talking with @emersonknapp, we think that the quickest way to unblock this nightly CI is to swap the logging implementation using the "RCL_LOGGING_IMPLEMENTATION" environment variable so that it isn't trying to link against log4cxx. We can open a Github issue to track supporting libc++ for rcl_logging_log4cxx if that's something we decide is worth doing. @dirk-thomas Do you agree with this approach? |
|
Sounds fine with me. |
|
I've started a PR to fix the nightly build: ros2/ci#291 |
During the review of this PR: ros2/rcl#425 we found that there were permission issues with writing to /var/log. The quick fix for this is to use the same behavior that we have on Windows, which is to log to a local directory instead.