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 if --incompatible_modify_execution_info_additive is set.

Fixes #13342

Closes #16262.

PiperOrigin-RevId: 631803759
Change-Id: I5386cbb0d02ef19a6b2ddf2f818cbab660b17c31

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

@@ -1652,6 +1652,7 @@ java_library(
         ":config/build_options",
         ":config/compilation_mode",
         ":config/core_options",
+        ":config/execution_info_modifier",
         ":config/feature_set",
         ":config/fragment",
         ":config/fragment_class_set",

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

@@ -362,7 +362,9 @@ public ArtifactRoot getOutputDirectory(RepositoryName repositoryName) {
     return outputDirectories.getOutputDirectory(repositoryName);
   }
 
-  /** @deprecated Use {@link #getBinDirectory} instead. */
+  /**
+   * @deprecated Use {@link #getBinDirectory} instead.
+   */
   @Override
   @Deprecated
   public ArtifactRoot getBinDir() {
@@ -897,7 +899,8 @@ public boolean remotableSourceManifestActions() {
    */
   public ImmutableMap<String, String> modifiedExecutionInfo(
       ImmutableMap<String, String> executionInfo, String mnemonic) {
-    if (!options.executionInfoModifier.matches(mnemonic)) {
+    if (!ExecutionInfoModifier.matches(
+        options.executionInfoModifier, options.additiveModifyExecutionInfo, mnemonic)) {
       return executionInfo;
     }
     Map<String, String> mutableCopy = new HashMap<>(executionInfo);
@@ -907,7 +910,11 @@ 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.apply(
+        options.executionInfoModifier,
+        options.additiveModifyExecutionInfo,
+        mnemonic,
+        executionInfo);
   }
 
   /** Returns the list of default features used for all packages. */

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

@@ -819,14 +819,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 "
@@ -841,7 +842,22 @@ 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",
+      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,7 @@
 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;
@@ -38,6 +39,7 @@ public abstract class ExecutionInfoModifier {
   private static final ExecutionInfoModifier EMPTY = create("", ImmutableList.of());
 
   abstract String option();
+
   abstract ImmutableList<Expression> expressions();
 
   @AutoValue
@@ -77,7 +79,7 @@ public ExecutionInfoModifier convert(String input) throws OptionsParsingExceptio
                 // Convert to get a useful exception if it's not a valid pattern, but use the regex
                 // (see comment in Expression)
                 new RegexPatternConverter()
-                    .convert(specMatcher.group("pattern"), /*conversionContext=*/ null)
+                    .convert(specMatcher.group("pattern"), /* conversionContext= */ null)
                     .regexPattern()
                     .pattern(),
                 specMatcher.group("sign").equals("-"),
@@ -99,10 +101,41 @@ private static ExecutionInfoModifier create(String input, ImmutableList<Expressi
   /**
    * Determines whether the given {@code mnemonic} (e.g. "CppCompile") matches any of the patterns.
    */
-  public boolean matches(String mnemonic) {
+  boolean matches(String mnemonic) {
     return expressions().stream().anyMatch(expr -> expr.pattern().matcher(mnemonic).matches());
   }
 
+  /** Checks whether the {@code executionInfoList} matches the {@code mnemonic}. */
+  public static boolean matches(
+      List<ExecutionInfoModifier> executionInfoList, boolean isAdditive, String mnemonic) {
+    if (executionInfoList.isEmpty()) {
+      return false;
+    }
+
+    if (isAdditive) {
+      return executionInfoList.stream().anyMatch(eim -> eim.matches(mnemonic));
+    } else {
+      return executionInfoList.getLast().matches(mnemonic);
+    }
+  }
+
+  /** Applies {@code executionInfoList} to the given {@code executionInfo}. */
+  public static void apply(
+      List<ExecutionInfoModifier> executionInfoList,
+      boolean isAdditive,
+      String mnemonic,
+      Map<String, String> executionInfo) {
+    if (executionInfoList.isEmpty()) {
+      return;
+    }
+
+    if (isAdditive) {
+      executionInfoList.forEach(eim -> eim.apply(mnemonic, executionInfo));
+    } else {
+      executionInfoList.getLast().apply(mnemonic, executionInfo);
+    }
+  }
+
   /** 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/test/java/com/google/devtools/build/lib/analysis/config/ExecutionInfoModifierTest.java

@@ -69,6 +69,64 @@ public void executionInfoModifier_multipleExpressions() throws Exception {
     assertModifierMatchesAndResults(modifier, "GenericAction", ImmutableSet.of("y"));
   }
 
+  @Test
+  public void executionInfoModifier_multipleOptionsAdditive() throws Exception {
+    var modifier1 =
+        converter.convert(
+            "Genrule=+x,CppCompile=-y1,GenericAction=+z,MergeLayers=+t,OtherAction=+o");
+    var modifier2 =
+        converter.convert(
+            "Genrule=-x,CppCompile=+y1,CppCompile=+y2,GenericAction=+z,MergeLayers=+u");
+    var modifier3 = converter.convert(".*=-t");
+
+    var modifiers = ImmutableList.of(modifier1, modifier2, modifier3);
+    assertModifierMatchesAndResults(modifiers, /* additive= */ true, "Genrule", ImmutableSet.of());
+    assertModifierMatchesAndResults(
+        modifiers, /* additive= */ true, "CppCompile", ImmutableSet.of("y1", "y2"));
+    assertModifierMatchesAndResults(
+        modifiers, /* additive= */ true, "GenericAction", ImmutableSet.of("z"));
+    assertModifierMatchesAndResults(
+        modifiers, /* additive= */ true, "MergeLayers", ImmutableSet.of("u"));
+    assertModifierMatchesAndResults(
+        modifiers, /* additive= */ true, "OtherAction", ImmutableSet.of("o"));
+  }
+
+  @Test
+  public void executionInfoModifier_multipleOptionsNonAdditive() throws Exception {
+    var modifier1 =
+        converter.convert(
+            "Genrule=+x,CppCompile=-y1,GenericAction=+z,MergeLayers=+t,OtherAction=+o");
+    var modifier2 =
+        converter.convert(
+            "Genrule=-x,CppCompile=+y1,CppCompile=+y2,GenericAction=+z,MergeLayers=+u");
+    var modifier3 = converter.convert(".*=-t");
+
+    var modifiers1 = ImmutableList.of(modifier1, modifier2);
+
+    assertModifierMatchesAndResults(
+        modifiers1, /* additive= */ false, "Genrule", ImmutableSet.of());
+    assertModifierMatchesAndResults(
+        modifiers1, /* additive= */ false, "CppCompile", ImmutableSet.of("y1", "y2"));
+    assertModifierMatchesAndResults(
+        modifiers1, /* additive= */ false, "GenericAction", ImmutableSet.of("z"));
+    assertModifierMatchesAndResults(
+        modifiers1, /* additive= */ false, "MergeLayers", ImmutableSet.of("u"));
+    assertThat(ExecutionInfoModifier.matches(modifiers1, false, "OtherAction")).isFalse();
+
+    var modifiers2 = ImmutableList.of(modifier1, modifier2, modifier3);
+
+    assertModifierMatchesAndResults(
+        modifiers2, /* additive= */ false, "Genrule", ImmutableSet.of());
+    assertModifierMatchesAndResults(
+        modifiers2, /* additive= */ false, "CppCompile", ImmutableSet.of());
+    assertModifierMatchesAndResults(
+        modifiers2, /* additive= */ false, "GenericAction", ImmutableSet.of());
+    assertModifierMatchesAndResults(
+        modifiers2, /* additive= */ false, "MergeLayers", ImmutableSet.of());
+    assertModifierMatchesAndResults(
+        modifiers2, /* additive= */ false, "OtherAction", ImmutableSet.of());
+  }
+
   @Test
   public void executionInfoModifier_invalidFormat_throws() throws Exception {
     List<String> invalidModifiers =
@@ -110,9 +168,18 @@ public void executionInfoModifier_EqualsTester() throws Exception {
 
   private void assertModifierMatchesAndResults(
       ExecutionInfoModifier modifier, String mnemonic, Set<String> expectedKeys) {
+    assertModifierMatchesAndResults(
+        ImmutableList.of(modifier), /* additive= */ false, mnemonic, expectedKeys);
+  }
+
+  private void assertModifierMatchesAndResults(
+      List<ExecutionInfoModifier> modifiers,
+      boolean additive,
+      String mnemonic,
+      Set<String> expectedKeys) {
     Map<String, String> copy = new HashMap<>();
-    modifier.apply(mnemonic, copy);
-    assertThat(modifier.matches(mnemonic)).isTrue();
+    ExecutionInfoModifier.apply(modifiers, additive, mnemonic, copy);
+    assertThat(ExecutionInfoModifier.matches(modifiers, additive, mnemonic)).isTrue();
     assertThat(copy)
         .containsExactlyEntriesIn(
             expectedKeys.stream().collect(ImmutableMap.toImmutableMap(k -> k, unused -> "")));