LocalSpawnRunner: on Windows, don't use the process wrapper for now

This is ported from StandaloneSpawnStrategy.
PiperOrigin-RevId: 152493898

2 files changed, 85 insertions, 18 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunner.java b/src/main/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunner.java
index af4be6e5a5..f499800b8c 100644
--- a/src/main/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunner.java
+++ b/src/main/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunner.java
@@ -35,6 +35,7 @@ import com.google.devtools.build.lib.shell.CommandException;
 import com.google.devtools.build.lib.shell.CommandResult;
 import com.google.devtools.build.lib.shell.TerminationStatus;
 import com.google.devtools.build.lib.util.NetUtil;
+import com.google.devtools.build.lib.util.OS;
 import com.google.devtools.build.lib.util.Preconditions;
 import com.google.devtools.build.lib.util.io.FileOutErr;
 import com.google.devtools.build.lib.vfs.Path;
@@ -65,21 +66,41 @@ public final class LocalSpawnRunner implements SpawnRunner {
 
   private final ActionInputPrefetcher actionInputPrefetcher;
 
-  private final String processWrapper;
-
   private final LocalExecutionOptions localExecutionOptions;
 
+  private final boolean useProcessWrapper;
+  private final String processWrapper;
+
   public LocalSpawnRunner(
       Path execRoot,
       ActionInputPrefetcher actionInputPrefetcher,
       LocalExecutionOptions localExecutionOptions,
-      ResourceManager resourceManager) {
+      ResourceManager resourceManager,
+      boolean useProcessWrapper) {
     this.execRoot = execRoot;
     this.actionInputPrefetcher = Preconditions.checkNotNull(actionInputPrefetcher);
     this.processWrapper = execRoot.getRelative("_bin/process-wrapper").getPathString();
     this.localExecutionOptions = localExecutionOptions;
     this.hostName = NetUtil.findShortHostName();
     this.resourceManager = resourceManager;
+    this.useProcessWrapper = useProcessWrapper;
+  }
+
+  public LocalSpawnRunner(
+      Path execRoot,
+      ActionInputPrefetcher actionInputPrefetcher,
+      LocalExecutionOptions localExecutionOptions,
+      ResourceManager resourceManager) {
+    this(
+        execRoot,
+        actionInputPrefetcher,
+        localExecutionOptions,
+        resourceManager,
+        // TODO(bazel-team): process-wrapper seems to work on Windows, but requires additional setup
+        // as it is an msys2 binary, so it needs msys2 DLLs on %PATH%. Disable it for now to make
+        // the setup easier and to avoid further PATH hacks. Ideally we should have a native
+        // implementation of process-wrapper for Windows.
+        OS.getCurrent() != OS.WINDOWS);
   }
 
   @Override
@@ -198,20 +219,32 @@ public final class LocalSpawnRunner implements SpawnRunner {
             Status.LOCAL_ACTION_NOT_ALLOWED);
       }
 
-      List<String> cmdLine = new ArrayList<>();
-      cmdLine.add(processWrapper);
-      cmdLine.add(Float.toString(timeout));
-      cmdLine.add(Double.toString(localExecutionOptions.localSigkillGraceSeconds));
-      cmdLine.add(getPathOrDevNull(outErr.getOutputPath()));
-      cmdLine.add(getPathOrDevNull(outErr.getErrorPath()));
-      cmdLine.addAll(args);
-      Command cmd = new Command(cmdLine.toArray(new String[]{}), env, execRoot.getPathFile());
+      Command cmd;
+      OutputStream stdOut = ByteStreams.nullOutputStream();
+      OutputStream stdErr = ByteStreams.nullOutputStream();
+      if (useProcessWrapper) {
+        List<String> cmdLine = new ArrayList<>();
+        cmdLine.add(processWrapper);
+        cmdLine.add(Float.toString(timeout));
+        cmdLine.add(Double.toString(localExecutionOptions.localSigkillGraceSeconds));
+        cmdLine.add(getPathOrDevNull(outErr.getOutputPath()));
+        cmdLine.add(getPathOrDevNull(outErr.getErrorPath()));
+        cmdLine.addAll(args);
+        cmd = new Command(cmdLine.toArray(new String[]{}), env, execRoot.getPathFile());
+      } else {
+        stdOut = outErr.getOutputStream();
+        stdErr = outErr.getErrorStream();
+        cmd = new Command(
+            args.toArray(new String[0]),
+            env,
+            execRoot.getPathFile(),
+            (int) timeout);
+      }
 
       long startTime = System.currentTimeMillis();
