From ed03f3d8d4feab4336c131b9e8aaa5cc168787ab Mon Sep 17 00:00:00 2001 From: mstaib Date: Wed, 26 Sep 2018 10:04:59 -0700 Subject: [PATCH] Make ExecutionInfoModifier have a useful equals by making it AutoValue. All objects which are to be part of the build options must parse equal on repeated invocations, or the analysis cache will be discarded unnecessarily. It's also now possible to create an empty ExecutionInfoModifier. This is used instead of null as the default value. This allows for more easily turning --modify_execution_info off, even when it's in the RC file. (It's already possible to do this by creating a silly regex like "(?!.*)=-nonsense-value", so this isn't a new capability, it's just a more discoverable one.) Also cleans up the parsing code to use Guava string-handling methods and fixes some uses of plain non-linked HashMap surrounding the use of this. Most users of this method were already using LinkedHashMaps, so this makes callers more consistent. RELNOTES: None. PiperOrigin-RevId: 214622440 --- .../analysis/config/BuildConfiguration.java | 10 ++-- .../config/ExecutionInfoModifier.java | 55 +++++++++++-------- .../lib/rules/cpp/CppLinkActionBuilder.java | 3 +- .../config/ExecutionInfoModifierTest.java | 31 ++++++++++- 4 files changed, 68 insertions(+), 31 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java index 3568f35161b195..ac2f9a96c6496d 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java @@ -911,7 +911,7 @@ public ConfigsModeConverter() { OptionEffectTag.AFFECTS_OUTPUTS, OptionEffectTag.LOADING_AND_ANALYSIS, }, - defaultValue = "null", + 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 " @@ -1841,19 +1841,17 @@ public boolean runfilesEnabled() { */ public ImmutableMap modifiedExecutionInfo( ImmutableMap executionInfo, String mnemonic) { - if (options.executionInfoModifier == null || !options.executionInfoModifier.matches(mnemonic)) { + if (!options.executionInfoModifier.matches(mnemonic)) { return executionInfo; } - HashMap mutableCopy = new HashMap<>(executionInfo); + LinkedHashMap mutableCopy = new LinkedHashMap<>(executionInfo); modifyExecutionInfo(mutableCopy, mnemonic); return ImmutableMap.copyOf(mutableCopy); } /** Applies {@code executionInfoModifiers} to the given {@code executionInfo}. */ public void modifyExecutionInfo(Map executionInfo, String mnemonic) { - if (options.executionInfoModifier != null) { - options.executionInfoModifier.apply(mnemonic, executionInfo); - } + options.executionInfoModifier.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/ExecutionInfoModifier.java b/src/main/java/com/google/devtools/build/lib/analysis/config/ExecutionInfoModifier.java index 1a71fc8b5d28b2..ff37fb09f5dd16 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 @@ -15,7 +15,11 @@ package com.google.devtools.build.lib.analysis.config; import com.google.auto.value.AutoValue; +import com.google.auto.value.extension.memoized.Memoized; +import com.google.common.base.Splitter; +import com.google.common.base.Strings; import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableList.Builder; import com.google.devtools.common.options.Converters.RegexPatternConverter; import com.google.devtools.common.options.OptionsParsingException; import java.util.Map; @@ -26,40 +30,44 @@ * Represents a list of regexes over mnemonics and changes to add or remove keys, parsed from a * {@code --modify_execution_info} option. */ -public final class ExecutionInfoModifier { +@AutoValue +public abstract class ExecutionInfoModifier { private static final Pattern MODIFIER_PATTERN = Pattern.compile("^(?.+)=(?[+-])(?.+)$"); - private final ImmutableList expressions; - private final String option; + private static final ExecutionInfoModifier EMPTY = create("", ImmutableList.of()); + + abstract String option(); + abstract ImmutableList expressions(); @AutoValue abstract static class Expression { - abstract Pattern pattern(); + // Patterns do not have a useful equals(), so compare by the regex and memoize the derived + // Pattern. + @Memoized + Pattern pattern() { + return Pattern.compile(regex()); + } + + abstract String regex(); abstract boolean remove(); abstract String key(); } - private ExecutionInfoModifier(String option, ImmutableList expressions) { - this.expressions = expressions; - this.option = option; - } - /** Constructs an instance of ExecutionInfoModifier by parsing an option string. */ public static class Converter implements com.google.devtools.common.options.Converter { @Override public ExecutionInfoModifier convert(String input) throws OptionsParsingException { - - String[] expressionSpecs = input.split(","); - if (expressionSpecs.length == 0) { - throw new OptionsParsingException("expected 'regex=[+-]key,regex=[+-]key,...'", input); + if (Strings.isNullOrEmpty(input)) { + return EMPTY; } + ImmutableList.Builder expressionBuilder = ImmutableList.builder(); - for (String spec : expressionSpecs) { + for (String spec : Splitter.on(",").split(input)) { Matcher specMatcher = MODIFIER_PATTERN.matcher(spec); if (!specMatcher.matches()) { throw new OptionsParsingException( @@ -67,11 +75,13 @@ public ExecutionInfoModifier convert(String input) throws OptionsParsingExceptio } expressionBuilder.add( new AutoValue_ExecutionInfoModifier_Expression( - new RegexPatternConverter().convert(specMatcher.group("pattern")), + // 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")).pattern(), specMatcher.group("sign").equals("-"), specMatcher.group("key"))); } - return new ExecutionInfoModifier(input, expressionBuilder.build()); + return ExecutionInfoModifier.create(input, expressionBuilder.build()); } @Override @@ -80,16 +90,20 @@ public String getTypeDescription() { } } + private static ExecutionInfoModifier create(String input, ImmutableList expressions) { + return new AutoValue_ExecutionInfoModifier(input, expressions); + } + /** * Determines whether the given {@code mnemonic} (e.g. "CppCompile") matches any of the patterns. */ public boolean matches(String mnemonic) { - return expressions.stream().anyMatch(expr -> expr.pattern().matcher(mnemonic).matches()); + return expressions().stream().anyMatch(expr -> expr.pattern().matcher(mnemonic).matches()); } /** 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) { + for (Expression expr : expressions()) { if (expr.pattern().matcher(mnemonic).matches()) { if (expr.remove()) { executionInfo.remove(expr.key()); @@ -99,9 +113,4 @@ void apply(String mnemonic, Map executionInfo) { } } } - - @Override - public String toString() { - return option; - } } diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionBuilder.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionBuilder.java index 4f41338cb36429..1d7cb7320c7444 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionBuilder.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionBuilder.java @@ -58,6 +58,7 @@ import java.util.ArrayList; import java.util.Collection; import java.util.HashMap; +import java.util.LinkedHashMap; import java.util.LinkedHashSet; import java.util.List; import java.util.Map; @@ -1077,7 +1078,7 @@ public CppLinkAction build() throws InterruptedException { // If the crosstool uses action_configs to configure cc compilation, collect execution info // from there, otherwise, use no execution info. // TODO(b/27903698): Assert that the crosstool has an action_config for this action. - Map executionRequirements = new HashMap<>(); + Map executionRequirements = new LinkedHashMap<>(); if (featureConfiguration.actionIsConfigured(getActionName())) { for (String req : featureConfiguration.getToolRequirementsForAction(getActionName())) { 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 4b42fa09484c4a..ad746e444ccf1b 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 @@ -20,6 +20,7 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; +import com.google.common.testing.EqualsTester; import com.google.devtools.build.lib.analysis.config.ExecutionInfoModifier.Converter; import com.google.devtools.common.options.OptionsParsingException; import java.util.HashMap; @@ -36,6 +37,12 @@ public class ExecutionInfoModifierTest { private final ExecutionInfoModifier.Converter converter = new Converter(); + @Test + public void executionInfoModifier_empty() throws Exception { + ExecutionInfoModifier modifier = converter.convert(""); + assertThat(modifier.matches("Anything")).isFalse(); + } + @Test public void executionInfoModifier_singleAdd() throws Exception { ExecutionInfoModifier modifier = converter.convert("Genrule=+x"); @@ -65,7 +72,7 @@ public void executionInfoModifier_multipleExpressions() throws Exception { @Test public void executionInfoModifier_invalidFormat_throws() throws Exception { List invalidModifiers = - ImmutableList.of("", "A", "=", "A=", "A=+", "=+", "A=-B,A", "A=B", "A", ","); + ImmutableList.of("A", "=", "A=", "A=+", "=+", "A=-B,A", "A=B", "A", ","); for (String invalidModifer : invalidModifiers) { assertThrows(OptionsParsingException.class, () -> converter.convert(invalidModifer)); } @@ -79,6 +86,28 @@ public void executionInfoModifier_invalidFormat_exceptionShowsOffender() throws assertThat(thrown).hasMessageThat().contains("'B=2'"); } + @Test + public void executionInfoModifier_EqualsTester() throws Exception { + new EqualsTester() + // base empty + .addEqualityGroup(converter.convert(""), converter.convert("")) + // base non-empty + .addEqualityGroup(converter.convert("A=+B"), converter.convert("A=+B")) + // different pattern and key + .addEqualityGroup(converter.convert("C=+D")) + // different key + .addEqualityGroup(converter.convert("A=+D")) + // different pattern + .addEqualityGroup(converter.convert("C=+B")) + // different operation + .addEqualityGroup(converter.convert("A=-B")) + // more items + .addEqualityGroup(converter.convert("A=+B,C=-D"), converter.convert("A=+B,C=-D")) + // different order + .addEqualityGroup(converter.convert("C=-D,A=+B")) + .testEquals(); + } + private void assertModifierMatchesAndResults( ExecutionInfoModifier modifier, String mnemonic, Set expectedKeys) { Map copy = new HashMap<>();