Rewrite the Command API

Important: the simplified API now defaults to forwarding interrupts to
subprocesses. I did audit all the call sites, and I think this is a safe change
to make.

- Properly support timeouts with all implementations
- Simplify the API
  - only provide two flavours of blocking calls, which require no input and
    forward interrupts; this is the most common usage
  - provide a number of async calls, which optionally takes input, and a flag
    whether to forward interrupts
  - only support input streams, no byte arrays or other 'convenience features'
    that are rarely needed and unnecessarily increase the surface area
  - use java.time.Duration to specify timeout; for consistency, interpret a
    timeout of <= 0 as no timeout (i.e., including rather than excluding 0)
  - KillableObserver and subclasses are no longer part of the public API, but
    still used to implement timeouts if the Subprocess.Factory does not support
    them
- Update the documentation for Command
- Update all callers; most callers now use the simplified API

PiperOrigin-RevId: 164716782

2 files changed, 14 insertions, 10 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/exec/apple/XCodeLocalEnvProvider.java b/src/main/java/com/google/devtools/build/lib/exec/apple/XCodeLocalEnvProvider.java
index 8744ef0186..57f1082978 100644
--- a/src/main/java/com/google/devtools/build/lib/exec/apple/XCodeLocalEnvProvider.java
+++ b/src/main/java/com/google/devtools/build/lib/exec/apple/XCodeLocalEnvProvider.java
@@ -115,9 +115,10 @@ public final class XCodeLocalEnvProvider implements LocalEnvProvider {
       } else {
         Map<String, String> env = Strings.isNullOrEmpty(developerDir)
             ? ImmutableMap.<String, String>of() : ImmutableMap.of("DEVELOPER_DIR", developerDir);
-        CommandResult xcrunResult = new Command(
-            new String[] {"/usr/bin/xcrun", "--sdk", sdkString, "--show-sdk-path"},
-            env, null).execute();
+        CommandResult xcrunResult =
+            new Command(
+                new String[] {"/usr/bin/xcrun", "--sdk", sdkString, "--show-sdk-path"}, env, null)
+            .execute();
 
         // calling xcrun via Command returns a value with a newline on the end.
         String sdkRoot = new String(xcrunResult.getStdout(), StandardCharsets.UTF_8).trim();
@@ -178,8 +179,9 @@ public final class XCodeLocalEnvProvider implements LocalEnvProvider {
       if (cacheResult != null) {
         return cacheResult;
       } else {
-        CommandResult xcodeLocatorResult = new Command(new String[] {
-            execRoot.getRelative("_bin/xcode-locator").getPathString(), version.toString()})
+        CommandResult xcodeLocatorResult = new Command(
+            new String[] {
+                execRoot.getRelative("_bin/xcode-locator").getPathString(), version.toString()})
             .execute();
 
         String developerDir =
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 b2c9d3c965..548b3b4091 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
@@ -241,6 +241,11 @@ public final class LocalSpawnRunner implements SpawnRunner {
       OutputStream stdOut = ByteStreams.nullOutputStream();
       OutputStream stdErr = ByteStreams.nullOutputStream();
       if (useProcessWrapper) {
+        // If the process wrapper is enabled, we use its timeout feature, which first interrupts the
+        // subprocess and only kills it after a grace period so that the subprocess can output 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());
@@ -259,16 +264,13 @@ public final class LocalSpawnRunner implements SpawnRunner {
             spawn.getArguments().toArray(new String[0]),
             localEnvProvider.rewriteLocalEnv(spawn.getEnvironment(), execRoot, productName),
             execRoot.getPathFile(),
-            // TODO(ulfjack): Command throws if timeouts are unsupported and timeout >= 0. For
-            // consistency, we should change it to not throw (and not enforce a timeout) if
-            // timeout <= 0 instead.
-            policy.getTimeout().isZero() ? -1 : policy.getTimeout().toMillis());
+            policy.getTimeout());
       }
 
       long startTime = System.currentTimeMillis();
       CommandResult result;
       try {
-        result = cmd.execute(Command.NO_INPUT, Command.NO_OBSERVER, stdOut, stdErr, true);
+        result = cmd.execute(stdOut, stdErr);
         if (Thread.currentThread().isInterrupted()) {
           throw new InterruptedException();
         }