-      OutputStream nullOut = ByteStreams.nullOutputStream();
       CommandResult result;
       try {
-        result = cmd.execute(Command.NO_INPUT, Command.NO_OBSERVER, nullOut, nullOut, true);
+        result = cmd.execute(Command.NO_INPUT, Command.NO_OBSERVER, stdOut, stdErr, true);
         if (Thread.currentThread().isInterrupted()) {
           throw new InterruptedException();
         }
diff --git a/src/test/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunnerTest.java b/src/test/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunnerTest.java
index 8fba192e15..97265c4438 100644
--- a/src/test/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunnerTest.java
+++ b/src/test/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunnerTest.java
@@ -60,6 +60,9 @@ import org.mockito.ArgumentCaptor;
  */
 @RunWith(JUnit4.class)
 public class LocalSpawnRunnerTest {
+  private static final boolean USE_WRAPPER = true;
+  private static final boolean NO_WRAPPER = false;
+
   private static class FinishedSubprocess implements Subprocess {
     private final int exitCode;
 
@@ -193,7 +196,7 @@ public class LocalSpawnRunnerTest {
     LocalExecutionOptions options = Options.getDefaults(LocalExecutionOptions.class);
     options.localSigkillGraceSeconds = 456;
     LocalSpawnRunner runner = new LocalSpawnRunner(
-        fs.getPath("/execroot"), ActionInputPrefetcher.NONE, options, resourceManager);
+        fs.getPath("/execroot"), ActionInputPrefetcher.NONE, options, resourceManager, USE_WRAPPER);
 
     timeoutMillis = 123 * 1000L;
     outErr = new FileOutErr(fs.getPath("/out/stdout"), fs.getPath("/out/stderr"));
@@ -210,6 +213,37 @@ public class LocalSpawnRunnerTest {
             "/execroot/_bin/process-wrapper", "123.0", "456.0", "/out/stdout", "/out/stderr",
             "/bin/echo", "Hi!"));
     assertThat(captor.getValue().getEnv()).containsExactly("VARIABLE", "value");
+    assertThat(captor.getValue().getTimeoutMillis()).isEqualTo(-1);
+
+    assertThat(calledLockOutputFiles).isTrue();
+  }
+
+  @Test
+  public void noProcessWrapper() throws Exception {
+    Subprocess.Factory factory = mock(Subprocess.Factory.class);
+    ArgumentCaptor<SubprocessBuilder> captor = ArgumentCaptor.forClass(SubprocessBuilder.class);
+    when(factory.create(captor.capture())).thenReturn(new FinishedSubprocess(0));
+    SubprocessBuilder.setSubprocessFactory(factory);
+
+    LocalExecutionOptions options = Options.getDefaults(LocalExecutionOptions.class);
+    options.localSigkillGraceSeconds = 456;
+    LocalSpawnRunner runner = new LocalSpawnRunner(
+        fs.getPath("/execroot"), ActionInputPrefetcher.NONE, options, resourceManager, NO_WRAPPER);
+
+    timeoutMillis = 123 * 1000L;
+    outErr = new FileOutErr(fs.getPath("/out/stdout"), fs.getPath("/out/stderr"));
+    SpawnResult result = runner.exec(SIMPLE_SPAWN, policy);
+    verify(factory).create(any());
+    assertThat(result.status()).isEqualTo(SpawnResult.Status.SUCCESS);
+    assertThat(result.exitCode()).isEqualTo(0);
+    assertThat(result.setupSuccess()).isTrue();
+    assertThat(result.getExecutorHostName()).isEqualTo(NetUtil.findShortHostName());
+
+    assertThat(captor.getValue().getArgv())
+        .isEqualTo(ImmutableList.of("/bin/echo", "Hi!"));
+    assertThat(captor.getValue().getEnv()).containsExactly("VARIABLE", "value");
+    // Without the process wrapper, we use the Command API to enforce the timeout.
+    assertThat(captor.getValue().getTimeoutMillis()).isEqualTo(timeoutMillis);
 
     assertThat(calledLockOutputFiles).isTrue();
   }
@@ -223,7 +257,7 @@ public class LocalSpawnRunnerTest {
 
     LocalExecutionOptions options = Options.getDefaults(LocalExecutionOptions.class);
     LocalSpawnRunner runner = new LocalSpawnRunner(
-        fs.getPath("/execroot"), ActionInputPrefetcher.NONE, options, resourceManager);
+        fs.getPath("/execroot"), ActionInputPrefetcher.NONE, options, resourceManager, USE_WRAPPER);
 
     outErr = new FileOutErr(fs.getPath("/out/stdout"), fs.getPath("/out/stderr"));
     SpawnResult result = runner.exec(SIMPLE_SPAWN, policy);
@@ -252,7 +286,7 @@ public class LocalSpawnRunnerTest {
 
     LocalExecutionOptions options = Options.getDefaults(LocalExecutionOptions.class);
     LocalSpawnRunner runner = new LocalSpawnRunner(
-        fs.getPath("/execroot"), ActionInputPrefetcher.NONE, options, resourceManager);
+        fs.getPath("/execroot"), ActionInputPrefetcher.NONE, options, resourceManager, USE_WRAPPER);
 
     outErr = new FileOutErr(fs.getPath("/out/stdout"), fs.getPath("/out/stderr"));
     SpawnResult result = runner.exec(SIMPLE_SPAWN, policy);
@@ -271,7 +305,7 @@ public class LocalSpawnRunnerTest {
     LocalExecutionOptions options = Options.getDefaults(LocalExecutionOptions.class);
     options.allowedLocalAction = Pattern.compile("none");
     LocalSpawnRunner runner = new LocalSpawnRunner(
-        fs.getPath("/execroot"), ActionInputPrefetcher.NONE, options, resourceManager);
+        fs.getPath("/execroot"), ActionInputPrefetcher.NONE, options, resourceManager, USE_WRAPPER);
 
     outErr = new FileOutErr();
     SpawnResult reply = runner.exec(SIMPLE_SPAWN, policy);
@@ -309,7 +343,7 @@ public class LocalSpawnRunnerTest {
 
     LocalExecutionOptions options = Options.getDefaults(LocalExecutionOptions.class);
     LocalSpawnRunner runner = new LocalSpawnRunner(
-        fs.getPath("/execroot"), ActionInputPrefetcher.NONE, options, resourceManager);
+        fs.getPath("/execroot"), ActionInputPrefetcher.NONE, options, resourceManager, USE_WRAPPER);
 
     outErr = new FileOutErr(fs.getPath("/out/stdout"), fs.getPath("/out/stderr"));
     try {