Always pass -fdiagnostics-color=always to cc-style compilers to avoid useless recompilations when Dune's stderr is redirected

[nojb]

Fixes #10882

[emillon]

cc @MisterDA do you remember why there's a guard here?

[Leonidas-from-XIV]

@nojb Does your PR filter out the color sequences when writing to _build/log? I could see a point in always forcing color and filtering it out depending on where we write to (log, stdout) but we'd need to make sure it doesn't write color to logfiles.

[nojb]

@nojb Does your PR filter out the color sequences when writing to _build/log? I could see a point in always forcing color and filtering it out depending on where we write to (log, stdout) but we'd need to make sure it doesn't write color to logfiles.

In general Dune knows how to strip ANSI color sequences from the output of commands if Dune's output is itself not a terminal. This works if one redirects the output of Dune, eg dune 2>.... However, there seems to be something not quite right with _build/log:

$ (cd _build/default && /home/nojebar/mlfi/mlfi-ins/bin/ocamlopt.opt -w @[email protected]@30..39@[email protected]@[email protected] -strict-sequence -strict-formats -short-paths -keep-locs -w -49 -nopervasives -nostdlib -g -I .lib.objs/byte -I .lib.objs/native -intf-suffix .ml-gen \
-no-alias-deps -opaque -o .lib.objs/native/lib.cmx -c -impl lib.ml-gen)
$ (cd _build/default && /usr/bin/gcc -O2 -fno-strict-aliasing -fwrapv -pthread -fPIC -D_FILE_OFFSET_BITS=64 -fdiagnostics-color=always -g -I /home/nojebar/mlfi/mlfi-ins/lib -o stubs.o -c stubs.c)
>
>     1 | #
>       |

but this is independent of this PR, the behaviour is the same on main.

[MisterDA]

Why does Dune decide to rebuild the stubs? Is it because the set of flags changed that it invalidates the artifacts? If so, I think Dune should rather ignore this flag.

I think this is the wrong fix; the original idea was to pass the -fdiagnostics-color=always flag to the C compiler if and only if Dune decides it should itself use colored output.
If Dune strips the ANSI color escape codes from the C compiler when Dune chooses not to output colors, then we can pass -fdiagnostics-color=always unconditionally to the C compiler and I'm fine with this patch.

As a reminder, the end goal was to implement Standard for ANSI Colors in Terminals (quite informal, inspired by the BSD world). Tools should detect whether to output colors based on a CLI flags, an handful of environment variables, or if the output is a terminal (using isatty). As the output of the tools Dune invokes is buffered through pipes, they don't output colored text by default. If they were to use a single, standardized environment variable, such as CLICOLOR_FORCE, Dune could simply set it for all its subprocesses. Unfortunately that's not the case, so we have to resort to passing flags.

[nojb]

If Dune strips the ANSI color escape codes from the C compiler when Dune chooses not to output colors, then we can pass -fdiagnostics-color=always unconditionally to the C compiler and I'm fine with this patch.

This is the behaviour Dune has always had, as far as I know, and also used with the OCaml compiler. @emillon: do you confirm?

[nojb]

If Dune strips the ANSI color escape codes from the C compiler when Dune chooses not to output colors, then we can pass -fdiagnostics-color=always unconditionally to the C compiler and I'm fine with this patch.

This is the behaviour Dune has always had, as far as I know, and also used with the OCaml compiler.

Also cc @rgrinberg which will know about this.

[rgrinberg]

Always choosing to use colors and stripping the colors ourselves is the consistent thing to do.

[nojb]

Always choosing to use colors and stripping the colors ourselves is the consistent thing to do.

Thanks for the quick reply! Do you confirm that this is already how Dune works and there isn't anything more to do in this PR to handle colors coming from the C compiler correctly?

[rgrinberg]

I confirm. Some testing would of course be appreciated though.

FYI, this might break --no-buffer in some ways, but I'm not particularly concerned as that mode is rather dangerous anyway.

[nojb]

I confirm. Some testing would of course be appreciated though.

Thanks. I tested locally as well as adapting some existing tests that check that the flag is passed correctly and that the color codes are filtered out when dune's stderr is not a terminal.

Planning to merge once CI passes.