search

windelbouwman / ppci

A compiler for ARM, X86, MSP430, xtensa and more implemented in pure Python
https://ppci.readthedocs.io/en/latest/
BSD 2-Clause "Simplified" License
337 stars 36 forks source link

ppci-build: No munging with terminal colors, please #19

Closed pfalcon closed 5 years ago

pfalcon commented 5 years ago

It's hard to believe, but the attached is the result of running ppci-build. Screenshot from 2019-09-03 22-57-22

Apparently, ppci-build does something with terminal colors, in the assumption the terminal has black background. Of course, there're no grounds for such assumptions. Generally, there's absolutely no reason why a build tool would munge with terminal colors. Though if it's really itches, using green and red foreground should be fine for both blacks and white backgrounds. (Of course, users of green background will loudly disagree.)

pfalcon commented 5 years ago

OMG, actually it applies to everything, e.g. python3 -m ppci cc. And I wondered why I'm getting an output like:


2019-09-03 23:13:07,888 |  WARNING |   ccodegen | Function does not return a value

2019-09-03 23:13:07,903 |  WARNING |       root | TODO: Linking with stdlibs
windelbouwman commented 5 years ago

This is a good issue. I added some coloring to the logging output. I will remove it again. Or should we use a color theme? I liked the coloring in the output, but it results in extra code, and this issue, so I would be happy to strip the escape code from the logging.

pfalcon commented 5 years ago

Well, I wouldn't make a suggestion to go and remove that functionality, given the effort invested into it. At least, not as a first choice.

The most conservative suggestion would be:

  1. Have it default off for maximum compatibility. But have an env var allowing interested parties to enable it persistently in their environment. I'm not sure how a command-line option would be helpful.

However, it's worth to look around and remember what's actual state of affairs with terminal color output we have nowadays. And it's that ls colored its output for quiet a long time. And wait, latest version of gcc also color error messages! And they likely followed clang in that regard. So, color apparently becomes a mundane thing in command-line output nowadays. But in all cases above the colors are apparently carefully chosen to work with both black and white terminal theme. There're also disabled if redirecting output to a file.

So, a more optimistic solution would be:

  1. Have a color on by default (while providing means to disable). But background color should never be manipulated, and only few "safe" foreground colors can be used. Red and green were mentioned above. Magenta, blue still good (blue might be too dark for black theme?). Cyan should be limited, already less legible on white theme. Yellow isn't really usable on white theme. And yeah, should be ready to perform isatty() check and disable coloring if redirecting.
pfalcon commented 5 years ago

Oh, and of course, explicit black/white foreground should never be used (I guess that's what really happens here). On the other hand, can use "bold" attribute (keeping in mind that it coincides with "bright" color IIRC, and e.g. gnome terminal has "allow bold text" setting, i.e. it can be disabled).

pfalcon commented 5 years ago

Ok, so https://github.com/windelbouwman/ppci-mirror/pull/25 pretty much fixes the issue. There may be further ideas of improvements, but in the name of closing bugs, let's consider this one resolved.