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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman marked 2 inline comments as done.
aaron.ballman added a comment.

In D152285#4401348 <https://reviews.llvm.org/D152285#4401348>, @MaskRay wrote:

> If we don't intend to support both standards, we can close https://github.com/llvm/llvm-project/issues/23983 (CLICOLOR) as a wontfix :)

That was my plan.

In D152285#4402603 <https://reviews.llvm.org/D152285#4402603>, @jhasse wrote:

> There's a valid usecase `CLICOLOR_FORCE`: Force color diagnostics. The "disable colors" part of https://bixense.com/clicolors/ is not that important to me, I could change it to point to `NO_COLOR` instead?
>
> btw: I've tried to join the "standards" a few years ago: https://github.com/jcs/no_color/issues/28
>
> Right now I think it would be best to drop `CLICOLOR` and have the following simple rules:
>
> - if `NO_COLOR` set: disable colors
> - if `CLICOLOR_FORCE` set: always set colors
> - else: isatty
>
> What do you think?

I don't think we should implement all of one standard and part of another. My thinking is: users will have to do `NO_COLOR=1` and `CLICOLOR_FORCE=0` to disable colors due to there being competing standards, so we need to support only one of these. `CLICOLOR_FORCE` is the more powerful option, but I don't know what compelling use case there is for forcing colors *on*, but forcing them *off* makes a lot of sense to me from an accessibility standpoint. So I think supporting `NO_COLOR` alone is sufficient until we know why users need to force-enable colors. If we are compelled by that reasoning, we could implement the other standard at that point (and I'd suggest we implement it fully). But I'd like to avoid that right now because then we have to think about what happens in circumstances like `NO_COLOR=1 CLICOLOR_FORCE=1`



================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:2346
+  bool ShowColors = true;
+  if (std::optional<std::string> NoColor =
+          llvm::sys::Process::GetEnv("NO_COLOR")) {
----------------
MaskRay wrote:
> We should inspect that `NO_COLOR` is not empty.
> 
> > Command-line software which adds ANSI color to its output by default should check for a NO_COLOR environment variable that, when **present and not an empty string** (regardless of its value), prevents the addition of ANSI color.
Good catch!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152285