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

Do not output colors e.g. into a pipe, unless forced. #1077

Merged
merged 2 commits into from May 8, 2018

Conversation

Projects
None yet
5 participants
@lanurmi
Copy link
Contributor

lanurmi commented May 2, 2018

Use the de-factoish environment variables CLICOLOR(_FORCE) to override default color behavior.

Do not output colors e.g. into a pipe, unless forced.
Use the de-factoish environment variables CLICOLOR(_FORCE) to override default color behavior.

@lanurmi lanurmi force-pushed the lanurmi:no-colors-on-non-tty branch from 9921fc6 to ba34e3e May 2, 2018

@ratzlaff

This comment has been minimized.

Copy link
Contributor

ratzlaff commented May 2, 2018

No windows option?

@tvandijck

This comment has been minimized.

Copy link
Member

tvandijck commented May 2, 2018

windows already does not put color information into the I/O stream, it has this special API to put the console in a certain color mode. It just works very differently.. So I don't think this really needs a windows option..

@ratzlaff

This comment has been minimized.

Copy link
Contributor

ratzlaff commented May 2, 2018

Yea, I understand that part (about putting color information in the stream). I was just thinking more about keeping things consistent between platforms.

If I have CLICOLOR=0 in my linux environment, I would like to see the same thing happen (interactively) when I set the environment variable in windows.

The functions for retrieving environment variables and testing for tty are available on windows too.

Also support the CLICOLOR variable on Windows.
Colors probably cannot be forced on though, because of different implementation on Windows.
@lanurmi

This comment has been minimized.

Copy link
Contributor

lanurmi commented May 4, 2018

I added support for Windows too, as much as it can be supported due to the different way it works. I can squash the two commits into one, if desired.

@TurkeyMan
Copy link
Member

TurkeyMan left a comment

Should this function result be cached?

static int shouldUseColors() {
// CLICOLOR* documented at: http://bixense.com/clicolors/
return ((getenvOrFallback("CLICOLOR", "1")[0] != '0') && canUseColors())
|| getenvOrFallback("CLICOLOR_FORCE", "0")[0] != '0';

This comment has been minimized.

@TurkeyMan

TurkeyMan May 5, 2018

Member

I feel like, given the code below, that this function could be called a LOT...
Like, it appears to do this work every time you wanna change colour, is that right?
Can't this just be executed once at startup?

This comment has been minimized.

@tvandijck

tvandijck May 7, 2018

Member

I doubt it's of big impact... we don't set colors that often... only for errors, which are fatal generally, so it's a one time event...

I do agree that we could at startup however.. if @lanurmi has the time to do that, that would be great? but if not, I don't want to hold off for too long and let this PR sit around.

This comment has been minimized.

@TurkeyMan

TurkeyMan May 7, 2018

Member

Yeah, give him a few days, and if not... just merge ;)

This comment has been minimized.

@lanurmi

lanurmi May 8, 2018

Contributor

As long as premake doesn't color each character in regular output with a different color, is there a scenario where term_doSetTextColor() would be called a lot in the first place? What does one need to do wrong to get hundreds or thousands of colored lines (i.e. warnings or errors) currently?

I can make it cache the result, sure, but it adds some complexity, and we don't even know if shouldUseColors() is a hotspot in profiling.

This comment has been minimized.

@samsinsane

samsinsane May 8, 2018

Member

is there a scenario where term_doSetTextColor() would be called a lot in the first place?

The unit tests now do this, sorry! It will be called 8200+ times when running the unit tests now, so it might be a good idea to cache the result. Additionally, Premake core might not call it a lot in regular usage, but that doesn't mean that modules can't call it a lot for any number of reasons.

This comment has been minimized.

@tvandijck

tvandijck May 8, 2018

Member

Lets merge it, and I'll take care of the caching...

@tvandijck tvandijck merged commit f8c7edf into premake:master May 8, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment