Make --modify_execution_info additive

Calling --modify_execution_info multiple times used to result in the
last version being used. With this change, options are amended
additively.

Fixes #13342

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

@@ -1660,6 +1660,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/actions:artifacts",
         "//src/main/java/com/google/devtools/build/lib/actions:build_configuration_event",
         "//src/main/java/com/google/devtools/build/lib/actions:commandline_limits",
+        "//src/main/java/com/google/devtools/build/lib/analysis:config/execution_info_modifier",
         "//src/main/java/com/google/devtools/build/lib/buildeventstream",
         "//src/main/java/com/google/devtools/build/lib/buildeventstream/proto:build_event_stream_java_proto",
         "//src/main/java/com/google/devtools/build/lib/cmdline",

src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationValue.java

@@ -894,7 +894,7 @@ public boolean remotableSourceManifestActions() {
    */
   public ImmutableMap<String, String> modifiedExecutionInfo(
       ImmutableMap<String, String> executionInfo, String mnemonic) {
-    if (!options.executionInfoModifier.matches(mnemonic)) {
+    if (!ExecutionInfoModifier.collapse(options.executionInfoModifier, options.additiveModifyExecutionInfo).matches(mnemonic)) {
       return executionInfo;
     }
     Map<String, String> mutableCopy = new HashMap<>(executionInfo);
@@ -904,7 +904,7 @@ public ImmutableMap<String, String> modifiedExecutionInfo(
 
   /** Applies {@code executionInfoModifiers} to the given {@code executionInfo}. */
   public void modifyExecutionInfo(Map<String, String> executionInfo, String mnemonic) {
-    options.executionInfoModifier.apply(mnemonic, executionInfo);
+    ExecutionInfoModifier.collapse(options.executionInfoModifier, options.additiveModifyExecutionInfo).apply(mnemonic, executionInfo);
   }
 
   /** Returns the list of default features used for all packages. */

src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptions.java

@@ -807,14 +807,15 @@ public OutputPathsConverter() {
 
   @Option(
       name = "modify_execution_info",
+      allowMultiple = true,
       converter = ExecutionInfoModifier.Converter.class,
+      defaultValue = "null",
       documentationCategory = OptionDocumentationCategory.EXECUTION_STRATEGY,
       effectTags = {
         OptionEffectTag.EXECUTION,
         OptionEffectTag.AFFECTS_OUTPUTS,
         OptionEffectTag.LOADING_AND_ANALYSIS,
       },
-      defaultValue = "",
       help =
           "Add or remove keys from an action's execution info based on action mnemonic.  "
               + "Applies only to actions which support execution info. Many common actions "
@@ -829,7 +830,25 @@ public OutputPathsConverter() {
               + "all Genrule actions.\n"
               + "  '(?!Genrule).*=-requires-x' removes 'requires-x' from the execution info for "
               + "all non-Genrule actions.\n")
-  public ExecutionInfoModifier executionInfoModifier;
+  public List<ExecutionInfoModifier> executionInfoModifier;
+
+  @Option(
+      name = "incompatible_modify_execution_info_additive_flag",
+      defaultValue = "false",
+      documentationCategory = OptionDocumentationCategory.EXECUTION_STRATEGY,
+      effectTags = {
+        OptionEffectTag.EXECUTION,
+        OptionEffectTag.AFFECTS_OUTPUTS,
+        OptionEffectTag.LOADING_AND_ANALYSIS,
+      },
+      metadataTags = {OptionMetadataTag.INCOMPATIBLE_CHANGE},
+      help =
+          "When enabled, passing multiple --modify_execution_info flags is additive."
+              + " When disabled, only the last flag is taken into account."
+
+      )
+  public boolean additiveModifyExecutionInfo;
+
 
   @Option(
       name = "include_config_fragments_provider",

src/main/java/com/google/devtools/build/lib/analysis/config/ExecutionInfoModifier.java

@@ -21,6 +21,8 @@
 import com.google.common.collect.ImmutableList;
 import com.google.devtools.common.options.Converters.RegexPatternConverter;
 import com.google.devtools.common.options.OptionsParsingException;
+
+import java.util.List;
 import java.util.Map;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
@@ -81,7 +83,9 @@ public ExecutionInfoModifier convert(String input) throws OptionsParsingExceptio
                     .regexPattern()
                     .pattern(),
                 specMatcher.group("sign").equals("-"),
-                specMatcher.group("key")));
+                specMatcher.group("key")
+            )
+        );
       }
       return ExecutionInfoModifier.create(input, expressionBuilder.build());
     }
@@ -103,6 +107,28 @@ public boolean matches(String mnemonic) {
     return expressions().stream().anyMatch(expr -> expr.pattern().matcher(mnemonic).matches());
   }
 
+  /**
+   * Merge all the elements of {@code executionInfoList} into a single {@link ExecutionInfoModifier}.
+   * The expressions in the returned instance may contain the same pattern multiple times.
+   */
+  public static ExecutionInfoModifier collapse(List<ExecutionInfoModifier> executionInfoList, boolean isAdditive) {
+    ImmutableList.Builder<Expression> allExpressionsBuilder = ImmutableList.builder();
+
+    if (executionInfoList != null && executionInfoList.size() > 0) {
+      if (isAdditive) {
+        for (ExecutionInfoModifier eim : executionInfoList) {
+          allExpressionsBuilder.addAll(eim.expressions());
+        }
+      } else {
+        // When not treating execution info as additive, only the last passed option wins.
+        allExpressionsBuilder.addAll(executionInfoList.get(executionInfoList.size() - 1).expressions());
+      }
+
+    }
+
+    return create("", allExpressionsBuilder.build());
+  }
+
   /** Modifies the given map of {@code executionInfo} to add or remove the keys for this option. */
   void apply(String mnemonic, Map<String, String> executionInfo) {
     for (Expression expr : expressions()) {

src/main/starlark/builtins_bzl/common/builtin_exec_platforms.bzl

@@ -227,6 +227,7 @@ bazel_fragments["CoreOptions"] = fragment(
         "//command_line_option:incompatible_merge_genfiles_directory",
         "//command_line_option:experimental_platform_in_output_dir",
         "//command_line_option:host_cpu",
+        "//command_line_option:incompatible_modify_execution_info_additive_flag",
         "//command_line_option:include_config_fragments_provider",
         "//command_line_option:experimental_debug_selects_always_succeed",
         "//command_line_option:incompatible_check_testonly_for_output_files",

src/test/java/com/google/devtools/build/lib/analysis/config/ExecutionInfoModifierTest.java

@@ -69,6 +69,44 @@ public void executionInfoModifier_multipleExpressions() throws Exception {
     assertModifierMatchesAndResults(modifier, "GenericAction", ImmutableSet.of("y"));
   }
 
+  @Test
+  public void executionInfoModifier_multipleOptionsAdditive() throws Exception {
+    ExecutionInfoModifier modifier1 = converter.convert("Genrule=+x,CppCompile=-y1,GenericAction=+z,MergeLayers=+t,OtherAction=+o");
+    ExecutionInfoModifier modifier2 = converter.convert("Genrule=-x,CppCompile=+y1,CppCompile=+y2,GenericAction=+z,MergeLayers=+u");
+    ExecutionInfoModifier modifier3 = converter.convert(".*=-t");
+
+    ExecutionInfoModifier mergedModifier = ExecutionInfoModifier.collapse(List.of(modifier1, modifier2, modifier3), true);
+
+    assertModifierMatchesAndResults(mergedModifier, "Genrule", ImmutableSet.of());
+    assertModifierMatchesAndResults(mergedModifier, "CppCompile", ImmutableSet.of("y1", "y2"));
+    assertModifierMatchesAndResults(mergedModifier, "GenericAction", ImmutableSet.of("z"));
+    assertModifierMatchesAndResults(mergedModifier, "MergeLayers", ImmutableSet.of("u"));
+    assertModifierMatchesAndResults(mergedModifier, "OtherAction", ImmutableSet.of("o"));
+  }
+
+  @Test
+  public void executionInfoModifier_multipleOptionsNonAdditive() throws Exception {
+    ExecutionInfoModifier modifier1 = converter.convert("Genrule=+x,CppCompile=-y1,GenericAction=+z,MergeLayers=+t,OtherAction=+o");
+    ExecutionInfoModifier modifier2 = converter.convert("Genrule=-x,CppCompile=+y1,CppCompile=+y2,GenericAction=+z,MergeLayers=+u");
+    ExecutionInfoModifier modifier3 = converter.convert(".*=-t");
+
+    ExecutionInfoModifier mergedModifier1 = ExecutionInfoModifier.collapse(List.of(modifier1, modifier2), false);
+
+    assertModifierMatchesAndResults(mergedModifier1, "Genrule", ImmutableSet.of());
+    assertModifierMatchesAndResults(mergedModifier1, "CppCompile", ImmutableSet.of("y1", "y2"));
+    assertModifierMatchesAndResults(mergedModifier1, "GenericAction", ImmutableSet.of("z"));
+    assertModifierMatchesAndResults(mergedModifier1, "MergeLayers", ImmutableSet.of("u"));
+    assertThat(mergedModifier1.matches("OtherAction")).isFalse();
+
+    ExecutionInfoModifier mergedModifier2 = ExecutionInfoModifier.collapse(List.of(modifier1, modifier2, modifier3), false);
+
+    assertModifierMatchesAndResults(mergedModifier2, "Genrule", ImmutableSet.of());
+    assertModifierMatchesAndResults(mergedModifier2, "CppCompile", ImmutableSet.of());
+    assertModifierMatchesAndResults(mergedModifier2, "GenericAction", ImmutableSet.of());
+    assertModifierMatchesAndResults(mergedModifier2, "MergeLayers", ImmutableSet.of());
+    assertModifierMatchesAndResults(mergedModifier2, "OtherAction", ImmutableSet.of());
+  }
+
   @Test
   public void executionInfoModifier_invalidFormat_throws() throws Exception {
     List<String> invalidModifiers =