-
Notifications
You must be signed in to change notification settings - Fork 36
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
Implement a backend logger using spdlog. #17
Conversation
From spdlog's readme, the supported platforms are,
while the package managers are,
I did encounter versioning issues a while back with |
I'd like to hear what everyone thinks as following questions,
|
My opinions for your question, @clalancette
|
Yeah, that will be a problem with vendoring (either here in this package, or in a vendor package). So if we can (and as pointed out by others), it would be better to use packages and offload that work elsewhere.
Yeah, that's a good point. That is one reason to support the external configuration file, but on the the other hand, we can just add a new environment variable for that. I'll put it on the list to do.
I'm not going to worry too much about this unless/until someone comes up with a realistic scenario where this is a problem. If it does turn out to be a problem, then we can either enhance spdlog upstream, or switch to yet another logging implementation. Thanks for the feedback! |
OK, that was my feeling as well. I just wanted to make sure there wasn't some critical use case I was missing.
What confused me was that I wasn't using the launch system. That directory is showing up even when I just do
OK, I think it is clear that we want to use separate packages. It seems to already be packaged for Ubuntu, and available via Brew for macOS, so that's a good start. I'll leave it to @nuclearsandwich and @mjcarroll to give any feedback about chocolatey packaging (or whatever) for Windows. |
this is done by luanch.logging, ros2cli imports logging, |
log4cxx definitely set a new worst case bar for Chocolatey packaging.
|
@clalancette it wasn't immediately clear which version of spdlog is vendored here. Ubuntu bionic packages 0.16 but the current version in Homebrew is 1.3.1. |
This is still on my list to come back and look at for Eloquent, but I haven't had time for it yet. Probably September. |
The current status on this is the following:
@tfoote FYI |
Looking at the above list of platforms, I think it is broadly clear that we want to use spdlog 1.3.1, as that will get us compatibility on Buster and macOS/brew out-of-the-box. For Ubuntu Bionic and Windows, then, I think we should probably vendor it like we do in https://github.com/ros2/console_bridge_vendor/blob/master/CMakeLists.txt . Essentially, if we find spdlog 1.3.1 on the system, use that, otherwise vendor it ourselves. I'm going to start working on that. |
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
CI is definitely going to fail here because it needs the whole set of things in ros2/ros2#789 in order to succeed. |
It doesn't really know how to destruct itself. Just leave it around for a possible reinitialization. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Thanks for the initial review! All review comments have been addressed in b32968f. Please take another look when you have a chance. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ultimately, it looks like we might want to define a common logging API so that the backend can be easily changed. But I'm okay with this PR as-is, if we want to leave that as a follow-up task.
It would be nice to add linter tests.
Some more minor comments below.
So there actually is one that they all conform to: https://github.com/ros2/rcl/blob/d655147310e3e63aff1555ad40fca1bb9cb7783e/rcl/include/rcl/logging_external_interface.h . It's a little hard to find it, though, because of the way this whole thing was done. I honestly don't remember all of the details at the moment, but it definitely could be improved so that they use more common stuff. |
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
All review comments are addressed in e268e69. I've also added the linters and fixed things up there. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
The Epr job won't work until spdlog_vendor is released into eloquent. I think it would be nice if we got the PR job passing before merging this. |
@ros-pull-request-builder retest this please |
1 similar comment
@ros-pull-request-builder retest this please |
Woohoo, PR testing succeeded. Merging now. |
@clalancette For reference https://github.com/guangie88/spdlog_setup is a reasonable place to start when it comes to configuring spdlog from a file. I have used it on a few small projects and have been pleased. |
Is there somewhere a hint or manual on how to switch to spdlog? |
@maxlein |
That's correct; in Eloquent and later, spdlog is now the default. |
Thanks, I am on eloquent. |
Along with ros2/rcutils#166 and ros2/rcl#466, this PR is one way we could implement per-process logging in ROS 2. It uses a header-only C++ library called spdlog instead of log4cxx (which has a bunch of problems). I'm not at all tied to using this library, but it is relatively straightforward and has most of the features we want. The open questions with this PR (in rough order of priority):
ros2 run demo_nodes_cpp talker
), you get 2 things in~/.ros/log
; a directory of<date>-<hostname>-<pid>
, and a log file of<processname>-<pid>-<ms-since-epoch>
. This PR creates the latter; I'm not sure where the former is coming from. We should figure out our strategy here and only make one of them show up.ExternalProject
it or dospdlog_vendor
package separately.@ros2/team @nburek FYI.