Make strategy matching stricter, remove wrong entries in returned map from StandaloneContextConsumer.

--
MOS_MIGRATED_REVID=96300473

3 files changed, 65 insertions, 33 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRulesModule.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRulesModule.java
index c5effa0bed..0bf594b8a3 100644
--- a/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRulesModule.java
+++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRulesModule.java
@@ -61,7 +61,7 @@ public class BazelRulesModule extends BlazeModule {
 
     @Option(
         name = "genrule_strategy",
-        defaultValue = "standalone", 
+        defaultValue = "standalone",
         category = "strategy",
         help = "Specify how to execute genrules."
             + "'standalone' means run all of them locally."
@@ -111,10 +111,11 @@ public class BazelRulesModule extends BlazeModule {
 
     @Override
     public Map<Class<? extends ActionContext>, String> getActionContexts() {
-      return ImmutableMap.of(
-          CppCompileActionContext.class, "",
-          CppLinkActionContext.class, "",
-          WriteAdbArgsActionContext.class, "");
+      return ImmutableMap.<Class<? extends ActionContext>, String>builder()
+          .put(CppCompileActionContext.class, "")
+          .put(CppLinkActionContext.class, "")
+          .put(WriteAdbArgsActionContext.class, "")
+          .build();
     }
   }
 
@@ -145,7 +146,7 @@ public class BazelRulesModule extends BlazeModule {
     return ImmutableList.<ActionContextConsumer>of(new BazelActionContextConsumer(
         optionsProvider.getOptions(BazelExecutionOptions.class)));
   }
-  
+
   @Subscribe
   public void gotOptions(GotOptionsEvent event) {
     optionsProvider = event.getOptions();
diff --git a/src/main/java/com/google/devtools/build/lib/buildtool/ExecutionTool.java b/src/main/java/com/google/devtools/build/lib/buildtool/ExecutionTool.java
index ce501d0da5..41256d4cbf 100644
--- a/src/main/java/com/google/devtools/build/lib/buildtool/ExecutionTool.java
+++ b/src/main/java/com/google/devtools/build/lib/buildtool/ExecutionTool.java
@@ -18,6 +18,7 @@ import com.google.common.base.Preconditions;
 import com.google.common.base.Predicate;
 import com.google.common.base.Stopwatch;
 import com.google.common.collect.HashBasedTable;
+import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
@@ -43,6 +44,7 @@ import com.google.devtools.build.lib.actions.ExecutorInitException;
 import com.google.devtools.build.lib.actions.LocalHostCapacity;
 import com.google.devtools.build.lib.actions.ResourceManager;
 import com.google.devtools.build.lib.actions.ResourceSet;
+import com.google.devtools.build.lib.actions.SimpleActionContextProvider;
 import com.google.devtools.build.lib.actions.SpawnActionContext;
 import com.google.devtools.build.lib.actions.TestExecException;
 import com.google.devtools.build.lib.actions.cache.ActionCache;
@@ -53,6 +55,7 @@ import com.google.devtools.build.lib.analysis.FileProvider;
 import com.google.devtools.build.lib.analysis.InputFileConfiguredTarget;
 import com.google.devtools.build.lib.analysis.OutputFileConfiguredTarget;
 import com.google.devtools.build.lib.analysis.OutputGroupProvider;
+import com.google.devtools.build.lib.analysis.SymlinkTreeActionContext;
 import com.google.devtools.build.lib.analysis.TopLevelArtifactContext;
 import com.google.devtools.build.lib.analysis.TopLevelArtifactHelper;
 import com.google.devtools.build.lib.analysis.TransitiveInfoCollection;
@@ -98,6 +101,7 @@ import java.io.IOException;
 import java.io.OutputStream;
 import java.io.PrintWriter;
 import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.Collection;
 import java.util.HashMap;
 import java.util.HashSet;
@@ -174,31 +178,64 @@ public class ExecutionTool {
   private final BuildRequest request;
   private BlazeExecutor executor;
   private ActionInputFileCache fileCache;
-  private List<ActionContextProvider> actionContextProviders;
+  private final ImmutableList<ActionContextProvider> actionContextProviders;
 
   private Map<String, SpawnActionContext> spawnStrategyMap =
       new TreeMap<>(String.CASE_INSENSITIVE_ORDER);
   private List<ActionContext> strategies = new ArrayList<>();
 
-  ExecutionTool(BlazeRuntime runtime, BuildRequest request) throws ExecutorInitException {
-    this.runtime = runtime;
-    this.request = request;
-
-    List<ActionContextConsumer> actionContextConsumers = new ArrayList<>();
-    actionContextProviders = new ArrayList<>();
+  private ImmutableList<ActionContextConsumer> getActionContextConsumersFromModules(
+      ActionContextConsumer... extraConsumers) {
+    ImmutableList.Builder<ActionContextConsumer> builder = ImmutableList.builder();
     for (BlazeModule module : runtime.getBlazeModules()) {
-      Iterables.addAll(actionContextProviders, module.getActionContextProviders());
-      Iterables.addAll(actionContextConsumers, module.getActionContextConsumers());
+      builder.addAll(module.getActionContextConsumers());
     }
+    builder.addAll(Arrays.asList(extraConsumers));
+    return builder.build();
+  }
 
-    actionContextProviders.add(new FilesetActionContextImpl.Provider(
-        runtime.getReporter(), runtime.getWorkspaceName()));
+  private ImmutableList<ActionContextProvider> getActionContextProvidersFromModules(
+      ActionContextProvider... extraProviders) {
+    ImmutableList.Builder<ActionContextProvider> builder = ImmutableList.builder();
+    for (BlazeModule module : runtime.getBlazeModules()) {
+      builder.addAll(module.getActionContextProviders());
+    }
+    builder.addAll(Arrays.asList(extraProviders));
+    return builder.build();
+  }
 
-    strategies.add(new SymlinkTreeStrategy(runtime.getOutputService(), runtime.getBinTools()));
+  ExecutionTool(final BlazeRuntime runtime, BuildRequest request) throws ExecutorInitException {
+    this.runtime = runtime;
+    this.request = request;
 
+    this.actionContextProviders =
+        getActionContextProvidersFromModules(
+            new FilesetActionContextImpl.Provider(
+                runtime.getReporter(), runtime.getWorkspaceName()),
+            new SimpleActionContextProvider(
+                new SymlinkTreeStrategy(runtime.getOutputService(), runtime.getBinTools())));
     StrategyConverter strategyConverter = new StrategyConverter(actionContextProviders);
-    strategies.add(strategyConverter.getStrategy(FilesetActionContext.class, ""));
-    strategies.add(strategyConverter.getStrategy(WorkspaceStatusAction.Context.class, ""));
+
+    ImmutableList<ActionContextConsumer> actionContextConsumers =
+        getActionContextConsumersFromModules(
+            // TODO(philwo) - the ExecutionTool should not add arbitrary dependencies on its own,
+            // instead these dependencies should be added to the ActionContextConsumer of the module
+            // that actually depends on them.
+            new ActionContextConsumer() {
+              @Override
+              public Map<String, String> getSpawnActionContexts() {
+                return ImmutableMap.of();
+              }
+
+              @Override
+              public Map<Class<? extends ActionContext>, String> getActionContexts() {
+                return ImmutableMap.<Class<? extends ActionContext>, String>builder()
+                    .put(FilesetActionContext.class, "")
+                    .put(WorkspaceStatusAction.Context.class, "")
+                    .put(SymlinkTreeActionContext.class, "")
+                    .build();
+              }
+            });
 
     for (ActionContextConsumer consumer : actionContextConsumers) {
       // There are many different SpawnActions, and we want to control the action context they use
@@ -209,26 +246,24 @@ public class ExecutionTool {
         SpawnActionContext context =
             strategyConverter.getStrategy(SpawnActionContext.class, entry.getValue());
         if (context == null) {
-          throw makeExceptionForInvalidStrategyValue(entry.getValue(), "spawn",
+          throw makeExceptionForInvalidStrategyValue(
+              entry.getValue(),
+              "spawn",
               strategyConverter.getValidValues(SpawnActionContext.class));
         }
-
         spawnStrategyMap.put(entry.getKey(), context);
       }
 
       for (Map.Entry<Class<? extends ActionContext>, String> entry :
           consumer.getActionContexts().entrySet()) {
         ActionContext context = strategyConverter.getStrategy(entry.getKey(), entry.getValue());
-        if (context != null) {
-          strategies.add(context);
-        } else if (!entry.getValue().isEmpty()) {
-          // If the action context consumer requested the default value (by passing in the empty
-          // string), we do not throw the exception, because we assume that whoever put together
-          // the modules in this Blaze binary knew what they were doing.
-          throw makeExceptionForInvalidStrategyValue(entry.getValue(),
+        if (context == null) {
+          throw makeExceptionForInvalidStrategyValue(
+              entry.getValue(),
               strategyConverter.getUserFriendlyName(entry.getKey()),
               strategyConverter.getValidValues(entry.getKey()));
         }
+        strategies.add(context);
       }
     }
 
diff --git a/src/main/java/com/google/devtools/build/lib/standalone/StandaloneContextConsumer.java b/src/main/java/com/google/devtools/build/lib/standalone/StandaloneContextConsumer.java
index 82c9f68a2f..d581dd1fbf 100644
--- a/src/main/java/com/google/devtools/build/lib/standalone/StandaloneContextConsumer.java
+++ b/src/main/java/com/google/devtools/build/lib/standalone/StandaloneContextConsumer.java
@@ -21,8 +21,6 @@ import com.google.devtools.build.lib.actions.SpawnActionContext;
 import com.google.devtools.build.lib.analysis.actions.FileWriteActionContext;
 import com.google.devtools.build.lib.rules.cpp.CppCompileActionContext;
 import com.google.devtools.build.lib.rules.cpp.IncludeScanningContext;
-import com.google.devtools.build.lib.rules.cpp.LinkStrategy;
-import com.google.devtools.build.lib.rules.test.TestStrategy;
 
 import java.util.Map;
 
@@ -44,10 +42,8 @@ public class StandaloneContextConsumer implements ActionContextConsumer {
     actionContexts.put(SpawnActionContext.class, "standalone");
 
     // C++.
-    actionContexts.put(LinkStrategy.class, "");
     actionContexts.put(IncludeScanningContext.class, "");
     actionContexts.put(CppCompileActionContext.class, "");
-    actionContexts.put(TestStrategy.class, "");
     actionContexts.put(FileWriteActionContext.class, "");
 
     return actionContexts.build();