Send ANSI color codes only to TTYs

[amccarth]

Summary

Color code changes were sent to the raw_ostream, even if the stream was piped to something other than a terminal (e.g., to FileCheck). This caused a new test to fail on Linux because the output differed from the expected output by the ANSI codes.

[chandlerc]

But we use this to send color codes to temporary files that capture output for build systems and such when -fcolor-diagnostics is explicitly provided? (As an example...)

[zturner]

In D31246#707656, @chandlerc wrote:

But we use this to send color codes to temporary files that capture output for build systems and such when -fcolor-diagnostics is explicitly provided? (As an example...)

That's strange, because everyone I asked thought that the raw_ostream stuff already worked this way -- i.e. doesn't send color if the destination doesn't support it. Is there a currently accepted practice for writing FileCheck test against tools that output color? Should every such tool have an explicit -no-color option?

[chandlerc]

In D31246#707661, @zturner wrote:

In D31246#707656, @chandlerc wrote:

But we use this to send color codes to temporary files that capture output for build systems and such when -fcolor-diagnostics is explicitly provided? (As an example...)

That's strange, because everyone I asked thought that the raw_ostream stuff already worked this way -- i.e. doesn't send color if the destination doesn't support it. Is there a currently accepted practice for writing FileCheck test against tools that output color? Should every such tool have an explicit -no-color option?

Possibly.... What's the failure?

[amccarth]

In D31246#707746, @chandlerc wrote:

In D31246#707661, @zturner wrote:

In D31246#707656, @chandlerc wrote:

But we use this to send color codes to temporary files that capture output for build systems and such when -fcolor-diagnostics is explicitly provided? (As an example...)

That's strange, because everyone I asked thought that the raw_ostream stuff already worked this way -- i.e. doesn't send color if the destination doesn't support it. Is there a currently accepted practice for writing FileCheck test against tools that output color? Should every such tool have an explicit -no-color option?

Possibly.... What's the failure?

I added some functionality to llvm-pdbdump and included a test for it that pipes the output of llvm-pdbdump through FileCheck. Worked on Windows, which doesn't use ANSI codes to achieve colors, but failed on Linux because the ANSI codes weren't expected.

I'm happy to add a -no-color option to llvm-pdbdump that I can use to make the test output consistent on all platforms if you think that's a better way to go.

[amccarth]

Here's a log of the test failure: http://lab.llvm.org:8011/builders/clang-with-lto-ubuntu/builds/1914/steps/test-stage1-compiler/logs/stdio

[chandlerc]

In D31246#707747, @amccarth wrote:

In D31246#707746, @chandlerc wrote:

In D31246#707661, @zturner wrote:

In D31246#707656, @chandlerc wrote:

But we use this to send color codes to temporary files that capture output for build systems and such when -fcolor-diagnostics is explicitly provided? (As an example...)

That's strange, because everyone I asked thought that the raw_ostream stuff already worked this way -- i.e. doesn't send color if the destination doesn't support it. Is there a currently accepted practice for writing FileCheck test against tools that output color? Should every such tool have an explicit -no-color option?

Possibly.... What's the failure?

I added some functionality to llvm-pdbdump and included a test for it that pipes the output of llvm-pdbdump through FileCheck. Worked on Windows, which doesn't use ANSI codes to achieve colors, but failed on Linux because the ANSI codes weren't expected.

I'm happy to add a -no-color option to llvm-pdbdump that I can use to make the test output consistent on all platforms if you think that's a better way to go.

I would look at how things like FileCheck tests for Clang's AST dump avoid tripping over ANSI codes.

My memory was that Clang itself checked a flag controlling color and whether the output was a TTY, and then set the flags to control color, and I would expect something similar here.

Essentially, that there would be a '-color-output' flag for llvm-pdbdump that defaults by detecting TTY but can be explicitly set to "on" and explicitly set to "off".

Without that kind of behavior, how would you write a test of the ANSI escape codes themselves with FileCheck, which you might want to do?

[amccarth]

In D31246#707786, @chandlerc wrote:

