Rollback of commit 1e37a5375f918376c132fa537e25695f673f41b8.

*** Reason for rollback ***

Apparently we now try to open output files for the process twice: once when we are constructing the output streams, and the second time when we tell the process to redirect its outputs. This causes the outputs to be empty on Windows

*** Original change description ***

Do redirection of stdout / stderr in Java instead of reimplementing it in every process wrapper again.

--
MOS_MIGRATED_REVID=126801016

5 files changed, 37 insertions, 50 deletions

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 0a85494d99..729bace0ef 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
@@ -254,6 +254,8 @@ public class RunCommand implements BlazeCommand  {
       cmdLine.add(env.getExecRoot().getRelative(processWrapperPath).getPathString());
       cmdLine.add("-1");
       cmdLine.add("15");
+      cmdLine.add("-");
+      cmdLine.add("-");
     }
     List<String> prettyCmdLine = new ArrayList<>();
     // Insert the command prefix specified by the "--run_under=<command-prefix>" option
diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/NamespaceSandboxRunner.java b/src/main/java/com/google/devtools/build/lib/sandbox/NamespaceSandboxRunner.java
index b60f47dbfd..9516fe2753 100644
--- a/src/main/java/com/google/devtools/build/lib/sandbox/NamespaceSandboxRunner.java
+++ b/src/main/java/com/google/devtools/build/lib/sandbox/NamespaceSandboxRunner.java
@@ -190,8 +190,8 @@ public class NamespaceSandboxRunner {
       cmd.execute(
           /* stdin */ new byte[] {},
           Command.NO_OBSERVER,
-          outErr.getOutputFile(),
-          outErr.getErrorFile(),
+          outErr.getOutputStream(),
+          outErr.getErrorStream(),
           /* killSubprocessOnInterrupt */ true);
     } catch (CommandException e) {
       boolean timedOut = false;
diff --git a/src/main/java/com/google/devtools/build/lib/shell/Command.java b/src/main/java/com/google/devtools/build/lib/shell/Command.java
index 8d40c15e24..84f78d2ae3 100644
--- a/src/main/java/com/google/devtools/build/lib/shell/Command.java
+++ b/src/main/java/com/google/devtools/build/lib/shell/Command.java
@@ -410,10 +410,12 @@ public final class Command {
   }
 
   /**
-   * Like {@link #execute(byte[], KillableObserver, OutputStream, OutputStream, boolean)} but allows
-   * using files to redirect stdOut and stdErr. This gives better performance as the data doesn't
-   * flow through the JVM but instead is written directly to the corresponding file descriptors by
-   * the process.
+   * Execute this command with given input to stdin; this stream is closed when the process
+   * terminates, and exceptions raised when closing this stream are ignored. This call blocks until
+   * the process completes or an error occurs. The caller provides {@link OutputStream} instances
+   * into which the process writes its stdout/stderr output; these streams are <em>not</em> closed
+   * when the process terminates. The given {@link KillableObserver} may also terminate the process
+   * early while running.
    *
    * <p>If stdOut or stdErr is {@code null}, it will be redirected to /dev/null.
    */
@@ -680,12 +682,10 @@ public final class Command {
 
     final Subprocess process = startProcess();
 
-    if (outErrConsumers != null) {
-      outErrConsumers.logConsumptionStrategy();
+    outErrConsumers.logConsumptionStrategy();
 
-      outErrConsumers.registerInputs(
-          process.getInputStream(), process.getErrorStream(), closeOutputStreams);
-    }
+    outErrConsumers.registerInputs(
+        process.getInputStream(), process.getErrorStream(), closeOutputStreams);
 
     processInput(stdinInput, process);
 
@@ -826,12 +826,10 @@ public final class Command {
     log.finer(status.toString());
 
     try {
-      if (outErr != null) {
-        if (Thread.currentThread().isInterrupted()) {
-          outErr.cancel();
-        } else {
-          outErr.waitForCompletion();
-        }
+      if (Thread.currentThread().isInterrupted()) {
+        outErr.cancel();
+      } else {
+        outErr.waitForCompletion();
       }
     } catch (IOException ioe) {
       CommandResult noOutputResult =
@@ -854,10 +852,7 @@ public final class Command {
     }
 
     CommandResult result =
-        new CommandResult(
-            outErr != null ? outErr.getAccumulatedOut() : CommandResult.NO_OUTPUT_COLLECTED,
-            outErr != null ? outErr.getAccumulatedErr() : CommandResult.NO_OUTPUT_COLLECTED,
-            status);
+        new CommandResult(outErr.getAccumulatedOut(), outErr.getAccumulatedErr(), status);
     result.logThis();
     if (status.success()) {
       return result;
diff --git a/src/main/java/com/google/devtools/build/lib/standalone/StandaloneSpawnStrategy.java b/src/main/java/com/google/devtools/build/lib/standalone/StandaloneSpawnStrategy.java
index 4a666832e1..6c5df1b864 100644
--- a/src/main/java/com/google/devtools/build/lib/standalone/StandaloneSpawnStrategy.java
+++ b/src/main/java/com/google/devtools/build/lib/standalone/StandaloneSpawnStrategy.java
@@ -99,6 +99,11 @@ public class StandaloneSpawnStrategy implements SpawnActionContext {
       args.add(processWrapper.getPathString());
       args.add(Integer.toString(timeout));
       args.add("5"); /* kill delay: give some time to print stacktraces and whatnot. */
+
+      // TODO(bazel-team): use process-wrapper redirection so we don't have to
+      // pass test logs through the Java heap.
+      args.add("-"); /* stdout. */
+      args.add("-"); /* stderr. */
     }
     args.addAll(spawn.getArguments());
 
@@ -111,8 +116,8 @@ public class StandaloneSpawnStrategy implements SpawnActionContext {
       cmd.execute(
           /* stdin */ new byte[] {},
           Command.NO_OBSERVER,
-          outErr.getOutputFile(),
-          outErr.getErrorFile(),
+          outErr.getOutputStream(),
+          outErr.getErrorStream(),
           /*killSubprocessOnInterrupt*/ true);
     } catch (AbnormalTerminationException e) {
       TerminationStatus status = e.getResult().getTerminationStatus();
diff --git a/src/main/java/com/google/devtools/build/lib/util/io/FileOutErr.java b/src/main/java/com/google/devtools/build/lib/util/io/FileOutErr.java
index add8a0f792..b1e0faa68e 100644
--- a/src/main/java/com/google/devtools/build/lib/util/io/FileOutErr.java
+++ b/src/main/java/com/google/devtools/build/lib/util/io/FileOutErr.java
@@ -19,7 +19,6 @@ import com.google.devtools.build.lib.concurrent.ThreadSafety;
 import com.google.devtools.build.lib.vfs.FileSystemUtils;
 import com.google.devtools.build.lib.vfs.Path;
 
-import java.io.File;
 import java.io.IOException;
 import java.io.InputStream;
 import java.io.OutputStream;
@@ -52,6 +51,18 @@ public class FileOutErr extends OutErr {
     this(new FileRecordingOutputStream(stdout), new FileRecordingOutputStream(stderr));
   }
 
+  /**
+   * Creates a new FileOutErr that writes its input to the file specified by output. Both
+   * stdout/stderr will be copied into the single file.
+   *
+   * @param output The file for the both stdout and stderr of this outErr.
+   */
+  public FileOutErr(Path output) {
+    // We don't need to create a synchronized funnel here, like in the OutErr -- The
+    // respective functions in the FileRecordingOutputStream take care of locking.
+    this(new FileRecordingOutputStream(output));
+  }
+
   protected FileOutErr(AbstractFileRecordingOutputStream out,
                        AbstractFileRecordingOutputStream err) {
     super(out, err);
@@ -113,32 +124,6 @@ public class FileOutErr extends OutErr {
     return getFileErrorStream().getFile();
   }
 
-  /**
-   * Returns the {@link File} this OutErr uses to buffer stdout
-   *
-   * <p>The user must ensure that no other process is writing to the files at time of creation.
-   *
-   * @return the file object with the contents of stdout
-   */
-  public File getOutputFile() {
-    if (getFileOutputStream().getFile() != null) {
-      return getFileOutputStream().getFile().getPathFile();
-    }
-    return null;
-  }
-
-  /**
-   * Returns the {@link File} this OutErr uses to buffer stderr.
-   *
-   * @return the file object with the contents of stderr
-   */
-  public File getErrorFile() {
-    if (getFileErrorStream().getFile() != null) {
-      return getFileErrorStream().getFile().getPathFile();
-    }
-    return null;
-  }
-
   /** Interprets the captured out content as an {@code ISO-8859-1} encoded string. */
   public String outAsLatin1() {
     return getFileOutputStream().getRecordedOutput();