Add a "canExec" method to SpawnActionContext and SpawnRunner.

This is used by the ProxySpawnActionContext to determine at runtime
whether a spawn strategy can execute a given spawn.

Adds the flag --incompatible_list_based_execution_strategy_selection,
which is used to make this an opt-in change that will be managed by the
incompatible flags process.

The flag's GitHub issue is here:
#7480

RELNOTES[INC]: The flag --incompatible_list_based_execution_strategy_selection
was added and is used to ease the migration to the new style of specifying
execution strategy selection and fallback behavior. The documentation for
this flag is here: #7480

PiperOrigin-RevId: 234877574

src/main/java/com/google/devtools/build/lib/actions/SpawnActionContext.java

@@ -35,4 +35,7 @@ default FutureSpawn execMaybeAsync(Spawn spawn, ActionExecutionContext actionExe
     SpawnResult result = Iterables.getOnlyElement(exec(spawn, actionExecutionContext));
     return FutureSpawn.immediate(result);
   }
+
+  /** Returns whether this SpawnActionContext supports executing the given Spawn. */
+  boolean canExec(Spawn spawn);
 }

src/main/java/com/google/devtools/build/lib/buildtool/ExecutionTool.java

@@ -163,9 +163,14 @@ public class ExecutionTool {
     // independently from each other, for example, to run genrules locally and Java compile action
     // in prod. Thus, for SpawnActions, we decide the action context to use not only based on the
     // context class, but also the mnemonic of the action.
+    ExecutionOptions options = request.getOptions(ExecutionOptions.class);
     spawnActionContextMaps =
-        builder.getSpawnActionContextMapsBuilder().build(
-            actionContextProviders, request.getOptions(ExecutionOptions.class).testStrategy);
+        builder
+            .getSpawnActionContextMapsBuilder()
+            .build(
+                actionContextProviders,
+                options.testStrategy,
+                options.incompatibleListBasedExecutionStrategySelection);
   }
 
   Executor getExecutor() throws ExecutorInitException {

src/main/java/com/google/devtools/build/lib/dynamic/DynamicSpawnStrategy.java

@@ -316,6 +316,13 @@ public List<SpawnResult> callImpl() throws InterruptedException, ExecException {
     return dynamicExecutionResult.spawnResults();
   }
 
+  @Override
+  public boolean canExec(Spawn spawn) {
+    return remoteStrategy.canExec(spawn)
+        || workerStrategy.canExec(spawn)
+        || localStrategy.canExec(spawn);
+  }
+
   private void moveFileOutErr(ActionExecutionContext actionExecutionContext, FileOutErr outErr)
       throws IOException {
     if (outErr.getOutputPath().exists()) {

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

@@ -76,6 +76,11 @@ public List<SpawnResult> exec(Spawn spawn, ActionExecutionContext actionExecutio
     return exec(spawn, actionExecutionContext, null);
   }
 
+  @Override
+  public boolean canExec(Spawn spawn) {
+    return spawnRunner.canExec(spawn);
+  }
+
   @Override
   public List<SpawnResult> exec(
       Spawn spawn,

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

@@ -26,10 +26,10 @@
 import com.google.devtools.common.options.BoolOrEnumConverter;
 import com.google.devtools.common.options.Converters.AssignmentToListOfValuesConverter;
 import com.google.devtools.common.options.Converters.CommaSeparatedNonEmptyOptionListConverter;
-import com.google.devtools.common.options.Converters.CommaSeparatedOptionListConverter;
 import com.google.devtools.common.options.Option;
 import com.google.devtools.common.options.OptionDocumentationCategory;
 import com.google.devtools.common.options.OptionEffectTag;
+import com.google.devtools.common.options.OptionMetadataTag;
 import com.google.devtools.common.options.Options;
 import com.google.devtools.common.options.OptionsBase;
 import com.google.devtools.common.options.OptionsParsingException;
@@ -57,6 +57,18 @@ public class ExecutionOptions extends OptionsBase {
 
   public static final ExecutionOptions DEFAULTS = Options.getDefaults(ExecutionOptions.class);
 
+  @Option(
+      name = "incompatible_list_based_execution_strategy_selection",
+      defaultValue = "false",
+      documentationCategory = OptionDocumentationCategory.EXECUTION_STRATEGY,
+      effectTags = {OptionEffectTag.EXECUTION},
+      metadataTags = {
+        OptionMetadataTag.INCOMPATIBLE_CHANGE,
+        OptionMetadataTag.TRIGGERED_BY_ALL_INCOMPATIBLE_CHANGES
+      },
+      help = "See https://github.com/bazelbuild/bazel/issues/7480")
+  public boolean incompatibleListBasedExecutionStrategySelection;
+
   @Option(
       name = "spawn_strategy",
       defaultValue = "",
@@ -73,7 +85,7 @@ public class ExecutionOptions extends OptionsBase {
   @Option(
       name = "genrule_strategy",
       defaultValue = "",
-      converter = CommaSeparatedOptionListConverter.class,
+      converter = CommaSeparatedNonEmptyOptionListConverter.class,
       documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
       effectTags = {OptionEffectTag.UNKNOWN},
       help =

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

@@ -22,30 +22,37 @@
 import com.google.devtools.build.lib.actions.SpawnResult;
 import com.google.devtools.build.lib.actions.UserExecException;
 import com.google.devtools.build.lib.events.EventHandler;
+import com.google.devtools.build.lib.events.NullEventHandler;
 import java.util.List;
+import java.util.stream.Collectors;
 
 /** Proxy that looks up the right SpawnActionContext for a spawn during {@link #exec}. */
 public final class ProxySpawnActionContext implements SpawnActionContext {
 
   private final SpawnActionContextMaps spawnActionContextMaps;
+  private final boolean listBasedExecutionStrategySelection;
 
   /**
    * Creates a new {@link ProxySpawnActionContext}.
    *
    * @param spawnActionContextMaps The {@link SpawnActionContextMaps} to use to decide which {@link
    *     SpawnActionContext} should execute a given {@link Spawn} during {@link #exec}.
+   * @param listBasedExecutionStrategySelection
    */
-  public ProxySpawnActionContext(SpawnActionContextMaps spawnActionContextMaps) {
+  public ProxySpawnActionContext(
+      SpawnActionContextMaps spawnActionContextMaps, boolean listBasedExecutionStrategySelection) {
     this.spawnActionContextMaps = spawnActionContextMaps;
+    this.listBasedExecutionStrategySelection = listBasedExecutionStrategySelection;
   }
 
   @Override
   public List<SpawnResult> exec(Spawn spawn, ActionExecutionContext actionExecutionContext)
       throws ExecException, InterruptedException {
     List<SpawnActionContext> strategies = resolve(spawn, actionExecutionContext.getEventHandler());
 
-    // For now, we only support executing with the first strategy in the list. Later versions of
-    // this code will add some smartness to pick the best out of the list.
+    // Because the strategies are ordered by preference, we can execute the spawn with the best
+    // possible one by simply filtering out the ones that can't execute it and then picking the
+    // first one from the remaining strategies in the list.
     return strategies.get(0).exec(spawn, actionExecutionContext);
   }
 
@@ -54,8 +61,9 @@ public FutureSpawn execMaybeAsync(Spawn spawn, ActionExecutionContext actionExec
       throws ExecException, InterruptedException {
     List<SpawnActionContext> strategies = resolve(spawn, actionExecutionContext.getEventHandler());
 
-    // For now, we only support executing with the first strategy in the list. Later versions of
-    // this code will add some smartness to pick the best out of the list.
+    // Because the strategies are ordered by preference, we can execute the spawn with the best
+    // possible one by simply filtering out the ones that can't execute it and then picking the
+    // first one from the remaining strategies in the list.
     return strategies.get(0).execMaybeAsync(spawn, actionExecutionContext);
   }
 
@@ -72,6 +80,13 @@ public List<SpawnActionContext> resolve(Spawn spawn, EventHandler eventHandler)
     List<SpawnActionContext> strategies =
         spawnActionContextMaps.getSpawnActionContexts(spawn, eventHandler);
 
+    if (listBasedExecutionStrategySelection) {
+      strategies =
+          strategies.stream()
+              .filter(spawnActionContext -> spawnActionContext.canExec(spawn))
+              .collect(Collectors.toList());
+    }
+
     if (strategies.isEmpty()) {
       throw new UserExecException(
           String.format(
@@ -82,4 +97,10 @@ public List<SpawnActionContext> resolve(Spawn spawn, EventHandler eventHandler)
 
     return strategies;
   }
+
+  @Override
+  public boolean canExec(Spawn spawn) {
+    return spawnActionContextMaps.getSpawnActionContexts(spawn, NullEventHandler.INSTANCE).stream()
+        .anyMatch(spawnActionContext -> spawnActionContext.canExec(spawn));
+  }
 }

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

@@ -14,6 +14,7 @@
 package com.google.devtools.build.lib.exec;
 
 import com.google.common.base.Splitter;
+import com.google.common.collect.ImmutableList;
 import com.google.common.collect.Maps;
 import com.google.devtools.build.lib.util.RegexFilter;
 import com.google.devtools.build.lib.util.RegexFilter.RegexFilterConverter;
@@ -36,6 +37,16 @@ public Map.Entry<RegexFilter, List<String>> convert(String input) throws Options
           "Must be in the form of a 'regex=value[,value]' assignment");
     }
     List<String> value = splitter.splitToList(input.substring(pos + 1));
+    if (value.contains("")) {
+      // If the list contains exactly the empty string, it means an empty value was passed and we
+      // should instead return an empty list.
+      if (value.size() == 1) {
+        value = ImmutableList.of();
+      } else {
+        throw new OptionsParsingException(
+            "Values must not contain empty strings or leading / trailing commas");
+      }
+    }
     RegexFilter filter = new RegexFilterConverter().convert(input.substring(0, pos));
     return Maps.immutableEntry(filter, value);
   }

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

@@ -73,14 +73,17 @@ public abstract static class RegexFilterSpawnActionContext {
   private final ImmutableSortedMap<String, List<SpawnActionContext>> mnemonicToSpawnStrategiesMap;
   private final ImmutableList<ActionContext> strategies;
   private final ImmutableList<RegexFilterSpawnActionContext> spawnStrategyRegexList;
+  private final boolean listBasedExecutionStrategySelection;
 
   private SpawnActionContextMaps(
       ImmutableSortedMap<String, List<SpawnActionContext>> mnemonicToSpawnStrategiesMap,
       ImmutableList<ActionContext> strategies,
-      ImmutableList<RegexFilterSpawnActionContext> spawnStrategyRegexList) {
+      ImmutableList<RegexFilterSpawnActionContext> spawnStrategyRegexList,
+      boolean listBasedExecutionStrategySelection) {
     this.mnemonicToSpawnStrategiesMap = mnemonicToSpawnStrategiesMap;
     this.strategies = strategies;
     this.spawnStrategyRegexList = spawnStrategyRegexList;
+    this.listBasedExecutionStrategySelection = listBasedExecutionStrategySelection;
   }
 
   /**
@@ -121,7 +124,9 @@ ImmutableMap<Class<? extends ActionContext>, ActionContext> contextMap() {
       }
       contextMap.put(context.getClass(), context);
     }
-    contextMap.put(SpawnActionContext.class, new ProxySpawnActionContext(this));
+    contextMap.put(
+        SpawnActionContext.class,
+        new ProxySpawnActionContext(this, listBasedExecutionStrategySelection));
     return ImmutableMap.copyOf(contextMap);
   }
 
@@ -185,7 +190,8 @@ public static SpawnActionContextMaps createStub(
     return new SpawnActionContextMaps(
         ImmutableSortedMap.copyOf(spawnStrategyMnemonicMap, String.CASE_INSENSITIVE_ORDER),
         ImmutableList.copyOf(strategies),
-        ImmutableList.of());
+        ImmutableList.of(),
+        false);
   }
 
   /** A stored entry for a {@link RegexFilter} to {@code strategy} mapping. */
@@ -238,7 +244,9 @@ public void addStrategyByRegexp(RegexFilter regexFilter, List<String> strategy)
 
     /** Builds a {@link SpawnActionContextMaps} instance. */
     public SpawnActionContextMaps build(
-        ImmutableList<ActionContextProvider> actionContextProviders, String testStrategyValue)
+        ImmutableList<ActionContextProvider> actionContextProviders,
+        String testStrategyValue,
+        boolean listBasedExecutionStrategySelection)
         throws ExecutorInitException {
       StrategyConverter strategyConverter = new StrategyConverter(actionContextProviders);
 
@@ -250,7 +258,19 @@ public SpawnActionContextMaps build(
 
       for (String mnemonic : strategyByMnemonicMap.keySet()) {
         ImmutableList.Builder<SpawnActionContext> contexts = ImmutableList.builder();
-        for (String strategy : strategyByMnemonicMap.get(mnemonic)) {
+        Set<String> strategiesForMnemonic = strategyByMnemonicMap.get(mnemonic);
+        if (strategiesForMnemonic.size() > 1 && !listBasedExecutionStrategySelection) {
+          String flagName =
+              mnemonic.isEmpty() ? "--spawn_strategy=" : ("--strategy=" + mnemonic + "=");
+          throw new ExecutorInitException(
+              "Flag "
+                  + flagName
+                  + Joiner.on(',').join(strategiesForMnemonic)
+                  + " contains a list of strategies to use, but "
+                  + "--incompatible_list_based_execution_strategy_selection was not enabled.",
+              ExitCode.COMMAND_LINE_ERROR);
+        }
+        for (String strategy : strategiesForMnemonic) {
           SpawnActionContext context =
               strategyConverter.getStrategy(SpawnActionContext.class, strategy);
           if (context == null) {
@@ -284,7 +304,18 @@ public SpawnActionContextMaps build(
 
       for (RegexFilterStrategy entry : strategyByRegexpBuilder.build()) {
         ImmutableList.Builder<SpawnActionContext> contexts = ImmutableList.builder();
-        for (String strategy : entry.strategy()) {
+        List<String> strategiesForRegex = entry.strategy();
+        if (strategiesForRegex.size() > 1 && !listBasedExecutionStrategySelection) {
+          throw new ExecutorInitException(
+              "Flag --strategy_regexp="
+                  + entry.regexFilter().toString()
+                  + "="
+                  + Joiner.on(',').join(strategiesForRegex)
+                  + " contains a list of strategies to use, but "
+                  + "--incompatible_list_based_execution_strategy_selection was not enabled.",
+              ExitCode.COMMAND_LINE_ERROR);
+        }
+        for (String strategy : strategiesForRegex) {
           SpawnActionContext context =
               strategyConverter.getStrategy(SpawnActionContext.class, strategy);
           if (context == null) {
@@ -310,7 +341,10 @@ public SpawnActionContextMaps build(
       strategies.add(context);
 
       return new SpawnActionContextMaps(
-          spawnStrategyMap.build(), strategies.build(), spawnStrategyRegexList.build());
+          spawnStrategyMap.build(),
+          strategies.build(),
+          spawnStrategyRegexList.build(),
+          listBasedExecutionStrategySelection);
     }
   }
 

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

@@ -232,6 +232,9 @@ default FutureSpawn execAsync(Spawn spawn, SpawnExecutionContext context)
   SpawnResult exec(Spawn spawn, SpawnExecutionContext context)
       throws InterruptedException, IOException, ExecException;
 
+  /** Returns whether this SpawnRunner supports executing the given Spawn. */
+  boolean canExec(Spawn spawn);
+
   /* Name of the SpawnRunner. */
   String getName();
 }

src/main/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunner.java

@@ -154,6 +154,11 @@ public SpawnResult exec(Spawn spawn, SpawnExecutionContext context)
     }
   }
 
+  @Override
+  public boolean canExec(Spawn spawn) {
+    return true;
+  }
+
   protected Path createActionTemp(Path execRoot) throws IOException {
     return execRoot.getRelative(
         java.nio.file.Files.createTempDirectory(

src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java

@@ -279,6 +279,11 @@ public SpawnResult exec(Spawn spawn, SpawnExecutionContext context)
     }
   }
 
+  @Override
+  public boolean canExec(Spawn spawn) {
+    return Spawns.mayBeExecutedRemotely(spawn);
+  }
+
   private void maybeWriteParamFilesLocally(Spawn spawn) throws IOException {
     if (!executionOptions.materializeParamFiles) {
       return;

src/main/java/com/google/devtools/build/lib/sandbox/AbstractSandboxSpawnRunner.java

@@ -25,6 +25,7 @@
 import com.google.devtools.build.lib.actions.Spawn;
 import com.google.devtools.build.lib.actions.SpawnResult;
 import com.google.devtools.build.lib.actions.SpawnResult.Status;
+import com.google.devtools.build.lib.actions.Spawns;
 import com.google.devtools.build.lib.actions.UserExecException;
 import com.google.devtools.build.lib.exec.BinTools;
 import com.google.devtools.build.lib.exec.ExecutionOptions;
@@ -78,6 +79,11 @@ public SpawnResult exec(Spawn spawn, SpawnExecutionContext context)
     }
   }
 
+  @Override
+  public boolean canExec(Spawn spawn) {
+    return Spawns.mayBeSandboxed(spawn);
+  }
+
   // TODO(laszlocsomor): refactor this class to make `actuallyExec`'s contract clearer: the caller
   // of `actuallyExec` should not depend on `actuallyExec` calling `runSpawn` because it's easy to
   // forget to do so in `actuallyExec`'s implementations.

src/main/java/com/google/devtools/build/lib/sandbox/SandboxModule.java

@@ -26,7 +26,6 @@
 import com.google.devtools.build.lib.actions.Spawn;
 import com.google.devtools.build.lib.actions.SpawnActionContext;
 import com.google.devtools.build.lib.actions.SpawnResult;
-import com.google.devtools.build.lib.actions.Spawns;
 import com.google.devtools.build.lib.buildtool.BuildRequest;
 import com.google.devtools.build.lib.buildtool.buildevent.BuildCompleteEvent;
 import com.google.devtools.build.lib.buildtool.buildevent.BuildInterruptedEvent;
@@ -307,12 +306,17 @@ public String getName() {
     @Override
     public SpawnResult exec(Spawn spawn, SpawnExecutionContext context)
         throws InterruptedException, IOException, ExecException {
-      if (!Spawns.mayBeSandboxed(spawn)) {
-        return fallbackSpawnRunner.exec(spawn, context);
-      } else {
+      if (sandboxSpawnRunner.canExec(spawn)) {
         return sandboxSpawnRunner.exec(spawn, context);
+      } else {
+        return fallbackSpawnRunner.exec(spawn, context);
       }
     }
+
+    @Override
+    public boolean canExec(Spawn spawn) {
+      return sandboxSpawnRunner.canExec(spawn) || fallbackSpawnRunner.canExec(spawn);
+    }
   }
 
   /**

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

@@ -121,6 +121,11 @@ public SpawnResult exec(Spawn spawn, SpawnExecutionContext context)
     return actuallyExec(spawn, context);
   }
 
+  @Override
+  public boolean canExec(Spawn spawn) {
+    return Spawns.supportsWorkers(spawn);
+  }
+
   private SpawnResult actuallyExec(Spawn spawn, SpawnExecutionContext context)
       throws ExecException, IOException, InterruptedException {
     if (Iterables.isEmpty(spawn.getToolFiles())) {