Skip to content
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

Make output of colcon colorful using colorama #487

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

timonegk
Copy link

This pull request replaces #195. In general, it provides the same coloring, heavily inspired by catkin_tools. To do the formatting, it uses colorama, a library that also supports Windows.

@v-lopez
Copy link

v-lopez commented Feb 28, 2022

Thanks for this, I tested it and it is beautiful.

It may seem like a minor feature, but the first time I jumped from catkin tools to colcon, the lack of colors made the new tool even more daunting, as you haven't learnt yet which kind of messages are important and which can be ignored, so you have to read everything until you familiarize yourself with it. Colors bring a certain degree of familiarity, as well as highlighting important messages.

I have installed this, and hoping it's merged and I can share it with my team easily as well.

@mikepurvis
Copy link
Contributor

I definitely vote in favour of some strategy for coloring colcon, but I don't think the present PR addresses all of the concerns that have been raised in the past (eg #195):

  • Will this gracefully degrade in the presence of a non-TERM, for example many CI setups, or running colcon piped directly to a file?
  • How much is it going to complicate a future i18n effort?
  • How badly is code readability going to be affected?
  • What is the strategy for coordinating a rollout of this across colcon's sizable ecosystem of plugins?
  • Is it okay to punt on "passing through" color codes from underlying tools (like make), or do we need a plan in place for that upfront? (eg, showing colors in the terminal view but stripping them from the saved logs)

@Timple
Copy link

Timple commented Mar 3, 2022

the first time I jumped from catkin tools to colcon, the lack of colors made the new tool even more daunting

However minor such a feature sounds. I think the 'ROS2 migration fear' is a summation of such encounters.
I think these kind of endeavors are really worth it!

@v-lopez
Copy link

v-lopez commented Mar 3, 2022

Will this gracefully degrade in the presence of a non-TERM, for example many CI setups, or running colcon piped directly to a file?

It looks like colorama is disabled when output is redirected. https://github.com/tartley/colorama#init-keyword-args

How much is it going to complicate a future i18n effort?

I believe not much, since English sentences or words are not split by these tags or have different colors.

What is the strategy for coordinating a rollout of this across colcon's sizable ecosystem of plugins?

Like I said, I am new to colcon, and I'm not the author of this PR, but if you could enumerate them, I think these changes are simple enough so that all of us who are interested in this can push it easily.

Is it okay to punt on "passing through" color codes from underlying tools (like make), or do we need a plan in place for that upfront? (eg, showing colors in the terminal view but stripping them from the saved logs)

I believe most of these tools already have systems in place to disable color output when redirecting. I know that I have to explicitly enable some of them to have colors in Jenkins output for instance.

@Flova
Copy link

Flova commented Mar 9, 2022

I am also highly in favor of this pull request! The missing color bothered me for quite some time now.

@jaagut
Copy link

jaagut commented Jul 21, 2022

Hi! Thank you all for your great contributions!
I am just curious to know how to progress with this PR.
Are the open questions resolved? What should be discussed or changed? What stops this from being merged (except for the merge conflict)?

@Flova
Copy link

Flova commented Oct 17, 2022

Any updates on this PR?

@Flova
Copy link

Flova commented Feb 21, 2023

I think @v-lopez addressed the concerns quite well. Are there any other concerns that hold back colorization of colcon? @cottsay

@jonselling
Copy link

Hi! I would really like to have colored output for colcon!

Are there any other concerns that need to be addressed?

@MatthijsBurgh
Copy link

@timonegk @Timple @mikepurvis Lets put some effort to these color PRs.

Do we think the coloring should be in colcon-core or colcon-output?

@mikepurvis
Copy link
Contributor

mikepurvis commented Aug 8, 2023

I don't have enough of an architectural vision for colcon at this point— I think it has to be @cottsay making that call. Instinctively, it does feel like it needs to be in core, though, as colouring information is something that you can't really bolt onto the content after the fact; it has to be injected at the origin.

@tokoro10g
Copy link

Any updates on this?

I agree with everyone here pointing out how the colorless output bothers developers.
I believe this PR will improve the DX of colcon pretty much and will encourage more people to use ROS2!

@MatthijsBurgh
Copy link

@cottsay friendly ping ;)

@Timple
Copy link

Timple commented Oct 10, 2023

Seems like this PR waiting long enough to have conflicts already 😞

@Flova
Copy link

Flova commented Oct 23, 2023

Any updates on this?

@Flova
Copy link

Flova commented Oct 31, 2023

Fyi you can already easily use colcon with colors by executing the following commands. Keep in mind that no updates are applied that way so you need to execute them again if some interface changed (the fork also needs to be updated first).

