Fixed bug where blaze run with the --color=no flag was still printing out in color when the build fails.

Includes fix for problems causing the original slowdown to blaze query

--
MOS_MIGRATED_REVID=99755414

6 files changed, 67 insertions, 6 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/events/Reporter.java b/src/main/java/com/google/devtools/build/lib/events/Reporter.java
index e0c39250ba..d723adc6bd 100644
--- a/src/main/java/com/google/devtools/build/lib/events/Reporter.java
+++ b/src/main/java/com/google/devtools/build/lib/events/Reporter.java
@@ -13,6 +13,7 @@
 // limitations under the License.
 package com.google.devtools.build.lib.events;
 
+import com.google.common.base.Preconditions;
 import com.google.devtools.build.lib.util.io.OutErr;
 
 import java.io.PrintStream;
@@ -25,9 +26,10 @@ import java.util.List;
  * is not intended as a logging mechanism for developer-only messages; use a
  * Logger for that.
  *
- * The reporter instance is consumed by the build system, and passes events to
+ * <p>The reporter instance is consumed by the build system, and passes events to
  * {@link EventHandler} instances. These handlers are registered via {@link
- * #addHandler(EventHandler)}.
+ * #addHandler(EventHandler)}. The reporter's main use is in the blaze runtime
+ * and its lifetime is the lifetime of the blaze server.
  *
  * <p>Thread-safe: calls to {@code #report} may be made on any thread.
  * Handlers may be run in an arbitary thread (but right now, they will not be
@@ -42,6 +44,9 @@ public final class Reporter implements EventHandler, ExceptionListener {
    */
   private final OutErr outErrToReporter = outErrForReporter(this);
   private volatile OutputFilter outputFilter = OutputFilter.OUTPUT_EVERYTHING;
+  private EventHandler ansiAllowingHandler;
+  private EventHandler ansiStrippingHandler;
+  private boolean ansiAllowingHandlerRegistered;
 
   public Reporter() {}
 
@@ -82,6 +87,7 @@ public final class Reporter implements EventHandler, ExceptionListener {
    * Adds a handler to this reporter.
    */
   public synchronized void addHandler(EventHandler handler) {
+    Preconditions.checkNotNull(handler);
     handlers.add(handler);
   }
 
@@ -143,4 +149,34 @@ public final class Reporter implements EventHandler, ExceptionListener {
   public void setOutputFilter(OutputFilter outputFilter) {
     this.outputFilter = outputFilter;
   }
+
+  /**
+   * Registers an ANSI-control-code-allowing EventHandler with an ANSI-stripping EventHandler
+   * that is already registered with the reporter.  The ANSI-stripping handler can then be replaced
+   * with the ANSI-allowing handler by calling {@code #switchToAnsiAllowingHandler} which
+   * calls {@code removeHandler} for the ANSI-stripping handler and then {@code addHandler} for the
+   * ANSI-allowing handler.
+   */
+  public synchronized void registerAnsiAllowingHandler(
+      EventHandler ansiStrippingHandler,
+      EventHandler ansiAllowingHandler) {
+    this.ansiAllowingHandler = ansiAllowingHandler;
+    this.ansiStrippingHandler = ansiStrippingHandler;
+    ansiAllowingHandlerRegistered = true;
+  }
+
+  /**
+   * Restores the ANSI-allowing EventHandler registered using
+   * {@link #registerAnsiAllowingHandler}.
+   */
+  public synchronized void switchToAnsiAllowingHandler() {
+    if (ansiAllowingHandlerRegistered) {
+      removeHandler(ansiStrippingHandler);
+      addHandler(ansiAllowingHandler);
+      ansiStrippingHandler = null;
+      ansiAllowingHandler = null;
+      ansiAllowingHandlerRegistered = false;
+    }
+  }
+
 }
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/BlazeCommandDispatcher.java b/src/main/java/com/google/devtools/build/lib/runtime/BlazeCommandDispatcher.java
index 4d7e9aefe8..2e63b79758 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/BlazeCommandDispatcher.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/BlazeCommandDispatcher.java
@@ -362,13 +362,14 @@ public class BlazeCommandDispatcher {
     // Setup log filtering
     BlazeCommandEventHandler.Options eventHandlerOptions =
         optionsParser.getOptions(BlazeCommandEventHandler.Options.class);
+    OutErr colorfulOutErr = outErr;
     if (!eventHandlerOptions.useColor()) {
+      outErr = ansiStripOut(ansiStripErr(outErr));
       if (!commandAnnotation.binaryStdOut()) {
-        outErr = ansiStripOut(outErr);
+        colorfulOutErr = ansiStripOut(colorfulOutErr);
       }
-
       if (!commandAnnotation.binaryStdErr()) {
-        outErr = ansiStripErr(outErr);
+        colorfulOutErr = ansiStripErr(colorfulOutErr);
       }
     }
 
@@ -384,6 +385,17 @@ public class BlazeCommandDispatcher {
     EventHandler handler = createEventHandler(outErr, eventHandlerOptions);
     Reporter reporter = runtime.getReporter();
     reporter.addHandler(handler);
+
+    // We register an ANSI-allowing handler associated with {@code handler} so that ANSI control
+    // codes can be re-introduced later even if blaze is invoked with --color=no. This is useful
+    // for commands such as 'blaze run' where the output of the final executable shouldn't be
+    // modified.
+    EventHandler ansiAllowingHandler = null;
+    if (!eventHandlerOptions.useColor()) {
+      ansiAllowingHandler = createEventHandler(colorfulOutErr, eventHandlerOptions);
+      reporter.registerAnsiAllowingHandler(handler, ansiAllowingHandler);
+    }
+
     try {
       // While a Blaze command is active, direct all errors to the client's
       // event handler (and out/err streams).
@@ -436,6 +448,10 @@ public class BlazeCommandDispatcher {
       System.setErr(savedErr);
       reporter.removeHandler(handler);
       releaseHandler(handler);
+      if (!eventHandlerOptions.useColor()) {
+        reporter.removeHandler(ansiAllowingHandler);
+        releaseHandler(ansiAllowingHandler);
+      }
       runtime.getTimestampGranularityMonitor().waitForTimestampGranularity(outErr);
     }
   }
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/Command.java b/src/main/java/com/google/devtools/build/lib/runtime/Command.java
index 318e3e419b..c030a1d558 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/Command.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/Command.java
@@ -75,12 +75,16 @@ public @interface Command {
   /**
    * Returns true if this command wants to write binary data to stdout.
    * Enabling this flag will disable ANSI escape stripping for this command.
+   * This should be used in conjunction with {@code Reporter#switchToAnsiAllowingHandler}.
+   * See {@link RunCommand} for example usage.
    */
   boolean binaryStdOut() default false;
 
   /**
    * Returns true if this command wants to write binary data to stderr.
    * Enabling this flag will disable ANSI escape stripping for this command.
+   * This should be used in conjunction with {@code Reporter#switchToAnsiAllowingHandler}.
+   * See {@link RunCommand} for example usage.
    */
   boolean binaryStdErr() default false;
 
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/commands/InfoCommand.java b/src/main/java/com/google/devtools/build/lib/runtime/commands/InfoCommand.java
index 2448d90aca..d1f18cefcd 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/commands/InfoCommand.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/commands/InfoCommand.java
@@ -181,8 +181,8 @@ public class InfoCommand implements BlazeCommand {
 
   @Override
   public ExitCode exec(final BlazeRuntime runtime, final OptionsProvider optionsProvider) {
+    runtime.getReporter().switchToAnsiAllowingHandler();
     Options infoOptions = optionsProvider.getOptions(Options.class);
-
     OutErr outErr = runtime.getReporter().getOutErr();
     // Creating a BuildConfiguration is expensive and often unnecessary. Delay the creation until
     // it is needed.
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/commands/QueryCommand.java b/src/main/java/com/google/devtools/build/lib/runtime/commands/QueryCommand.java
index ecc487534b..04e2b047c3 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/commands/QueryCommand.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/commands/QueryCommand.java
@@ -136,6 +136,7 @@ public final class QueryCommand implements BlazeCommand {
       return ExitCode.ANALYSIS_FAILURE;
     }
 
+    runtime.getReporter().switchToAnsiAllowingHandler();
     // 3. Output results:
     PrintStream output = new PrintStream(runtime.getReporter().getOutErr().getOutputStream());
     try {
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/commands/RunCommand.java b/src/main/java/com/google/devtools/build/lib/runtime/commands/RunCommand.java
index 715ef0e385..16ed00abe9 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/commands/RunCommand.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/commands/RunCommand.java
@@ -305,6 +305,10 @@ public class RunCommand implements BlazeCommand  {
         .addArgs(cmdLine).setEnv(runtime.getClientEnv()).setWorkingDir(workingDir).build();
 
     try {
+      // Restore a raw EventHandler if it is registered. This allows for blaze run to produce the
+      // actual output of the command being run even if --color=no is specified.
+      runtime.getReporter().switchToAnsiAllowingHandler();
+
       // The command API is a little strange in that the following statement
       // will return normally only if the program exits with exit code 0.
       // If it ends with any other code, we have to catch BadExitStatusException.