In D31246#707747, @amccarth wrote:

In D31246#707746, @chandlerc wrote:

In D31246#707661, @zturner wrote:

In D31246#707656, @chandlerc wrote:

But we use this to send color codes to temporary files that capture output for build systems and such when -fcolor-diagnostics is explicitly provided? (As an example...)

That's strange, because everyone I asked thought that the raw_ostream stuff already worked this way -- i.e. doesn't send color if the destination doesn't support it. Is there a currently accepted practice for writing FileCheck test against tools that output color? Should every such tool have an explicit -no-color option?

Possibly.... What's the failure?

I added some functionality to llvm-pdbdump and included a test for it that pipes the output of llvm-pdbdump through FileCheck. Worked on Windows, which doesn't use ANSI codes to achieve colors, but failed on Linux because the ANSI codes weren't expected.

I'm happy to add a -no-color option to llvm-pdbdump that I can use to make the test output consistent on all platforms if you think that's a better way to go.

I would look at how things like FileCheck tests for Clang's AST dump avoid tripping over ANSI codes.

My memory was that Clang itself checked a flag controlling color and whether the output was a TTY, and then set the flags to control color, and I would expect something similar here.

Essentially, that there would be a '-color-output' flag for llvm-pdbdump that defaults by detecting TTY but can be explicitly set to "on" and explicitly set to "off".

Without that kind of behavior, how would you write a test of the ANSI escape codes themselves with FileCheck, which you might want to do?

Well, any test of ANSI escape codes themselves would necessarily be platform specific, since not all the platforms use ANSI codes to control color and other rendering attributes.

Anyway, a -color-output flag with the default behavior that you suggest seems like the way to go. I'll abandon this and work on that. Thanks.

llvm/lib/Support/raw_ostream.cpp

 
 raw_ostream &raw_ostream::operator<<(const formatv_object_base &Obj) {
   SmallString<128> S;
   Obj.format(*this);
   return *this;
 }
 
 raw_ostream &raw_ostream::operator<<(const FormattedString &FS) {
-  unsigned Len = FS.Str.size(); 
+  unsigned Len = FS.Str.size();
   int PadAmount = FS.Width - Len;
   if (FS.RightJustify && (PadAmount > 0))
     this->indent(PadAmount);
   this->operator<<(FS.Str);
   if (!FS.RightJustify && (PadAmount > 0))
     this->indent(PadAmount);
   return *this;
 }
#if !defined(_MSC_VER) && !defined(__MINGW32__) && !defined(__minix)
   return statbuf.st_blksize;
 #else
   return raw_ostream::preferred_buffer_size();
 #endif
 }
 
 raw_ostream &raw_fd_ostream::changeColor(enum Colors colors, bool bold,
                                          bool bg) {
+  if (!has_colors())
+    return *this;
   if (sys::Process::ColorNeedsFlush())
     flush();
   const char *colorcode =
     (colors == SAVEDCOLOR) ? sys::Process::OutputBold(bg)
     : sys::Process::OutputColor(colors, bold, bg);
   if (colorcode) {
     size_t len = strlen(colorcode);
     write(colorcode, len);
     // don't account colors towards output characters
     pos -= len;
   }
   return *this;
 }
 
 raw_ostream &raw_fd_ostream::resetColor() {
+  if (!has_colors())
+    return *this;
   if (sys::Process::ColorNeedsFlush())
     flush();
   const char *colorcode = sys::Process::ResetColor();
   if (colorcode) {
     size_t len = strlen(colorcode);
     write(colorcode, len);
     // don't account colors towards output characters
     pos -= len;
   }
   return *this;
 }
 
 raw_ostream &raw_fd_ostream::reverseColor() {
+  if (!has_colors())
+    return *this;
   if (sys::Process::ColorNeedsFlush())
     flush();
   const char *colorcode = sys::Process::OutputReverse();
   if (colorcode) {
     size_t len = strlen(colorcode);
     write(colorcode, len);
     // don't account colors towards output characters
     pos -= len;