Let env var CLICOLOR_FORCE set -fdiagnostics-color=always.

Message ID 20180612224659.spp22g33ym4pib5r@angband.pl
State New
Headers show
Series
  • Let env var CLICOLOR_FORCE set -fdiagnostics-color=always.
Related show

Commit Message

Adam Borowski June 12, 2018, 10:46 p.m.
Hi!
Here's a patch for Bugzilla 80271, to add support for env CLICOLOR_FORCE.

Automated build environments -- and even some which you run by hand --
redirect the output somewhere to save a log, even if they tee it to the
terminal anyway.  This kills all colored highlights for diagnostics.
Those highlights are really nice for a human -- unlike the old days when
a single-threaded build always had the failure at the end, modern setups
tend to spew pages of stuff after the real reason your build failed.  As
human vision evolved to be extremely good at noticing a fleck of color,
it's easy to spot that bit of red even when scrolling very fast.

Besides terminals, you can read colored logs via "less -R", ansi2html, etc.

For such piped setups, gcc has -fdiagnostics-color=always.  Alas, while this
flag is easy to set by someone writing a given piece of software, there's
no reasonable way to set it globally in the build environment.

Thus, in #80271 Jan Niklas Hasse requested, and I concur, to be able to
set the same via an env var.  This flag doesn't affect code produced by gcc
in any way, thus it's reasonable to set it via env.  There's already
GCC_COLORS letting you customize colors used, you just can't tell it to
ignore stderr not being a terminal.


Please forgive my lack of knowledge wrt proper ways of submitting patches to
gcc (ie, when biting please point to piece of TFM I failed to R).  In
particular, I'm not sure how to run the testsuite: it produces a lot of
fails even without my patch, and I don't know what is your way to spot new
failures.  The testsuite could be interesting as gcc in a lot of places sets
and unsets CLICOLOR_FORCE despite not supporting it itself (yet a number of
other tools do).  Weirdly, in every case you unset it in the very next line
after enabling it.

I've submitted an equivalent of this patch to llvm/clang as well.

-- >8 -- >8 -- >8 -- >8 -- >8 -- >8 -- >8 -- >8 -- >8 -- >8 -- >8 -- >8 --
From: Adam Borowski <kilobyte@angband.pl>
Date: Mon, 11 Jun 2018 01:01:46 +0200
Subject: [PATCH] Let env var CLICOLOR_FORCE set -fdiagnostics-color=always.

---
 gcc/diagnostic.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

Comments

Jeff Law June 22, 2018, 8:08 p.m. | #1
On 06/12/2018 04:46 PM, Adam Borowski wrote:
> Hi!
> Here's a patch for Bugzilla 80271, to add support for env CLICOLOR_FORCE.
> 
> Automated build environments -- and even some which you run by hand --
> redirect the output somewhere to save a log, even if they tee it to the
> terminal anyway.  This kills all colored highlights for diagnostics.
> Those highlights are really nice for a human -- unlike the old days when
> a single-threaded build always had the failure at the end, modern setups
> tend to spew pages of stuff after the real reason your build failed.  As
> human vision evolved to be extremely good at noticing a fleck of color,
> it's easy to spot that bit of red even when scrolling very fast.
> 
> Besides terminals, you can read colored logs via "less -R", ansi2html, etc.
> 
> For such piped setups, gcc has -fdiagnostics-color=always.  Alas, while this
> flag is easy to set by someone writing a given piece of software, there's
> no reasonable way to set it globally in the build environment.
> 
> Thus, in #80271 Jan Niklas Hasse requested, and I concur, to be able to
> set the same via an env var.  This flag doesn't affect code produced by gcc
> in any way, thus it's reasonable to set it via env.  There's already
> GCC_COLORS letting you customize colors used, you just can't tell it to
> ignore stderr not being a terminal.
> 
> 
> Please forgive my lack of knowledge wrt proper ways of submitting patches to
> gcc (ie, when biting please point to piece of TFM I failed to R).  In
> particular, I'm not sure how to run the testsuite: it produces a lot of
> fails even without my patch, and I don't know what is your way to spot new
> failures.  The testsuite could be interesting as gcc in a lot of places sets
> and unsets CLICOLOR_FORCE despite not supporting it itself (yet a number of
> other tools do).  Weirdly, in every case you unset it in the very next line
> after enabling it.
> 
> I've submitted an equivalent of this patch to llvm/clang as well.
We _strongly_ discourage the use of environment variables to control the
compiler.  Really the way to go here is to use the
-fdiagnostics-color=always and deal with the difficulties of flag injection.

