clang: Force colors on when building with ninja.

clang: Force colors on when building with ninja.

BUG=110262
TEST=Build with ninja & clang, still get color diagnostics.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=118223

[Nico, 2012-01-17 01:57:24]

It looks like gyp _does_ support checking for the generator in a conditional.

Since clang's colored diagnostics don't Just Work with ninja (
https://github.com/martine/ninja/issues/174 ), force them on when building with
ninja. Since no bots use ninja yet, this can be landed independently of ansii
escape support for buildbot (e.g. http://codereview.chromium.org/9179005/).

[M-A Ruel, 2012-01-17 02:00:49]

lgtm

Why do you always write "ansii"?

[Nico, 2012-01-17 02:03:38]

> Why do you always write "ansii"?

I guess I'm mixing it up with ascii :-(

[Mark Mentovai, 2012-01-17 02:08:55]

I don’t think we should do this. Can’t we just fix ninja?

[Nico, 2012-01-17 02:10:35]

On 2012/01/17 02:08:55, Mark Mentovai wrote:
> I don’t think we should do this. Can’t we just fix ninja?

We couldn't think of a way, see https://github.com/martine/ninja/issues/174
(also linked from the bug linked from here).

[Mark Mentovai, 2012-01-17 02:13:21]

But it looks like there’s no way to defeat this if you want to pipe
output into a file, or if you just think colors in a terminal look
terrible and you don’t want to see them. This should have a dedicated
knob, and it shouldn’t be on by default.

[Nico, 2012-01-17 02:20:24]

On 2012/01/17 02:13:21, Mark Mentovai wrote:
> But it looks like there’s no way to defeat this if you want to pipe
> output into a file, or if you just think colors in a terminal look
> terrible and you don’t want to see them. This should have a dedicated
> knob, and it shouldn’t be on by default.

Most people prefer colors, so I'd like to have them on by default with ninja
somehow.

I just noticed that this patch does break vim's errorformat parsing, so I agree
it is the wrong approach.

I hate computers :-(

[Evan Martin, 2012-01-17 16:34:33]

On 2012/01/17 02:20:24, Nico wrote:
> I just noticed that this patch does break vim's errorformat parsing, so I
agree
> it is the wrong approach.
> 
> I hate computers :-(

Hm, yeah.  I build through emacs and colors there will also break things.

Thinking aloud:  we want the no-special-flags behavior to be colors on, but we
need a way for someone to toggle colors off when necessary.  The toggle
shouldn't be at gyp time (otherwise you're forced to re-run gyp every time you
want to build through emacs).  So environment variable or ninja flag that
somehow toggles things?

But now that I've written that, if you add/remove flags to the compiler at
runtime (like -fcolor-diagnostics) ninja will rebuild every source file because
the command line changed.

Another option: force colors always on, add an extra post-processor that removes
colors that can be used via a pipe.

(PS: in theory I should hack emacs to understand the colors output by clang so
that I get the colors in my editor as well.)

[Nico, 2012-01-17 16:38:47]

> Another option: force colors always on, add an extra post-processor that
removes
> colors that can be used via a pipe.

Ninja could contain code that does "if (!isatty())
stipEscapeSequencesFromSubcommandOutput()"?

[Evan Martin, 2012-01-17 16:52:19]

On 2012/01/17 16:38:47, Nico wrote:
> > Another option: force colors always on, add an extra post-processor that
> removes
> > colors that can be used via a pipe.
> 
> Ninja could contain code that does "if (!isatty())
> stipEscapeSequencesFromSubcommandOutput()"?

Hm, I like it!

[Mark Mentovai, 2012-01-17 17:10:47]

I express that I don't want colors through my $TERM. Does that factor in?
Could it?
On Jan 17, 2012 11:52 AM, <evan@chromium.org> wrote:

> On 2012/01/17 16:38:47, Nico wrote:
>
>> > Another option: force colors always on, add an extra post-processor that
>> removes
>> > colors that can be used via a pipe.
>>
>
>  Ninja could contain code that does "if (!isatty())
>> stipEscapeSequencesFromSubcomm**andOutput()"?
>>
>
> Hm, I like it!
>
>
http://codereview.chromium.**org/9241001/<http://codereview.chromium.org/9241...
>

[Evan Martin, 2012-01-17 17:25:17]

On 2012/01/17 17:10:47, Mark Mentovai wrote:
> I express that I don't want colors through my $TERM. Does that factor in?
> Could it?

I'm reminded of Rob Pike's amusing lament about colors in googletest, though I
can't find a link to it at the moment.

[Nico, 2012-01-17 18:12:53]

On Tue, Jan 17, 2012 at 9:10 AM, Mark Mentovai <mark@chromium.org> wrote:
> I express that I don't want colors through my $TERM. Does that factor in?
> Could it?

llvm's StandardErrHasColors() check roughly looks like this:

  if (!isatty(stderr)) return false;
  if (const char *term = std::getenv("TERM")) if (strcmp(term, "dumb")
== 0) return false;
  return true;

The logic in ninja would look the same, so if you do TERM=dumb you
wouldn't get colors.

If we add this to ninja, does this CL here work for you?

[Evan Martin, 2012-01-17 18:16:36]

On Tue, Jan 17, 2012 at 10:12 AM, Nico Weber <thakis@chromium.org> wrote:
> On Tue, Jan 17, 2012 at 9:10 AM, Mark Mentovai <mark@chromium.org> wrote:
>> I express that I don't want colors through my $TERM. Does that factor in?
>> Could it?
>
> llvm's StandardErrHasColors() check roughly looks like this:
>
>  if (!isatty(stderr)) return false;
>  if (const char *term = std::getenv("TERM")) if (strcmp(term, "dumb")
> == 0) return false;
>  return true;
>
> The logic in ninja would look the same, so if you do TERM=dumb you
> wouldn't get colors.

Note that ninja itself examines TERM to decide whether to do its fancy
overprinting output.

[Mark Mentovai, 2012-01-17 18:57:13]

Nico Weber wrote:
> The logic in ninja would look the same, so if you do TERM=dumb you
> wouldn't get colors.
>
> If we add this to ninja, does this CL here work for you?

Not really. I have a real TERM, it’s just a TERM whose capabilities
don’t include color.

[Nico, 2012-01-17 19:01:22]

On Tue, Jan 17, 2012 at 10:57 AM, Mark Mentovai <mark@chromium.org> wrote:
> Nico Weber wrote:
>> The logic in ninja would look the same, so if you do TERM=dumb you
>> wouldn't get colors.
>>
>> If we add this to ninja, does this CL here work for you?
>
> Not really. I have a real TERM, it’s just a TERM whose capabilities
> don’t include color.

If you do `echo foo | clang -x c -c -`, do you get colored output with
your term setup today?

[Mark Mentovai, 2012-01-17 19:21:58]

Looks like I’ve wrapped it in something that sets TERM=dumb.

Yuck. I shouldn’t have to do that (and I’d hope I wouldn’t have to
replicate it for more tools). :(

Nico Weber wrote:
> On Tue, Jan 17, 2012 at 10:57 AM, Mark Mentovai <mark@chromium.org> wrote:
>> Nico Weber wrote:
>>> The logic in ninja would look the same, so if you do TERM=dumb you
>>> wouldn't get colors.
>>>
>>> If we add this to ninja, does this CL here work for you?
>>
>> Not really. I have a real TERM, it’s just a TERM whose capabilities
>> don’t include color.
>
> If you do `echo foo | clang -x c -c -`, do you get colored output with
> your term setup today?

[Nico, 2012-01-17 19:26:19]

On Tue, Jan 17, 2012 at 11:21 AM, Mark Mentovai <mark@chromium.org> wrote:
> Looks like I’ve wrapped it in something that sets TERM=dumb.
>
> Yuck. I shouldn’t have to do that (and I’d hope I wouldn’t have to
> replicate it for more tools). :(

Ok, that sounds fringe-casy enough that making this
on-by-default-off-by-include.gypi-override-for-mark then :-)

So force colors on, and let ninja strip them again it is.

(If you have an easy tweak to the TERM check above, I'm happy to do
that in ninja and to try to get that into clang, too.)

[Mark Mentovai, 2012-01-17 19:40:56]

If you have easy access to the termcap/terminfo database from ninja,
we can probably make it do the right thing (like most other tools do)
and only do colors when the capabilities include it. Otherwise, I’m
with Mike Burrows, but whatever, as long as the bots and I can disable
it.

Nico Weber wrote:
> On Tue, Jan 17, 2012 at 11:21 AM, Mark Mentovai <mark@chromium.org> wrote:
>> Looks like I’ve wrapped it in something that sets TERM=dumb.
>>
>> Yuck. I shouldn’t have to do that (and I’d hope I wouldn’t have to
>> replicate it for more tools). :(
>
> Ok, that sounds fringe-casy enough that making this
> on-by-default-off-by-include.gypi-override-for-mark then :-)
>
> So force colors on, and let ninja strip them again it is.
>
> (If you have an easy tweak to the TERM check above, I'm happy to do
> that in ninja and to try to get that into clang, too.)

[commit-bot: I haz the power, 2012-01-19 01:21:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/thakis@chromium.org/9241001/2002

[Nico, 2013-05-30 00:29:04]

.