Refactor persistent workers to use SpawnRunner.

Change the persistent worker spawn strategy to extend
AbstractSpawnStrategy and put the actual logic into
WorkerSpawnRunner. WorkerTestStrategy is unaffected.

I had to extend SpawnPolicy with a speculating() method. Persistent
workers need to know if speculation is happening in order to require
sandboxing.

Additionally, I added java_test rules for the local runner tests and
worker tests. See bazelbuild#3481.

NOTE: ulfjack@ made some changes to this change before merging:
 - changed Reporter to EventHandler; added TODO about its usage
 - reverted non-semantic indentation change in AbstractSpawnStrategy
 - reverted a non-semantic indentation change in WorkerSpawnRunner
 - updated some internal classes to match
 - removed catch IOException in WorkerSpawnRunner in some cases,
   removed verboseFailures flag from WorkerSpawnRunner, updated callers
 - disable some tests on Windows; we were previously not running them,
   now that we do, they fail :-(

Change-Id: I207b3938f0dc84d374ab052d5030020886451d47
PiperOrigin-RevId: 164965398

src/main/java/com/google/devtools/build/lib/exec/AbstractSpawnStrategy.java

@@ -149,6 +149,11 @@ public void lockOutputFiles() throws InterruptedException {
       }
     }
 
+    @Override
+    public boolean speculating() {
+      return writeOutputFiles != null;
+    }
+
     @Override
     public Duration getTimeout() {
       return timeout;

src/main/java/com/google/devtools/build/lib/exec/SpawnRunner.java

@@ -169,6 +169,12 @@ public interface SpawnExecutionPolicy {
      */
     void lockOutputFiles() throws InterruptedException;
 
+    /**
+     * Returns whether this spawn may be executing concurrently under multiple spawn runners. If so,
+     * {@link #lockOutputFiles} may raise {@link InterruptedException}.
+     */
+    boolean speculating();
+
     /** Returns the timeout that should be applied for the given {@link Spawn} instance. */
     Duration getTimeout();
 

src/main/java/com/google/devtools/build/lib/worker/BUILD

@@ -15,6 +15,8 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib:util",
         "//src/main/java/com/google/devtools/build/lib:vfs",
         "//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/sandbox",
         "//src/main/java/com/google/devtools/build/lib/standalone",
         "//src/main/java/com/google/devtools/common/options",

src/main/java/com/google/devtools/build/lib/worker/WorkerActionContextProvider.java

@@ -16,34 +16,54 @@
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMultimap;
 import com.google.devtools.build.lib.actions.ActionContext;
+import com.google.devtools.build.lib.actions.ResourceManager;
 import com.google.devtools.build.lib.analysis.test.TestActionContext;
-import com.google.devtools.build.lib.buildtool.BuildRequest;
 import com.google.devtools.build.lib.exec.ActionContextProvider;
-import com.google.devtools.build.lib.exec.ExecutionOptions;
+import com.google.devtools.build.lib.exec.SpawnRunner;
+import com.google.devtools.build.lib.exec.apple.XCodeLocalEnvProvider;
+import com.google.devtools.build.lib.exec.local.LocalEnvProvider;
+import com.google.devtools.build.lib.exec.local.LocalExecutionOptions;
+import com.google.devtools.build.lib.exec.local.LocalSpawnRunner;
 import com.google.devtools.build.lib.runtime.CommandEnvironment;
+import com.google.devtools.build.lib.util.OS;
 
 /**
  * Factory for the Worker-based execution strategy.
  */
 final class WorkerActionContextProvider extends ActionContextProvider {
   private final ImmutableList<ActionContext> strategies;
 
-  public WorkerActionContextProvider(
-      CommandEnvironment env, BuildRequest buildRequest, WorkerPool workers) {
-    ImmutableMultimap.Builder<String, String> extraFlags = ImmutableMultimap.builder();
-    extraFlags.putAll(buildRequest.getOptions(WorkerOptions.class).workerExtraFlags);
+  public WorkerActionContextProvider(CommandEnvironment env, WorkerPool workers) {
+    ImmutableMultimap<String, String> extraFlags =
+        ImmutableMultimap.copyOf(env.getOptions().getOptions(WorkerOptions.class).workerExtraFlags);
 
-    WorkerSpawnStrategy workerSpawnStrategy =
-        new WorkerSpawnStrategy(
+    WorkerSpawnRunner spawnRunner =
+        new WorkerSpawnRunner(
             env.getExecRoot(),
             workers,
-            buildRequest.getOptions(ExecutionOptions.class).verboseFailures,
-            extraFlags.build());
+            extraFlags,
+            env.getReporter(),
+            createFallbackRunner(env));
+
+    WorkerSpawnStrategy workerSpawnStrategy = new WorkerSpawnStrategy(spawnRunner);
     TestActionContext workerTestStrategy =
-        new WorkerTestStrategy(env, buildRequest, workers, extraFlags.build());
+        new WorkerTestStrategy(env, env.getOptions(), workers, extraFlags);
     this.strategies = ImmutableList.of(workerSpawnStrategy, workerTestStrategy);
   }
 
+  private static SpawnRunner createFallbackRunner(CommandEnvironment env) {
+    LocalExecutionOptions localExecutionOptions =
+        env.getOptions().getOptions(LocalExecutionOptions.class);
+    LocalEnvProvider localEnvProvider =
+        OS.getCurrent() == OS.DARWIN ? new XCodeLocalEnvProvider() : LocalEnvProvider.UNMODIFIED;
+    return new LocalSpawnRunner(
+        env.getExecRoot(),
+        localExecutionOptions,
+        ResourceManager.instance(),
+        env.getRuntime().getProductName(),
+        localEnvProvider);
+  }
+
   @Override
   public Iterable<? extends ActionContext> getActionContexts() {
     return strategies;

src/main/java/com/google/devtools/build/lib/worker/WorkerFilesHash.java

@@ -17,8 +17,8 @@
 import com.google.common.hash.HashCode;
 import com.google.common.hash.Hasher;
 import com.google.common.hash.Hashing;
-import com.google.devtools.build.lib.actions.ActionExecutionContext;
 import com.google.devtools.build.lib.actions.ActionInput;
+import com.google.devtools.build.lib.actions.ActionInputFileCache;
 import java.io.IOException;
 import java.nio.charset.Charset;
 
@@ -29,13 +29,12 @@
 public class WorkerFilesHash {
 
   public static HashCode getWorkerFilesHash(
-      Iterable<? extends ActionInput> toolFiles, ActionExecutionContext actionExecutionContext)
+      Iterable<? extends ActionInput> toolFiles, ActionInputFileCache actionInputFileCache)
       throws IOException {
     Hasher hasher = Hashing.sha256().newHasher();
     for (ActionInput tool : toolFiles) {
       hasher.putString(tool.getExecPathString(), Charset.defaultCharset());
-      hasher.putBytes(
-          actionExecutionContext.getActionInputFileCache().getMetadata(tool).getDigest());
+      hasher.putBytes(actionInputFileCache.getMetadata(tool).getDigest());
     }
     return hasher.hash();
   }

src/main/java/com/google/devtools/build/lib/worker/WorkerModule.java

@@ -143,8 +143,7 @@ private WorkerPoolConfig createWorkerPoolConfig(WorkerOptions options) {
   @Override
   public void executorInit(CommandEnvironment env, BuildRequest request, ExecutorBuilder builder) {
     Preconditions.checkNotNull(workerPool);
-    builder.addActionContextProvider(
-        new WorkerActionContextProvider(env, request, workerPool));
+    builder.addActionContextProvider(new WorkerActionContextProvider(env, workerPool));
     builder.addActionContextConsumer(new WorkerActionContextConsumer());
   }