Color handling: let's pass -colors instead of setting an env var

[emillon]

Hi,

dune is able to interpret and strip ANSI escape codes. Thanks to that we don't have to let tools determine what they're connected to (and because of buffering, it might return the wrong answers).

So we're setting OCAML_COLORS=always, but this has a drawback: it's inherited everywhere. If something that we call down the way calls ocamlc, it will have colors, but its output might be somewhere that does not expect color codes.

It seems to be easier to reason about flags if we only pass -color always to the executables dune call directly.

Thoughts?

[MisterDA]

Dune is able to interpret and strip ANSI escape codes. Thanks to that we don't have to let tools determine what they're connected to (and because of buffering, it might return the wrong answers).

On the other hand, if Dune (<= 3.5 at the time of writing) detects that it's not running in a tty, it will strip ANSI sequences. This is a problem for my use case (running Dune in CI systems), see #6323.

So we're setting OCAML_COLORS=always, but this has a drawback: it's inherited everywhere. If something that we call down the way calls ocamlc, it will have colors, but its output might be somewhere that does not expect color codes.

True, but that should be uncommon.

It seems to be easier to reason about flags if we only pass -color always to the executables dune call directly.

True.

OCAML_COLOR was introduced in 4.05. OCaml's -color flag was introduced in 4.03. Generally I'd say it's easier to use environment variables: if an older version of the tool doesn't support the color flag it's a no-op. For OCaml it seems cleaner to use the flag for the reasons above.

Using color flags for C compilers is what I intended with #4083. GCC and clangs might support CLICOLOR and CLICOLOR_FORCE some day, see https://bixense.com/clicolors/, but for now it's clear we have to use flags to enable colors in C compilers.

In general I think the precedence for controlling Dune colors should be (I hope I haven't make any mistakes, it's easy to get confused):

let supports_color fd dune_color_arg =
  let is_smart =
    match Stdlib.Sys.getenv "TERM" with
    | exception Not_found -> false
    | "dumb" -> false
    | _ -> true
  and clicolor =
    match Stdlib.Sys.getenv "CLICOLOR" with
    | exception Not_found -> true
    | "0" -> false
    | _ -> true
  and clicolor_force =
    match Stdlib.Sys.getenv "CLICOLOR_FORCE" with
    | exception Not_found -> false
    | "0" -> false
    | _ -> true
  and dune_color =
    match Stdlib.Sys.getenv "DUNE_COLOR" with
    | exception Not_found -> None
    | "never" -> Some `None
    | "always" -> Some `Ansi
    | "auto" -> None
    | s -> Printf.kprintf invalid_arg "DUNE_COLOR=%s" s
  in
  let dune_color = match dune_color_arg with Some _ -> dune_color_arg | _ -> dune_color in
  match dune_color with
  | None -> (is_smart && Unix.isatty fd && clicolor) || clicolor_force
  | Some `None -> false
  | Some `Ansi_tty -> true

The DUNE_COLOR and --color flag are compatible with Fmt_cli.
This is on the basis that we agree that if colors are enabled for Dune, Dune should forward this choice to programs it uses (OCaml compiler, C/C++ compilers).

ref #145.

[rgrinberg]

We discussed this with the team and the solution that was deemed most attractive was to make all compiler output contain colors (with environment variables or flags) and then handle the user preference of displaying colors with dune itself. Dune already needs to understand the output of these tools, so it would be natural to do so.