pip install git+https://github.com/timonegk/colcon-core.git@colors --user
pip install git+https://github.com/timonegk/colcon-notification.git@colors --user
pip install git+https://github.com/timonegk/colcon-output.git@colors --user

That being said it would definitely be nicer to have this available by default.

@MatthijsBurgh
Copy link

Friendly ping ;)

@SammyRamone
Copy link

It has been more then 4 years now (including the original PR) and there is continous interest by multiple persons. Is there any way to get this PR finally merged? @cottsay

This was referenced Jan 23, 2024
@Flova
Copy link

Flova commented Feb 1, 2024

Ping @cottsay just for a statement regarding this matter.

If you don't like it or don't really care about the feature itself it's fine, just say it. It is understandable if there is limited time and resources, but not addressing the matter for years despite continuing interest from the community is not the best look for a project. I fully understand that people bothering the maintainer with feature requests can be exhausting. But if people put in the work and do a contribution of reasonable quality (which is the case here considering this comes from one of catkin_tools top contributiors / maintainer) only to be ignored for extended periods of time is quite disrespectful. I normally want to keep things chill, but this bothers me sorry for that.

@cottsay
Copy link
Member

cottsay commented Feb 2, 2024

Please allow me to start by apologizing for neglecting this issue. There's really no excuse for not providing any input on these open PRs at all. I could tell you how thin resources are for maintaining colcon right now, and how there are significantly more impactful features and bugs that need attention, but I should have taken the time to communicate and iterate earlier.


Some of the reasons I've not felt that this is a priority feature are that there is very little console output generated by colcon itself, where much of the output comes from (already colorized) cmake output. Colorization also isn't universally usable by all users and in all contexts, and in some cases can even regress the experience, especially when colorization sequences appear in raw log files.

These are not excuses, but in the spirit of better communication, an explanation of some of my reluctance to take either of the open PRs as-is.


Here are some of my thoughts for moving this forward.

Functionality provided by colcon-core itself should be the bare minimum set of features for colcon to bootstrap itself. Even such trivial features as recursive directory traversal and package selection are implemented in separate extensions. So too should colorization of output, especially if it means picking up a new dependency. Fewer features means fewer bugs and simpler maintenance where it matters. However, this is complicated by the fact that there is at least some console output produced in colcon core itself, and you'll probably want to see colors there too.

Building from your desire for colorized output, I think we can build an efficient system to handle arbitrary output styling at little to no additional cost. An example of such augmentation would be section start/end markers such as those used by Travis CI, GitHub Actions, and Jenkins. Looking further out, I have created extensions for sending E-mail notifications for build failures that could take advantage of HTML for package URL hyperlinks, headers, and more.

Implementing this as an extension would also give users the opportunity to create "themes" and customize the output style to suit their preferences and environments without the need to modify existing packages.

I've created a branch with a proof-of-concept extension point for doing this: cottsay/output-style

Please take a look and tell me if you think this would suit your needs. I expect that we'd probably create a new extension, something like colcon-style-colorama that would map the high-level stylizers to specific ANSI codes in colorama, and we could consider adding that extension to the recommended family.

@mikepurvis
Copy link
Contributor

I definitely see the argument for adding hooks to do this and farming it out to a separate extension. However, I do wonder about whether the indirection associated with this is worth it. Colorama is well established and has already done the hard work of abstracting color codes across all of Python's supported platforms. At a certain point we risk making things more difficult and less conventional by effectively having colcon-core roll its own in-house abstraction layer rather than leveraging what already exists.

At risk of attacking a straw man, I see the argument against native coloring as being not that far removed from arguing that colcon should abstract the terminal itself, or maybe even the concept of textual output. I know that's not really fair because those are things provided by the Python stdlib, but I think it's reasonable for Colorama's API to be natively part of the "platform" that colcon-core exposes to its ecosystem of plugins.

If nothing else, it's worth noting that this working PR adds 18 lines to colcon-core whereas the incomplete abstraction scheme being proposed adds ten times that, and would likely end up being much more once the plugin is also accounted for— that is an awfully high cost to pay for avoiding the coupling to a lightweight project that is well maintained and itself dependency-free.

@tfoote
Copy link
Member

tfoote commented Feb 2, 2024

