Add a CommandLineBuilder for the process-wrapper embedded tool, and use it everywhere instead of duplicating process-wrapper --shell_arguments in Blaze.

To avoid a cyclic dependency, I broke up exec/local:local into exec/local:local and exec/local:options.

RELNOTES: None.
PiperOrigin-RevId: 177419268

12 files changed, 252 insertions, 31 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/BUILD b/src/main/java/com/google/devtools/build/lib/BUILD
index cc0385a207..0e60f09aa6 100644
--- a/src/main/java/com/google/devtools/build/lib/BUILD
+++ b/src/main/java/com/google/devtools/build/lib/BUILD
@@ -1014,7 +1014,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/collect",
         "//src/main/java/com/google/devtools/build/lib/collect/nestedset",
         "//src/main/java/com/google/devtools/build/lib/concurrent",
-        "//src/main/java/com/google/devtools/build/lib/exec/local",
+        "//src/main/java/com/google/devtools/build/lib/exec/local:options",
         "//src/main/java/com/google/devtools/build/lib/profiler",
         "//src/main/java/com/google/devtools/build/lib/profiler:profiler-output",
         "//src/main/java/com/google/devtools/build/lib/profiler/memory:allocationtracker",
diff --git a/src/main/java/com/google/devtools/build/lib/exec/local/BUILD b/src/main/java/com/google/devtools/build/lib/exec/local/BUILD
index e9226e127f..a320ea95de 100644
--- a/src/main/java/com/google/devtools/build/lib/exec/local/BUILD
+++ b/src/main/java/com/google/devtools/build/lib/exec/local/BUILD
@@ -8,22 +8,36 @@ filegroup(
 
 java_library(
     name = "local",
-    srcs = glob(["*.java"]),
+    srcs = [
+        "LocalEnvProvider.java",
+        "LocalSpawnRunner.java",
+    ],
     data = [
         "//src/main/tools:process-wrapper",
     ],
     deps = [
+        ":options",
         "//src/main/java/com/google/devtools/build/lib:build-base",
         "//src/main/java/com/google/devtools/build/lib:io",
         "//src/main/java/com/google/devtools/build/lib:packages-internal",
         "//src/main/java/com/google/devtools/build/lib:process_util",
+        "//src/main/java/com/google/devtools/build/lib:runtime",
         "//src/main/java/com/google/devtools/build/lib:util",
         "//src/main/java/com/google/devtools/build/lib/actions",
         "//src/main/java/com/google/devtools/build/lib/concurrent",
         "//src/main/java/com/google/devtools/build/lib/shell",
         "//src/main/java/com/google/devtools/build/lib/vfs",
-        "//src/main/java/com/google/devtools/common/options",
         "//third_party:guava",
         "//third_party:jsr305",
     ],
 )
+
+java_library(
+    name = "options",
+    srcs = [
+        "LocalExecutionOptions.java",
+    ],
+    deps = [
+        "//src/main/java/com/google/devtools/common/options",
+    ],
+)
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 bfc6a8fcbb..3677d2cd4f 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
@@ -30,6 +30,7 @@ import com.google.devtools.build.lib.actions.SpawnResult.Status;
 import com.google.devtools.build.lib.actions.Spawns;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
 import com.google.devtools.build.lib.exec.SpawnRunner;
+import com.google.devtools.build.lib.runtime.ProcessWrapperUtil;
 import com.google.devtools.build.lib.shell.AbnormalTerminationException;
 import com.google.devtools.build.lib.shell.Command;
 import com.google.devtools.build.lib.shell.CommandException;
@@ -43,7 +44,6 @@ import com.google.devtools.build.lib.vfs.Path;
 import java.io.IOException;
 import java.io.OutputStream;
 import java.time.Duration;
-import java.util.ArrayList;
 import java.util.EnumMap;
 import java.util.List;
 import java.util.Map;
@@ -78,6 +78,7 @@ public final class LocalSpawnRunner implements SpawnRunner {
   private final String productName;
   private final LocalEnvProvider localEnvProvider;
 
+  // TODO(b/62588075): Move this logic to ProcessWrapperUtil?
   private static Path getProcessWrapper(Path execRoot, OS localOs) {
     return execRoot.getRelative("_bin/process-wrapper" + OsUtils.executableExtension(localOs));
   }
@@ -265,13 +266,15 @@ public final class LocalSpawnRunner implements SpawnRunner {
           // a stack trace, test log or similar, which is incredibly helpful for debugging. The
           // process wrapper also supports output file redirection, so we don't need to stream the
           // output through this process.
-          List<String> cmdLine = new ArrayList<>();
-          cmdLine.add(processWrapper);
-          cmdLine.add("--timeout=" + policy.getTimeout().getSeconds());
-          cmdLine.add("--kill_delay=" + localExecutionOptions.localSigkillGraceSeconds);
-          cmdLine.add("--stdout=" + getPathOrDevNull(outErr.getOutputPath()));
-          cmdLine.add("--stderr=" + getPathOrDevNull(outErr.getErrorPath()));
-          cmdLine.addAll(spawn.getArguments());
+          List<String> cmdLine =
+              ProcessWrapperUtil.commandLineBuilder()
+                  .setProcessWrapperPath(processWrapper)
+                  .setCommandArguments(spawn.getArguments())
+                  .setStdoutPath(getPathOrDevNull(outErr.getOutputPath()))
+                  .setStderrPath(getPathOrDevNull(outErr.getErrorPath()))
+                  .setTimeout(policy.getTimeout())
+                  .setKillDelay(Duration.ofSeconds(localExecutionOptions.localSigkillGraceSeconds))
+                  .build();
           cmd =
               new Command(
                   cmdLine.toArray(new String[0]),
diff --git a/src/main/java/com/google/devtools/build/lib/remote/BUILD b/src/main/java/com/google/devtools/build/lib/remote/BUILD
index e73289826c..e7290e27bf 100644
--- a/src/main/java/com/google/devtools/build/lib/remote/BUILD
+++ b/src/main/java/com/google/devtools/build/lib/remote/BUILD
@@ -27,6 +27,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/concurrent",
         "//src/main/java/com/google/devtools/build/lib/exec/apple",
         "//src/main/java/com/google/devtools/build/lib/exec/local",
+        "//src/main/java/com/google/devtools/build/lib/exec/local:options",
         "//src/main/java/com/google/devtools/build/lib/vfs",
         "//src/main/java/com/google/devtools/common/options",
         "//third_party:apache_httpclient",
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/ProcessWrapperUtil.java b/src/main/java/com/google/devtools/build/lib/runtime/ProcessWrapperUtil.java
index e212fb1a73..0aabf2ba00 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/ProcessWrapperUtil.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/ProcessWrapperUtil.java
@@ -14,12 +14,14 @@
 
 package com.google.devtools.build.lib.runtime;
 
+import com.google.common.base.Preconditions;
 import com.google.devtools.build.lib.util.OsUtils;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import java.time.Duration;
 import java.util.ArrayList;
 import java.util.List;
+import java.util.Optional;
 
 /**
  * Utility functions for the process wrapper embedded tool, which should work on most platforms and
@@ -41,23 +43,96 @@ public final class ProcessWrapperUtil {
     return execPath != null ? cmdEnv.getExecRoot().getRelative(execPath) : null;
   }
 
+  /** Returns a new {@link CommandLineBuilder} for the process wrapper tool. */
+  public static CommandLineBuilder commandLineBuilder() {
+    return new CommandLineBuilder();
+  }
+
   /**
-   * Returns a command line to execute a specific command using the process wrapper with a timeout.
-   *
-   * @param processWrapper the path to the process wrapper
-   * @param spawnArguments the command to execute, and its arguments
-   * @param timeout the time limit to run command using the process wrapper
-   * @param timeoutGraceSeconds the delay (in seconds) to kill a command that exceeds its timeout
-   *
-   * @return the constructed command line to execute a command using the process wrapper
+   * A builder class for constructing the full command line to run a command using the
+   * process-wrapper tool.
    */
-  public static List<String> getCommandLine(
-      Path processWrapper, List<String> spawnArguments, Duration timeout, int timeoutGraceSeconds) {
-    List<String> commandLineArgs = new ArrayList<>(5 + spawnArguments.size());
-    commandLineArgs.add(processWrapper.getPathString());
-    commandLineArgs.add("--timeout=" + timeout.getSeconds());
-    commandLineArgs.add("--kill_delay=" + timeoutGraceSeconds);
-    commandLineArgs.addAll(spawnArguments);
-    return commandLineArgs;
+  public static class CommandLineBuilder {
+    private Optional<String> stdoutPath;
+    private Optional<String> stderrPath;
+    private Optional<Duration> timeout;
+    private Optional<Duration> killDelay;
+    private Optional<List<String>> commandArguments;
+    private Optional<String> processWrapperPath;
+
+    private CommandLineBuilder() {
+      this.stdoutPath = Optional.empty();
+      this.stderrPath = Optional.empty();
+      this.timeout = Optional.empty();
+      this.killDelay = Optional.empty();
+      this.commandArguments = Optional.empty();
+      this.processWrapperPath = Optional.empty();
+    }
+
+    /** Sets the path to use for redirecting stdout, if any. */
+    public CommandLineBuilder setStdoutPath(String stdoutPath) {
+      this.stdoutPath = Optional.of(stdoutPath);
+      return this;
+    }
+
+    /** Sets the path to use for redirecting stderr, if any. */
+    public CommandLineBuilder setStderrPath(String stderrPath) {
+      this.stderrPath = Optional.of(stderrPath);
+      return this;
+    }
+
+    /** Sets the timeout for the command run using the process-wrapper tool. */
+    public CommandLineBuilder setTimeout(Duration timeout) {
+      this.timeout = Optional.of(timeout);
+      return this;
+    }
+
+    /**
+     * Sets the kill delay for commands run using the process-wrapper tool that exceed their
+     * timeout.
+     */
+    public CommandLineBuilder setKillDelay(Duration killDelay) {
+      this.killDelay = Optional.of(killDelay);
+      return this;
+    }
+
+    /** Sets the command (and its arguments) to run using the process wrapper tool. */
+    public CommandLineBuilder setCommandArguments(List<String> commandArguments) {
+      this.commandArguments = Optional.of(commandArguments);
+      return this;
+    }
+
+    /** Sets the path of the process wrapper tool. */
+    public CommandLineBuilder setProcessWrapperPath(String processWrapperPath) {
+      this.processWrapperPath = Optional.of(processWrapperPath);
+      return this;
+    }
+
+    /** Build the command line to invoke a specific command using the process wrapper tool. */
+    public List<String> build() {
+      Preconditions.checkState(
+          this.processWrapperPath.isPresent(), "processWrapperPath is required");
+      Preconditions.checkState(this.commandArguments.isPresent(), "commandArguments are required");
+
+      List<String> fullCommandLine = new ArrayList<>();
+      fullCommandLine.add(processWrapperPath.get());
+
+      if (timeout.isPresent()) {
+        fullCommandLine.add("--timeout=" + timeout.get().getSeconds());
+      }
+      if (killDelay.isPresent()) {
+        fullCommandLine.add("--kill_delay=" + killDelay.get().getSeconds());
+      }
+      if (stdoutPath.isPresent()) {
+        fullCommandLine.add("--stdout=" + stdoutPath.get());
+      }
+      if (stderrPath.isPresent()) {
+        fullCommandLine.add("--stderr=" + stderrPath.get());
+      }
+
+      fullCommandLine.addAll(commandArguments.get());
+
+      return fullCommandLine;
+    }
   }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/BUILD b/src/main/java/com/google/devtools/build/lib/sandbox/BUILD
index 73df336ad0..9d849a8fc1 100644
--- a/src/main/java/com/google/devtools/build/lib/sandbox/BUILD
+++ b/src/main/java/com/google/devtools/build/lib/sandbox/BUILD
@@ -25,6 +25,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/actions",
         "//src/main/java/com/google/devtools/build/lib/exec/apple",
         "//src/main/java/com/google/devtools/build/lib/exec/local",
+        "//src/main/java/com/google/devtools/build/lib/exec/local:options",
         "//src/main/java/com/google/devtools/build/lib/shell",
         "//src/main/java/com/google/devtools/build/lib/standalone",
         "//src/main/java/com/google/devtools/build/lib/vfs",
diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/DarwinSandboxedSpawnRunner.java b/src/main/java/com/google/devtools/build/lib/sandbox/DarwinSandboxedSpawnRunner.java
index 97fc677503..e554865a3b 100644
--- a/src/main/java/com/google/devtools/build/lib/sandbox/DarwinSandboxedSpawnRunner.java
+++ b/src/main/java/com/google/devtools/build/lib/sandbox/DarwinSandboxedSpawnRunner.java
@@ -224,8 +224,12 @@ final class DarwinSandboxedSpawnRunner extends AbstractSandboxSpawnRunner {
     commandLineArgs.add("-f");
     commandLineArgs.add(sandboxConfigPath.getPathString());
     commandLineArgs.addAll(
-        ProcessWrapperUtil.getCommandLine(
-            processWrapper, spawn.getArguments(), timeout, timeoutGraceSeconds));
+        ProcessWrapperUtil.commandLineBuilder()
+            .setProcessWrapperPath(processWrapper.getPathString())
+            .setCommandArguments(spawn.getArguments())
+            .setTimeout(timeout)
+            .setKillDelay(Duration.ofSeconds(timeoutGraceSeconds))
+            .build());
     return commandLineArgs;
   }
 
diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/ProcessWrapperSandboxedSpawnRunner.java b/src/main/java/com/google/devtools/build/lib/sandbox/ProcessWrapperSandboxedSpawnRunner.java
index 8ab4ddbf45..52c72b716e 100644
--- a/src/main/java/com/google/devtools/build/lib/sandbox/ProcessWrapperSandboxedSpawnRunner.java
+++ b/src/main/java/com/google/devtools/build/lib/sandbox/ProcessWrapperSandboxedSpawnRunner.java
@@ -70,8 +70,12 @@ final class ProcessWrapperSandboxedSpawnRunner extends AbstractSandboxSpawnRunne
 
     Duration timeout = policy.getTimeout();
     List<String> arguments =
-        ProcessWrapperUtil.getCommandLine(
-            processWrapper, spawn.getArguments(), timeout, timeoutGraceSeconds);
+        ProcessWrapperUtil.commandLineBuilder()
+            .setProcessWrapperPath(processWrapper.getPathString())
+            .setCommandArguments(spawn.getArguments())
+            .setTimeout(timeout)
+            .setKillDelay(Duration.ofSeconds(timeoutGraceSeconds))
+            .build();
     Map<String, String> environment =
         localEnvProvider.rewriteLocalEnv(spawn.getEnvironment(), execRoot, tmpDir, productName);
 
diff --git a/src/main/java/com/google/devtools/build/lib/standalone/BUILD b/src/main/java/com/google/devtools/build/lib/standalone/BUILD
index f98cd234db..585051d2f7 100644
--- a/src/main/java/com/google/devtools/build/lib/standalone/BUILD
+++ b/src/main/java/com/google/devtools/build/lib/standalone/BUILD
@@ -24,6 +24,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/actions",
         "//src/main/java/com/google/devtools/build/lib/exec/apple",
         "//src/main/java/com/google/devtools/build/lib/exec/local",
+        "//src/main/java/com/google/devtools/build/lib/exec/local:options",
         "//src/main/java/com/google/devtools/build/lib/rules/apple",
         "//src/main/java/com/google/devtools/build/lib/rules/cpp",
         "//src/main/java/com/google/devtools/build/lib/shell",
diff --git a/src/main/java/com/google/devtools/build/lib/worker/BUILD b/src/main/java/com/google/devtools/build/lib/worker/BUILD
index ae68799d72..68561dca15 100644
--- a/src/main/java/com/google/devtools/build/lib/worker/BUILD
+++ b/src/main/java/com/google/devtools/build/lib/worker/BUILD
@@ -23,6 +23,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/concurrent",
         "//src/main/java/com/google/devtools/build/lib/exec/apple",
         "//src/main/java/com/google/devtools/build/lib/exec/local",
+        "//src/main/java/com/google/devtools/build/lib/exec/local:options",
         "//src/main/java/com/google/devtools/build/lib/sandbox",
         "//src/main/java/com/google/devtools/build/lib/shell",
         "//src/main/java/com/google/devtools/build/lib/standalone",
diff --git a/src/test/java/com/google/devtools/build/lib/BUILD b/src/test/java/com/google/devtools/build/lib/BUILD
index 48f7f27930..8234b54917 100644
--- a/src/test/java/com/google/devtools/build/lib/BUILD
+++ b/src/test/java/com/google/devtools/build/lib/BUILD
@@ -1139,6 +1139,7 @@ java_test(
         "//src/main/java/com/google/devtools/build/lib/actions",
         "//src/main/java/com/google/devtools/build/lib/clock",
         "//src/main/java/com/google/devtools/build/lib/exec/local",
+        "//src/main/java/com/google/devtools/build/lib/exec/local:options",
         "//src/main/java/com/google/devtools/build/lib/rules/apple",
         "//src/main/java/com/google/devtools/build/lib/shell",
         "//src/main/java/com/google/devtools/build/lib/standalone",
@@ -1191,6 +1192,7 @@ java_test(
         "//src/main/java/com/google/devtools/build/lib:util",
         "//src/main/java/com/google/devtools/build/lib/actions",
         "//src/main/java/com/google/devtools/build/lib/exec/local",
+        "//src/main/java/com/google/devtools/build/lib/exec/local:options",
         "//src/main/java/com/google/devtools/build/lib/shell",
         "//src/main/java/com/google/devtools/build/lib/vfs",
         "//src/main/java/com/google/devtools/build/lib/vfs/inmemoryfs",
diff --git a/src/test/java/com/google/devtools/build/lib/runtime/ProcessWrapperUtilTest.java b/src/test/java/com/google/devtools/build/lib/runtime/ProcessWrapperUtilTest.java
new file mode 100644
index 0000000000..91735e3a22
--- /dev/null
+++ b/src/test/java/com/google/devtools/build/lib/runtime/ProcessWrapperUtilTest.java
@@ -0,0 +1,115 @@
+// Copyright 2017 The Bazel Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//    http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.devtools.build.lib.runtime;
+
+import static com.google.common.truth.Truth.assertThat;
+import static com.google.devtools.build.lib.testutil.MoreAsserts.expectThrows;
+
+import java.time.Duration;
+import java.util.ArrayList;
+import java.util.List;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.JUnit4;
+
+/** Unit tests for {@link ProcessWrapperUtil}. */
+@RunWith(JUnit4.class)
+public final class ProcessWrapperUtilTest {
+
+  @Test
+  public void testProcessWrapperCommandLineBuilder_ProcessWrapperPathIsRequired() {
+    List<String> commandArguments = new ArrayList<>();
+    commandArguments.add("echo");
+    commandArguments.add("hello, world");
+
+    Exception e =
+        expectThrows(
+            IllegalStateException.class,
+            () ->
+                ProcessWrapperUtil.commandLineBuilder()
+                    .setCommandArguments(commandArguments)
+                    .build());
+    assertThat(e).hasMessageThat().contains("processWrapperPath");
+  }
+
+  @Test
+  public void testProcessWrapperCommandLineBuilder_CommandAgumentsAreRequired() {
+    String processWrapperPath = "process-wrapper";
+
+    Exception e =
+        expectThrows(
+            IllegalStateException.class,
+            () ->
+                ProcessWrapperUtil.commandLineBuilder()
+                    .setProcessWrapperPath(processWrapperPath)
+                    .build());
+    assertThat(e).hasMessageThat().contains("commandArguments");
+  }
+
+  @Test
+  public void testProcessWrapperCommandLineBuilder_BuildsWithoutOptionalArguments() {
+    String processWrapperPath = "process-wrapper";
+
+    List<String> commandArguments = new ArrayList<>();
+    commandArguments.add("echo");
+    commandArguments.add("hello, world");
+
+    List<String> expectedCommandLine = new ArrayList<>();
+    expectedCommandLine.add(processWrapperPath);
+    expectedCommandLine.addAll(commandArguments);
+
+    List<String> commandLine =
+        ProcessWrapperUtil.commandLineBuilder()
+            .setProcessWrapperPath(processWrapperPath)
+            .setCommandArguments(commandArguments)
+            .build();
+
+    assertThat(commandLine).containsExactlyElementsIn(expectedCommandLine);
+  }
+
+  @Test
+  public void testProcessWrapperCommandLineBuilder_BuildsWithOptionalArguments() {
+    String processWrapperPath = "process-wrapper";
+
+    List<String> commandArguments = new ArrayList<>();
+    commandArguments.add("echo");
+    commandArguments.add("hello, world");
+
+    Duration timeout = Duration.ofSeconds(10);
+    Duration killDelay = Duration.ofSeconds(2);
+    String stdoutPath = "stdout.txt";
+    String stderrPath = "stderr.txt";
+
+    List<String> expectedCommandLine = new ArrayList<>();
+    expectedCommandLine.add(processWrapperPath);
+    expectedCommandLine.add("--timeout=" + timeout.getSeconds());
+    expectedCommandLine.add("--kill_delay=" + killDelay.getSeconds());
+    expectedCommandLine.add("--stdout=" + stdoutPath);
+    expectedCommandLine.add("--stderr=" + stderrPath);
+    expectedCommandLine.addAll(commandArguments);
+
+    List<String> commandLine =
+        ProcessWrapperUtil.commandLineBuilder()
+            .setProcessWrapperPath(processWrapperPath)
+            .setCommandArguments(commandArguments)
+            .setTimeout(timeout)
+            .setKillDelay(killDelay)
+            .setStdoutPath(stdoutPath)
+            .setStderrPath(stderrPath)
+            .build();
+
+    assertThat(commandLine).containsExactlyElementsIn(expectedCommandLine);
+  }
+}