./mach build breaks clang diagnostic coloring

NEW
Unassigned

Status

Firefox Build System
Mach Core
6 years ago
5 months ago

People

(Reporter: Ehsan, Unassigned)

Tracking

Trunk
x86
Mac OS X

Firefox Tracking Flags

(Not tracked)

Details

(Reporter)

Description

6 years ago
I find those extremely helpful and miss them dearly.

Comment 1

6 years ago
I noticed this as well. However, strangely enough, color diagnostics are only not working when building js (and possibly base and nspr) but not platform or app. I have no clue why. This is most confounding to me. This is with "-fcolor-diagnostics" in my CFLAGS/CXXFLAGS. I have no clue why part of the build would behave differently from others.
(Reporter)

Comment 2

6 years ago
(In reply to comment #1)
> I noticed this as well. However, strangely enough, color diagnostics are only
> not working when building js (and possibly base and nspr) but not platform or
> app. I have no clue why. This is most confounding to me. This is with
> "-fcolor-diagnostics" in my CFLAGS/CXXFLAGS. I have no clue why part of the
> build would behave differently from others.

This has not been my experience, for me, color diagnostics don't work anywhere.  I'm on Mac, if that matters.

Comment 3

6 years ago
I can confirm that without -fcolor-diagnostics in CFLAGS/CXXFLAGS that clang does *not* emit color diagnostics when built with mach but does with |make -f client.mk| (irregardless of whether -s is in the make flags).

make likely ties the stdout of forked processes directly into the stdout of the calling process. Mach, by contrast, always has stdout going to a pipe (so mach can capture the output for logging, analyzing, etc). Unfortunately a side-effect of this is that isatty(stdout) on the child process (e.g. clang) will return false. And, isatty() is used as part of determining whether to emit terminal color sequences. So, with isatty() always returning false in mach, processes will never determine on their own that color sequences are safe to emit. So, you need to "trick" these processes into emitting terminal control characters. Hence -fcolor-diagnostics.

I suppose there might be some low-level hacks to trick the file descriptor into appearing to be a TTY. But, as long as tools have means to force them into emitting the same byte sequence as if they were connected to a TTY, my vote is to have these tools always emit color, etc and let mach sort out/strip the terminal control sequences depending on the run-time environment. This reminds me of another bug: mach needs to always strip terminal control sequences from the log output and conditionally strip these when !isatty(stdout) (and not overridden, of course).
(Reporter)

Comment 4

6 years ago
Hmm, ok, makes sense.  Can we be smarter about this though, and pass -fcolor-diagnostics in C(XX)FLAGS when mach is being used?

Comment 5

6 years ago
(In reply to Ehsan Akhgari [:ehsan] from comment #4)
> Hmm, ok, makes sense.  Can we be smarter about this though, and pass
> -fcolor-diagnostics in C(XX)FLAGS when mach is being used?

Eventually, yes. However, mach only has high-level integration with the build system currently. Once we replace client.mk we'll be in a much better position to inject extra things into the build system. Actually, once we no longer have client.mk, configure can blindly inject -fcolor-diagnostics because we no longer have to worry about accidentally producing terminal control characters (since mach will always be there and it will do the right thing).
(Reporter)

Comment 6

6 years ago
(In reply to comment #5)
> (In reply to Ehsan Akhgari [:ehsan] from comment #4)
> > Hmm, ok, makes sense.  Can we be smarter about this though, and pass
> > -fcolor-diagnostics in C(XX)FLAGS when mach is being used?
> 
> Eventually, yes. However, mach only has high-level integration with the build
> system currently. Once we replace client.mk we'll be in a much better position
> to inject extra things into the build system. Actually, once we no longer have
> client.mk, configure can blindly inject -fcolor-diagnostics because we no
> longer have to worry about accidentally producing terminal control characters
> (since mach will always be there and it will do the right thing).

Sounds sane to me.
Duplicate of this bug: 959226
I think we should just always add -fcolor-diagnostics to C{XX}FLAGS when building with clang.

Updated

5 months ago
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.