Skip to content

Conversation

@saumanraaj
Copy link

Pull Request description:


Fix ANSI Escape Code Pollution - Environment-Aware bcolors

Summary

This PR fixes Issue #2592 by making the existing bcolors class environment-aware, ensuring that ANSI escape codes do not pollute ROS logs while maintaining backward compatibility.

Changes Made

  • Made the bcolors class respect the RCUTILS_COLORIZED_OUTPUT environment variable.

  • Automatically detect whether output is a TTY to enable/disable colors intelligently.

  • Removed all ANSI codes from logger calls in:

    • controller_manager_services.py

    • spawner.py

    • hardware_spawner.py

  • Retained bcolors usage across the codebase to avoid breaking existing behavior.

Environment Variable Behavior

Variable Behavior
RCUTILS_COLORIZED_OUTPUT=0 No colors in output
RCUTILS_COLORIZED_OUTPUT=1 Colors enabled
Unset Auto-detects TTY and enables colors only if supported

Testing and Verification

  • Verified that logs are now clean and contain no escape codes.

  • Confirmed that CLI output remains colorized when applicable.

  • Verified backward compatibility with existing code paths.

  • Ran pre-commit checks (Python hooks passed;

Checklist

  • Limited scope: focuses solely on Issue Option for uncolored log output #2592

  • Descriptive commit message and PR title

  • CI expected to pass

  • Logger outputs verified to be ANSI-free

  • Manual tests confirm correct color behavior

Contributing Notes

Before submitting PRs:

  1. Keep PRs focused on a single issue or feature.

  2. Use clear, descriptive titles and concise summaries.

  3. Ensure the CI pipeline passes.

  4. Request reviews from maintainers when ready.

  5. Add or update tests where applicable.


- Make bcolors class environment-aware to respect RCUTILS_COLORIZED_OUTPUT
- Remove ANSI codes from all logger calls in controller_manager_services.py
- Remove ANSI codes from logger calls in spawner.py and hardware_spawner.py
- Maintain backward compatibility with existing bcolors usage
- Fixes Issue ros-controls#2592: Option for uncolored log output

Environment variable behavior:
- RCUTILS_COLORIZED_OUTPUT=0: No colors in output
- RCUTILS_COLORIZED_OUTPUT=1: Colors enabled
- Unset: Auto-detect TTY for color support
Copy link
Contributor

@christophfroehlich christophfroehlich left a comment

Choose a reason for hiding this comment

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

The change to bcolors class look fine, but can't we still keep that in the rclpy logging? You removed it without an alternative.

- Add _color_enabled() function that respects RCUTILS_COLORIZED_OUTPUT
- Make bcolors class environment-aware using _color_enabled()
- Remove redundant COLOR_ENABLED variable for cleaner implementation
- Add bcolors import to spawner.py and hardware_spawner.py
- Fixes ANSI escape code pollution in CI logs while preserving colors in interactive terminals

Resolves: ros-controls#2592
@saumanraaj
Copy link
Author

Hey @christophfroehlich can you take a look at my changes and let me know if I have to change anything else, thanks.

@christophfroehlich christophfroehlich linked an issue Nov 1, 2025 that may be closed by this pull request
Copy link
Contributor

@christophfroehlich christophfroehlich left a comment

Choose a reason for hiding this comment

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

This looks great, I tested it successfully locally
image

just wait for the CI to check the logs

@codecov
Copy link

codecov bot commented Nov 1, 2025

Codecov Report

❌ Patch coverage is 80.00000% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.60%. Comparing base (f53552b) to head (3fb7c0b).
⚠️ Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
.../controller_manager/controller_manager_services.py 77.77% 2 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2741      +/-   ##
==========================================
- Coverage   89.60%   89.60%   -0.01%     
==========================================
  Files         152      152              
  Lines       17637    17645       +8     
  Branches     1448     1450       +2     
==========================================
+ Hits        15804    15811       +7     
  Misses       1250     1250              
- Partials      583      584       +1     
Flag Coverage Δ
unittests 89.60% <80.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...ler_manager/controller_manager/hardware_spawner.py 70.23% <100.00%> (ø)
controller_manager/controller_manager/spawner.py 71.51% <100.00%> (ø)
.../controller_manager/controller_manager_services.py 81.14% <77.77%> (-1.39%) ⬇️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Contributor

@christophfroehlich christophfroehlich left a comment

Choose a reason for hiding this comment

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

Locally tested, and the log files are much cleaner now.

Still, I'd like anyone else to test this if the colored output isn't broken where it should not be.

In the meantime, please add a simple note to the release_notes and to the docs, maybe in the helper scripts section.

- Document color output handling in userdoc.rst
- Add release note about bcolors respecting RCUTILS_COLORIZED_OUTPUT
@mergify
Copy link
Contributor

mergify bot commented Nov 2, 2025

This pull request is in conflict. Could you fix it @saumanraaj?

@saumanraaj
Copy link
Author

I added some helper notes and a note in the release notes. Let me know if I have to change anything, will do thanks

@christophfroehlich
Copy link
Contributor

Please don't do force pushes, as this is hard for reviewers to track what has changed since the last commit. I now have to test everything again..

saikishor
saikishor previously approved these changes Nov 3, 2025
Copy link
Member

@saikishor saikishor left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you

Copy link
Contributor

@christophfroehlich christophfroehlich left a comment

Choose a reason for hiding this comment

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

please fix pre-commit and rst issues.

@saumanraaj
Copy link
Author

I tried fixing most of the pre-commit and rst issues. It looks fine for me locally. If the CI logs fail again will fix them accordingly, thanks.

@saumanraaj
Copy link
Author

Hey @christophfroehlich, this PR only touches Python (controller_manager_services.py, spawner.py, hardware_spawner.py) and docs (RST). No C++/controller logic was changed.
The failing test is in C++ (controller_manager/test/test_spawner_unspawner.cpp:601, TestLoadController.unload_on_kill_activate_as_group) and appears unrelated to the Python color/log handling in this PR.
The other noisy log lines (e.g., “The 'type' param was not defined…”) are part of negative test cases and not introduced by this PR. I am not exactly sure why they are failing. Can you help me out? I will fix them ASAP.

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.

Option for uncolored log output

3 participants