Conversation
This follows the convention for human-readable diagnostic output on Linux systems. fix ros2#168 Signed-off-by: Dan Rose <dan@digilabs.io>
19b6869 to
f953981
Compare
|
I'm going to weigh in on this, I took a look around at some popular logging utilities in different languages to see what they did. This is obviously non-exhaustive, but hopefully somewhat representative. Python logging module - Prints all output, at all levels, to stderr by default Given that survey, it seems to me that ROS 2 should definitely choose a single stream for all logging output by default. Given the results above, it actually doesn't seem that clear to me which stream the default should be. I don't care hugely one way or the other, but I'd lean towards making the default stderr (as in this PR), as in the case of a real error, you want to make sure the message gets flushed out immediately. But we should also have a way to switch to the stdout stream, as nearly all loggers surveyed above have a way to do that. So with all of that said, I'm going to do a review of this PR. |
| # define IS_STREAM_A_TTY(stream) (isatty(fileno(stream)) != 0) | ||
| #endif | ||
|
|
||
| #define IS_OUTPUT_COLORIZED(is_colorized, stream) \ |
There was a problem hiding this comment.
Here and elsewhere, instead of removing the stream, let's leave it configurable and add a new environment variable to let users choose between stderr and stdout. I don't care hugely what its called, but something descriptive would be nice. In my opinion, if the environment variable is not specified, the default should be stderr.
There was a problem hiding this comment.
@clalancette Why should it be reconfigurable here?
- If the user wants all program output in a single stream, that's just
2>&1. - If it's about performance, then we should instead change the
stderrbuffering withsetvbuf. - If we don't expect stream redirection to be available (or we want to make sure the user can differentiate from other types of program output), we should additionally allow the user to specify an arbitrary file, not just a stream.
There was a problem hiding this comment.
@clalancette Why should it be reconfigurable here?
So the 2>&1 isn't that straightforward to achieve when you are launching through a launch file. While it would be kind of weird to use stdout instead of stderr in one process of a launch file and not in another, it isn't invalid.
The other reason to have it is that all of the log libraries I surveyed have the ability to choose which stream to use. Given that they all have it, presumably it is functionality that is useful in a logging library, so I think we should probably follow suit here.
There was a problem hiding this comment.
(also, because there is a small performance impact from using stderr, we should give users the ability to opt out of it and get buffered performance if they want)
|
I did some additional work on this (including a rebase), but I'm not allowed to push to the branch here. I'm going to close out this PR, and open a new one with my new changes and replicating the discussion here. |
|
Thank you for taking this over, @clalancette! |
This follows the convention for human-readable diagnostic output on Linux systems.
fix #168