Jeff
Adam Borowski June 22, 2018, 8:19 p.m. | #2
On Fri, Jun 22, 2018 at 02:08:27PM -0600, Jeff Law wrote:
> On 06/12/2018 04:46 PM, Adam Borowski wrote:
> > Here's a patch for Bugzilla 80271, to add support for env CLICOLOR_FORCE.
> > 
> > Automated build environments -- and even some which you run by hand --
> > redirect the output somewhere to save a log, even if they tee it to the
> > terminal anyway.  This kills all colored highlights for diagnostics.
> > Those highlights are really nice for a human -- unlike the old days when
> > a single-threaded build always had the failure at the end, modern setups
> > tend to spew pages of stuff after the real reason your build failed.  As
> > human vision evolved to be extremely good at noticing a fleck of color,
> > it's easy to spot that bit of red even when scrolling very fast.
> > 
> > Besides terminals, you can read colored logs via "less -R", ansi2html, etc.
> > 
> > For such piped setups, gcc has -fdiagnostics-color=always.  Alas, while this
> > flag is easy to set by someone writing a given piece of software, there's
> > no reasonable way to set it globally in the build environment.
> > 
> > Thus, in #80271 Jan Niklas Hasse requested, and I concur, to be able to
> > set the same via an env var.  This flag doesn't affect code produced by gcc
> > in any way, thus it's reasonable to set it via env.  There's already
> > GCC_COLORS letting you customize colors used, you just can't tell it to
> > ignore stderr not being a terminal.

> We _strongly_ discourage the use of environment variables to control the
> compiler.  Really the way to go here is to use the
> -fdiagnostics-color=always and deal with the difficulties of flag injection.

As discussed in #80271, color vs b&w diagnostics don't change any produced
files in any way.  The presentation of these diagnostics is already affected
by things not included in -f args (namely, isatty(2)).

And, this very feature is already controlled by another env variable, ie,
GCC_COLORS.

Another reason is that, certain build systems store -f flags and consider a
build to be different if the flags differ.  This can cause spurious
recompiles just because one invocation was redirected but another was not,
something likely to happen in a debugging session.


Meow!

Patch

diff --git a/gcc/diagnostic.c b/gcc/diagnostic.c
index e22c17bc02c..d9e352932c6 100644
--- a/gcc/diagnostic.c
+++ b/gcc/diagnostic.c
@@ -190,12 +190,18 @@  diagnostic_color_init (diagnostic_context *context, int value /*= -1 */)
   /* value == -1 is the default value.  */
   if (value < 0)
     {
+      /* Explicit command-line args override env, but env prevails over
+         defaults, whatever they are.
+         The spec defines an existing but empty var to mean "yes".  */
+      const char* clic = getenv ("CLICOLOR_FORCE");
+      if (clic && strcmp (clic, "0"))
+	value = DIAGNOSTICS_COLOR_YES;
       /* If DIAGNOSTICS_COLOR_DEFAULT is -1, default to
 	 -fdiagnostics-color=auto if GCC_COLORS is in the environment,
 	 otherwise default to -fdiagnostics-color=never, for other
 	 values default to that
 	 -fdiagnostics-color={never,auto,always}.  */
-      if (DIAGNOSTICS_COLOR_DEFAULT == -1)
+      else if (DIAGNOSTICS_COLOR_DEFAULT == -1)
 	{
 	  if (!getenv ("GCC_COLORS"))
 	    return;