[PATCH] D152285: Add support for the NO_COLOR environment variable

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman updated this revision to Diff 529299.
aaron.ballman marked an inline comment as done.
aaron.ballman added a comment.

Update based on review feedback so that we check for an empty environment variable value.

This was a fun rabbit hole to fall into: lit is not a PTY, so our fallback of "automatically enable colors" means `env NO_COLOR=` still disables colors as far as lit tests are concerned. See https://github.com/llvm/llvm-project/blob/4418434c6de7a861e241ba2448ea4a12080cf08f/clang/test/Driver/color-diagnostics.c#L25 for another test running into this. What's more, Windows does not have a way to set an empty environment variable on the command line from what I can find, so testing this manually also doesn't work (at least for me). However, because this piggy-backs on the functionality for `-fdiagnostics-color=auto`, I think the test coverage I added is sufficient.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D152285/new/

https://reviews.llvm.org/D152285

Files:
  clang/docs/ReleaseNotes.rst
  clang/docs/UsersManual.rst
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/Driver/no-color.c


Index: clang/test/Driver/no-color.c
===================================================================
--- /dev/null
+++ clang/test/Driver/no-color.c
@@ -0,0 +1,17 @@
+// RUN: env NO_COLOR=1 %clang -### %s 2>&1 | FileCheck --check-prefix=NO-COLOR %s
+// RUN: env NO_COLOR=1 %clang -fcolor-diagnostics -### %s 2>&1 | FileCheck --check-prefix=COLOR %s
+// RUN: env NO_COLOR=1 %clang -fdiagnostics-color=auto -### %s 2>&1 | FileCheck --check-prefix=NO-COLOR %s
+// RUN: env NO_COLOR=1 %clang -fdiagnostics-color=always -### %s 2>&1 | FileCheck --check-prefix=COLOR %s
+// RUN: env NO_COLOR=1 %clang -fdiagnostics-color=never -### %s 2>&1 | FileCheck --check-prefix=NO-COLOR %s
+
+// Note, the value of the environment variable does not matter, only that it is defined and not empty.
+// RUN: env NO_COLOR=0 %clang -### %s 2>&1 | FileCheck --check-prefix=NO-COLOR %s
+// Note, an empty value means we automatically decide whether to enable colors or not, and lit tests
+// are not run in a PTY, so colors are disabled by default. There is no easy way for us to test this
+// configuration where auto enables colors.
+// RUN: env NO_COLOR= %clang -### %s 2>&1 | FileCheck --check-prefix=NO-COLOR %s
+
+int main(void) {}
+
+// COLOR: -fcolor-diagnostics
+// NO-COLOR-NOT: -fcolor-diagnostics
Index: clang/lib/Frontend/CompilerInvocation.cpp
===================================================================
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -2341,10 +2341,20 @@
   unsigned MissingArgIndex, MissingArgCount;
   InputArgList Args = getDriverOptTable().ParseArgs(
       Argv.slice(1), MissingArgIndex, MissingArgCount);
+
+  bool ShowColors = true;
+  if (std::optional<std::string> NoColor =
+          llvm::sys::Process::GetEnv("NO_COLOR");
+      NoColor && !NoColor->empty() && NoColor->at(0) != '\0') {
+    // If the user set the NO_COLOR environment variable, we'll honor that
+    // unless the command line overrides it.
+    ShowColors = false;
+  }
+
   // We ignore MissingArgCount and the return value of ParseDiagnosticArgs.
   // Any errors that would be diagnosed here will also be diagnosed later,
   // when the DiagnosticsEngine actually exists.
-  (void)ParseDiagnosticArgs(*DiagOpts, Args);
+  (void)ParseDiagnosticArgs(*DiagOpts, Args, /*Diags=*/nullptr, ShowColors);
   return DiagOpts;
 }
 
Index: clang/docs/UsersManual.rst
===================================================================
--- clang/docs/UsersManual.rst
+++ clang/docs/UsersManual.rst
@@ -251,6 +251,11 @@
                 ^
                 //
 
+   If the ``NO_COLOR`` environment variable is defined and not empty
+   (regardless of value), color diagnostics are disabled. If ``NO_COLOR`` is
+   defined and ``-fcolor-diagnostics`` is passed on the command line, Clang
+   will honor the command line argument.
+
 .. option:: -fansi-escape-codes
 
    Controls whether ANSI escape codes are used instead of the Windows Console
Index: clang/docs/ReleaseNotes.rst
===================================================================
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -222,6 +222,8 @@
 - Clang now support ``__builtin_FUNCSIG()`` which returns the same information
   as the ``__FUNCSIG__`` macro (available only with ``-fms-extensions`` flag).
   This fixes (`#58951 <https://github.com/llvm/llvm-project/issues/58951>`_).
+- Clang now supports the `NO_COLOR <https://no-color.org/>`_ environment
+  variable as a way to disable color diagnostics.
 
 New Compiler Flags
 ------------------


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D152285.529299.patch
Type: text/x-patch
Size: 3575 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20230607/49799e64/attachment.bin>