Colorama in itself is moderately light weight and on it's own wouldn't be a blocker. And it's fair to consider whether the level of indirection is worth doing. However we can't turn around and then say that this implementation is very small compared to the cleaner abstraction and thus would be less maintenance when the current implementation is missing a lot of the functionality that is sketched into the abstraction. This change should be at least disableable to turn off colorized outputs. It's currently just hard coded. Furthermore, there's additionally no way to customize the colors. For example these hard coded colors are not accessible for red green color blind people. And if someone wants to do something more visually distinct or apply any other styling there's no room for extension. Without the abstraction extension we're now looking at adding this potential whole constellation of options to the core functionality. I know that some people strongly want this. If you'd like this please work to improve and flesh it out to provide the full functionality that @cottsay layed out which will unlock not just the single short term minimal use case but meet the larger vision for the tool, and not require deprecation cycles to get there.

@mikepurvis
Copy link
Contributor

Hmm, I can't think of any other tool of this class that provides color output and also has a mechanism for customizing the coloring— is there an example we can look at? Across cmake, git, nix, brew, bazel, and gradle, I think all of them do various color-y things without any ability to modify the scheme short of forking the source.

I suspect that for most, the pragmatic direction here would be:

  • A pair of --color/--no-color flags, for which the default is on in the presence of a tty, and off otherwise. The "off" case is implemented by simply filtering the output right before printing it, likely doable through a log formatter.
  • All plugins that submit output do so with the understanding that the color codes may be filtered.
  • Color will never be used in a way that isn't redundant with the text itself, so it won't ever be required to be able to distinguish (for example, red-ERROR/green-OK over red/green-PACKAGENAME). It's understood that users with special needs around specific color pairs will have the option to remap them from their terminal software if desired.

I do agree that it would be nice to have css-style separation between semantics and presentation, but in the absence of an established ecosystem implementing that for Python terminal output, I just don't know if it's worth building it from scratch inside colcon. Possibly a halfway measure could be simply defining a Styles enum in colcon-core with constants for common cases like error, ok, info, emphasis, summary, pointing directly to the colorama constants. Plugin authors can be encouraged to use those constants rather than hard-coding, and then there's at least single place to update the scheme down the road or make it more configurable/pluggable if that's later felt to be a requirement.

@Flova
Copy link

Flova commented Feb 5, 2024

I fully agree with it being a bit over the top to include advanced theming. Especially if the development is constrained and complexity should be low. Tbh. except things like vim that can be configured to be a whole IDE in the terminal, no CLI tools I know of provide that flexibility.

I think it is important to be able to fall back to a non-colored mode easily. As discussed earlier in this thread colorama strips the coloring if the output is redirected (e.g. scripting or CI). An additional --no-color / --color flag might not hurt tho. and can be easily integrated in case you want colors in the CI or don't want them for aesthetic reasons.

I also agree with the other two points, especially that colors should be redundant.

Building custom stuff for standard things is also not ideal imo.. And especially if we have the choice between no feature, because we only implement a version that is harder to maintain and has additional things that are rarely used or a feature that is good enough for most users, I opt for the second one. Especially since anybody is still free to propose an extended version, that is integrated with proper deprecation or backwards compatibility. If this PR otherwise stays open for another 4 years, we can easily integrate a good enough (like most build tools use) solution now and deprecate it when the new one is ready. Even if that process takes a few years it's still faster, more user focused and not that much overhead. Also, I think the benefit of having proper formatting during normal use earlier is greater than pretty email formatting.

Additionally, each custom abstraction layer makes the code harder to understand for new contributors etc.. Googling issues gets harder and the list goes on. This is not the first time I have noticed a tendency towards adding maybe a bit too many abstraction layers in ROS 2 which look good on paper, but in the end everybody uses the default one and it makes everything more painful when you change something or have a specific issue. Don't get me wrong, ROS 2 is awesome, but that is some criticism I have. Sometimes KISS is the way to go.

Regarding the theming, this is in almost all cases done via the terminal emulator settings, as the color codes don't specify exact colors and they are rendered according to the emulators theme. Regarding accessibility, I would assume that color-blind people use a color-blind theme (also for warnings / errors in ROS terminal output).

tl;dr:

  • We should keep things simple as most tools do
  • Coloring should be easily disabled
  • Custom stuff has its drawbacks and over engineering is a thing
  • Too many abstractions can be harmful
  • Theming is done by the terminal emulator

This being said, while I have some criticism, I thank @cottsay for his statement!

@pulak-gautam
Copy link

Could anyone give an update on this? Has any extensible plugin been implemented which provides this functionality yet?

@Timple
Copy link

Timple commented Jul 2, 2024

I've been using this branch for months. Works like a charm.

However now it got out of date again. What is the way forward now? This PR has been open for years, let's get this in!

@timonegk
Copy link
Author

timonegk commented Jul 2, 2024

@Timple I rebased the branch on master, so at least for now it is up to date again.

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

Successfully merging this pull request may close these issues.