797216 - ./mach build breaks clang diagnostic coloring

Status: NEW

(Firefox Build System :: Mach Core, enhancement)

Description

[(no longer active)]

I find those extremely helpful and miss them dearly.

Comment 1

[Gregory Szorc [:gps]]

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.

Comment 2

[(no longer active)]

(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

[Gregory Szorc [:gps]]

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).

Comment 4

[(no longer active)]

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

[Gregory Szorc [:gps]]

(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).

Comment 6

[(no longer active)]

(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.

Comment 8

[(not currently active) Ted Mielczarek]

I think we should just always add -fcolor-diagnostics to C{XX}FLAGS when building with clang.