777952 - some clang warnings are being printed without colors

Status: NEW

(Firefox Build System :: General, defect)

Description

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

During a build some warnings are showing colors and some are not. I guess our use of ccache has something to do with it, but I haven't debugged it yet.

Comment 1

[Gregory Szorc [:gps]]

AFAIK we don't support -fcolor-diagnostics out of the box. We instead rely on people providing it via mozconfig.

The problem is that not all arguments are being passed to or honored by configure and/or make provided by sub-projects. e.g. nspr and nss doesn't inherit the mozconfig-provided cflags, etc from the master configure or autoconf output.

Comment 2

[(no longer active)]

But clang's warnings are colorized by default if they go to stdout...

Comment 3

[Gregory Szorc [:gps]]

(In reply to Ehsan Akhgari [:ehsan] from comment #2)
> But clang's warnings are colorized by default if they go to stdout...

More accurately, they are colorized by default if a terminal is detected on stderr. This is using heuristics as implemented by LLVM (see lib/Support/Unix/Process.inc). On Unix, this pretty much boils down to isatty().

When using ccache, isatty() will always return false. So, you'll need to forcefully pass -fcolor-diagnostics when using ccache.

Even without using ccache, there will sometimes be another process between clang and the terminal causing isatty() to return false. e.g. our |make -s| buffers output with print-failed-commands.sh and only emits if the process exited with a non-0 exit code. That will cause isatty() to return false and thus require -fcolor-diagnostics.

I don't think we can blindly turn on -fcolor-diagnostics because automated agents slurping the build output (like TBPL) don't want to have to deal with eliminating terminal character sequences. If everything handles this fine and/or you want to shift that burden to everyone else, go right ahead!

As an aside, mach/mozbuild solves this problem. Its logging infrastructure is intelligent about terminal escape sequences. If the thing that launches mach is a TTY, you get the escape sequences written to stderr. If it isn't, those sequences are stripped. (This can also be controlled by a config value.) Loggers not writing to stderr always strip escape sequences. So, you can blindly enable color everywhere and it just works. This is one of many benefits to having an intelligent Python shim fronting the build system. I just wish it would land so things like this bug aren't held back.

Comment 4

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

(In reply to Ehsan Akhgari [:ehsan] from comment #2)
> But clang's warnings are colorized by default if they go to stdout...

Exactly, that is why I think the problem with with ccache redirecting the output somewhere.

Comment 5

[(not currently active) Ted Mielczarek]

Could we effectively just do this check (isatty(stderr)) up-front in configure, and stick -fcolor-diagnostics in C{XX}FLAGS if so?

Comment 6

[Gregory Szorc [:gps]]

(In reply to Ted Mielczarek [:ted] from comment #5)
> Could we effectively just do this check (isatty(stderr)) up-front in
> configure, and stick -fcolor-diagnostics in C{XX}FLAGS if so?

Possibly. That assumes that there is consistency between the agents that invoke configure and whatever invokes make. This will hold on the buildbot machines since everything there should fail isatty().

End-users machines might be different. e.g. mozbuild invoking configure would fail isatty() because it is a pipe. But, invoking configure directly from the terminal would obviously work. We can work around this by providing an argument or environment variable to configure to explicitly enable color and agents invoking configure can do the right thing to ensure the desired result.