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 2211b2a98d11f2..950a64e5b9197c 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/BUILD +++ b/src/main/java/com/google/devtools/build/lib/analysis/BUILD @@ -1616,6 +1616,7 @@ java_library( ":platform_options", "//src/main/java/com/google/devtools/build/lib/actions", "//src/main/java/com/google/devtools/build/lib/actions:artifacts", + "//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", 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 f9227ed124b766..545a8ffcfd1c76 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 @@ -845,7 +845,7 @@ public boolean remotableSourceManifestActions() { */ public ImmutableMap modifiedExecutionInfo( ImmutableMap executionInfo, String mnemonic) { - if (!options.executionInfoModifier.matches(mnemonic)) { + if (!ExecutionInfoModifier.collapse(options.executionInfoModifier, options.additiveModifyExecutionInfo).matches(mnemonic)) { return executionInfo; } Map mutableCopy = new HashMap<>(executionInfo); @@ -855,7 +855,7 @@ public ImmutableMap modifiedExecutionInfo( /** Applies {@code executionInfoModifiers} to the given {@code executionInfo}. */ public void modifyExecutionInfo(Map executionInfo, String mnemonic) { - options.executionInfoModifier.apply(mnemonic, executionInfo); + ExecutionInfoModifier.collapse(options.executionInfoModifier, options.additiveModifyExecutionInfo).apply(mnemonic, executionInfo); } /** @return 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 1bb62da4272c74..f2867415c883da 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 @@ -734,14 +734,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 " @@ -756,7 +757,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; + + @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 will be additive." + + " When disabled, only the last flag will be taken into account." + + ) + public boolean additiveModifyExecutionInfo; + @Option( name = "incompatible_genquery_use_graphless_query", @@ -928,6 +947,7 @@ public FragmentOptions getHost() { host.outputPathsMode = outputPathsMode; host.enableRunfiles = enableRunfiles; host.executionInfoModifier = executionInfoModifier; + host.additiveModifyExecutionInfo = additiveModifyExecutionInfo; host.commandLineBuildVariables = commandLineBuildVariables; host.enforceConstraints = enforceConstraints; host.mergeGenfilesDirectory = mergeGenfilesDirectory; 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..855ae7c206682a 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,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 executionInfoList, boolean isAdditive) { + ImmutableList.Builder 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 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..bb9102599d0759 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,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 invalidModifiers =