Skip to content

Conversation

@ivanpauno
Copy link
Member

Signed-off-by: ivanpauno <[email protected]>
@ivanpauno ivanpauno added the in review Waiting for review (Kanban column) label May 14, 2019
@ivanpauno ivanpauno self-assigned this May 14, 2019
@dirk-thomas
Copy link
Member

It would be good to have an env var to control the behavior (force enable, force disable) and the default could be based on if the shell is interactive (if I recall correctly that is what other cases use).

…t variable.

* Stop adding colours in `rcutils_logging_format_message`. Add colours only in `rcutils_logging_console_output_handler`.

Signed-off-by: ivanpauno <[email protected]>
@ivanpauno
Copy link
Member Author

It would be good to have an env var to control the behavior (force enable, force disable) and the default could be based on if the shell is interactive (if I recall correctly that is what other cases use).

I added an env RCUTILS_COLORIZED_OUTPUT, possible values: FORCE_ENABLE, FORCE_DISABLE, DEFAULT.
I used isatty for checking if it is a terminal or not as default behavior.

I also moved the "add colour logic " from rcutils_logging_format_message to rcutils_logging_console_output_handler, as the first function is also used for formatting a message in other log functions (rcl/src/logging.c: rcl_logging_ext_lib_output_handler). I suppose we don't want to colorize that output.

@ivanpauno
Copy link
Member Author

An unrelated comment:
I think rcutils_logging_initialize is using strcmp in the wrong way with line_buffered (L83 and L86).

rcutils/src/logging.c

Lines 77 to 98 in 343e406

rcutils_ret_t rcutils_logging_initialize(void)
{
const char * line_buffered;
const char * ret_str = rcutils_get_env("RCUTILS_CONSOLE_STDOUT_LINE_BUFFERED", &line_buffered);
if (NULL == ret_str) {
if (strcmp(line_buffered, "1")) {
g_force_stdout_line_buffered = true;
} else {
if (!strcmp(line_buffered, "0")) {
fprintf(stderr,
"Warning: unexpected value [%s] specified for RCUTILS_CONSOLE_STDOUT_LINE_BUFFERED. "
"Default value 0 will be used. Valid values are 1 or 0.\n",
line_buffered);
}
}
} else {
fprintf(stderr, "Error getting env. variable "
"RCUTILS_CONSOLE_STDOUT_LINE_BUFFERED: %s\n", ret_str);
}
return rcutils_logging_initialize_with_allocator(rcutils_get_default_allocator());
}

Also, I think all the code related to RCUTILS_CONSOLE_STDOUT_LINE_BUFFERED should be moved to rcutils_logging_initialize_with_allocator.

@dirk-thomas if you agree, I will open another PR correcting that.

@dirk-thomas
Copy link
Member

possible values: FORCE_ENABLE, FORCE_DISABLE, DEFAULT.

Let's use standard boolean values for the new env var RCUTILS_COLORIZED_OUTPUT: 0 to disable the colorization, 1 to force using colorization and when no value is set use the automatic decision based on isatty.

@ivanpauno
Copy link
Member Author

possible values: FORCE_ENABLE, FORCE_DISABLE, DEFAULT.

Let's use standard boolean values for the new env var RCUTILS_COLORIZED_OUTPUT: 0 to disable the colorization, 1 to force using colorization and when no value is set use the automatic decision based on isatty.

Done!
Internally, I'm still using an enum with the three possibilities, as I have to defer the decision depending if the output stream is stdout or stderr.

ivanpauno added 4 commits May 15, 2019 09:24
Signed-off-by: ivanpauno <[email protected]>
Signed-off-by: ivanpauno <[email protected]>
} else { \
is_colorized = isatty(fileno(stream)) != 0; \
} \
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This macro is almost identical to the Windows implementation above. Please avoid that duplication and only customize the isatty call using a platform specific macro.

@ivanpauno
Copy link
Member Author

ivanpauno commented May 16, 2019

CI:

  • Linux Build Status (uncrustify failures and unrelated)
  • Linux-aarch64 Build Status (uncrustify failures and unrelated)
  • macOS Build Status (uncrustify failures and unrelated)
  • Windows Build Status (uncrustify failures, cppcheck false positive and unrelated)

@ivanpauno ivanpauno requested a review from dirk-thomas May 16, 2019 14:46
Signed-off-by: ivanpauno <[email protected]>
@ivanpauno ivanpauno requested a review from dirk-thomas May 16, 2019 19:03
@ivanpauno
Copy link
Member Author

ivanpauno commented May 16, 2019

I have tested locally that linter is passing in both windows and linux after e407efd. I'm going to skip running ci jobs again only because of linter failures.

@ivanpauno ivanpauno merged commit fe0342d into master May 16, 2019
@delete-merged-branch delete-merged-branch bot deleted the ivanpauno/colorized-output branch May 16, 2019 20:12
@ivanpauno ivanpauno removed the in review Waiting for review (Kanban column) label May 16, 2019
@peci1
Copy link

peci1 commented Jun 11, 2025

Just to keep information in one place, I'll amend to this years old PR.

When using ros2 launch, the nodes have isatty == False for both stdout and stderr. So to get colorized output of ros2 launch, you have to force it using the env variable.

I've added a documentation note about this caveat: ros2/ros2_documentation#5726 .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants