sandbox: Use process-wrapper in addition to sandbox-exec on macOS.

This gives us much improved process management, because Bazel can now
reliably kill child processes of spawns via their process group and wait
for them to exit.

Change-Id: Ib3cb20725b3c569aa5b317a69d7682f5774707b0
PiperOrigin-RevId: 155493511

4 files changed, 15 insertions, 18 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/DarwinSandboxRunner.java b/src/main/java/com/google/devtools/build/lib/sandbox/DarwinSandboxRunner.java
index 33de83eddf..0e12fe869b 100644
--- a/src/main/java/com/google/devtools/build/lib/sandbox/DarwinSandboxRunner.java
+++ b/src/main/java/com/google/devtools/build/lib/sandbox/DarwinSandboxRunner.java
@@ -21,8 +21,6 @@ import com.google.common.io.ByteStreams;
 import com.google.devtools.build.lib.runtime.CommandEnvironment;
 import com.google.devtools.build.lib.shell.Command;
 import com.google.devtools.build.lib.shell.CommandException;
-import com.google.devtools.build.lib.shell.KillableObserver;
-import com.google.devtools.build.lib.shell.TimeoutKillableObserver;
 import com.google.devtools.build.lib.vfs.Path;
 import java.io.BufferedWriter;
 import java.io.File;
@@ -56,7 +54,11 @@ final class DarwinSandboxRunner extends SandboxRunner {
     this.writableDirs = writableDirs;
   }
 
-  static boolean isSupported() {
+  static boolean isSupported(CommandEnvironment cmdEnv) {
+    if (!ProcessWrapperRunner.isSupported(cmdEnv)) {
+      return false;
+    }
+
     List<String> args = new ArrayList<>();
     args.add(SANDBOX_EXEC);
     args.add("-p");
@@ -97,7 +99,7 @@ final class DarwinSandboxRunner extends SandboxRunner {
     commandLineArgs.add(SANDBOX_EXEC);
     commandLineArgs.add("-f");
     commandLineArgs.add(sandboxConfigPath.getPathString());
-    commandLineArgs.addAll(arguments);
+    commandLineArgs.addAll(ProcessWrapperRunner.getCommandLine(cmdEnv, arguments, timeout));
     return new Command(commandLineArgs.toArray(new String[0]), env, sandboxExecRoot.getPathFile());
   }
 
@@ -135,14 +137,4 @@ final class DarwinSandboxRunner extends SandboxRunner {
       out.println("(allow file-write* (subpath \"" + resolvedPath.getPathString() + "\"))");
     }
   }
-
-  @Override
-  protected KillableObserver getCommandObserver(int timeout) {
-    return (timeout >= 0) ? new TimeoutKillableObserver(timeout * 1000) : Command.NO_OBSERVER;
-  }
-
-  @Override
-  protected int getSignalOnTimeout() {
-    return 15; /* SIGTERM */
-  }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/ProcessWrapperRunner.java b/src/main/java/com/google/devtools/build/lib/sandbox/ProcessWrapperRunner.java
index be00833cf8..71ceebac73 100644
--- a/src/main/java/com/google/devtools/build/lib/sandbox/ProcessWrapperRunner.java
+++ b/src/main/java/com/google/devtools/build/lib/sandbox/ProcessWrapperRunner.java
@@ -58,6 +58,12 @@ final class ProcessWrapperRunner extends SandboxRunner {
       boolean allowNetwork,
       boolean useFakeHostname,
       boolean useFakeUsername) {
+    List<String> commandLineArgs = getCommandLine(cmdEnv, spawnArguments, timeout);
+    return new Command(commandLineArgs.toArray(new String[0]), env, sandboxExecRoot.getPathFile());
+  }
+
+  static List<String> getCommandLine(
+      CommandEnvironment cmdEnv, List<String> spawnArguments, int timeout) {
     List<String> commandLineArgs = new ArrayList<>(5 + spawnArguments.size());
     commandLineArgs.add(getProcessWrapper(cmdEnv).getPathString());
     commandLineArgs.add(Integer.toString(timeout));
@@ -65,7 +71,6 @@ final class ProcessWrapperRunner extends SandboxRunner {
     commandLineArgs.add("-"); /* stdout. */
     commandLineArgs.add("-"); /* stderr. */
     commandLineArgs.addAll(spawnArguments);
-
-    return new Command(commandLineArgs.toArray(new String[0]), env, sandboxExecRoot.getPathFile());
+    return commandLineArgs;
   }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/SandboxActionContextConsumer.java b/src/main/java/com/google/devtools/build/lib/sandbox/SandboxActionContextConsumer.java
index 3c51a4718a..53523e8755 100644
--- a/src/main/java/com/google/devtools/build/lib/sandbox/SandboxActionContextConsumer.java
+++ b/src/main/java/com/google/devtools/build/lib/sandbox/SandboxActionContextConsumer.java
@@ -38,7 +38,7 @@ final class SandboxActionContextConsumer implements ActionContextConsumer {
     ImmutableMap.Builder<String, String> spawnContexts = ImmutableMap.builder();
 
     if ((OS.getCurrent() == OS.LINUX && LinuxSandboxedStrategy.isSupported(cmdEnv))
-        || (OS.getCurrent() == OS.DARWIN && DarwinSandboxRunner.isSupported())
+        || (OS.getCurrent() == OS.DARWIN && DarwinSandboxRunner.isSupported(cmdEnv))
         || (OS.isPosixCompatible() && ProcessWrapperSandboxedStrategy.isSupported(cmdEnv))) {
       // This makes the "sandboxed" strategy available via --spawn_strategy=sandboxed,
       // but it is not necessarily the default.
diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/SandboxActionContextProvider.java b/src/main/java/com/google/devtools/build/lib/sandbox/SandboxActionContextProvider.java
index b757ddbaab..9d130c083d 100644
--- a/src/main/java/com/google/devtools/build/lib/sandbox/SandboxActionContextProvider.java
+++ b/src/main/java/com/google/devtools/build/lib/sandbox/SandboxActionContextProvider.java
@@ -58,7 +58,7 @@ final class SandboxActionContextProvider extends ActionContextProvider {
         }
         break;
       case DARWIN:
-        if (DarwinSandboxRunner.isSupported()) {
+        if (DarwinSandboxRunner.isSupported(cmdEnv)) {
           contexts.add(
               DarwinSandboxedStrategy.create(
                   cmdEnv, buildRequest, sandboxBase, verboseFailures, productName));