diff --git a/src/main/java/com/google/devtools/build/lib/analysis/BUILD b/src/main/java/com/google/devtools/build/lib/analysis/BUILD index bcb9c0a27c6b20..633c85da854242 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/BUILD +++ b/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", diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationValue.java b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationValue.java index 2298a944c055b2..db06d81b32f01b 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationValue.java +++ b/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 modifiedExecutionInfo( ImmutableMap executionInfo, String mnemonic) { - if (!options.executionInfoModifier.matches(mnemonic)) { + if (!ExecutionInfoModifier.matches( + options.executionInfoModifier, options.additiveModifyExecutionInfo, mnemonic)) { return executionInfo; } Map mutableCopy = new HashMap<>(executionInfo); @@ -907,7 +910,11 @@ public ImmutableMap modifiedExecutionInfo( /** Applies {@code executionInfoModifiers} to the given {@code executionInfo}. */ public void modifyExecutionInfo(Map 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. */ diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptions.java b/src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptions.java index d089dba6863271..36aad58c7c38af 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptions.java +++ b/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; + + @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", diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/ExecutionInfoModifier.java b/src/main/java/com/google/devtools/build/lib/analysis/config/ExecutionInfoModifier.java index 2fbf3280b6cd20..33894fb577614f 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/config/ExecutionInfoModifier.java +++ b/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 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 expr.pattern().matcher(mnemonic).matches()); } + /** Checks whether the {@code executionInfoList} matches the {@code mnemonic}. */ + public static boolean matches( + List 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 executionInfoList, + boolean isAdditive, + String mnemonic, + Map 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 executionInfo) { for (Expression expr : expressions()) { diff --git a/src/test/java/com/google/devtools/build/lib/analysis/config/ExecutionInfoModifierTest.java b/src/test/java/com/google/devtools/build/lib/analysis/config/ExecutionInfoModifierTest.java index ea43eded712e31..47046a81dd9bb9 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/config/ExecutionInfoModifierTest.java +++ b/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 invalidModifiers = @@ -110,9 +168,18 @@ public void executionInfoModifier_EqualsTester() throws Exception { private void assertModifierMatchesAndResults( ExecutionInfoModifier modifier, String mnemonic, Set expectedKeys) { + assertModifierMatchesAndResults( + ImmutableList.of(modifier), /* additive= */ false, mnemonic, expectedKeys); + } + + private void assertModifierMatchesAndResults( + List modifiers, + boolean additive, + String mnemonic, + Set expectedKeys) { Map 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 -> "")));