From 08ff9b8761bdbf3e6ed7487678e892afb812d732 Mon Sep 17 00:00:00 2001 From: ulfjack Date: Thu, 28 Sep 2017 04:08:06 -0400 Subject: Make the state in RuleContext explicit This isn't ideal - RuleContext should not have state, but this ended up happening between adding a cache and refactoring how make variables are discovered. I have carefully traced back all callers that provide custom make variable suppliers and added an init call to their rule initialization. Note that the ConfigurationMakeVariableContext is _cached_, so callers that call in without any make variable suppliers and then call again with them would get the context from the previous call. We now enforce that the ConfigurationMakeVariableContext is only initialized once, and that this happens before any usage, which is slightly better than the previous state, where initialization was silently ignored on any subsequent call. Progress on #2475. PiperOrigin-RevId: 170312285 --- .../devtools/build/lib/analysis/RuleContext.java | 94 +++++++--------------- .../build/lib/analysis/RunfilesSupport.java | 29 ++----- .../devtools/build/lib/rules/cpp/CcBinary.java | 3 + .../devtools/build/lib/rules/cpp/CcCommon.java | 11 +-- .../devtools/build/lib/rules/cpp/CcLibrary.java | 3 + .../devtools/build/lib/rules/cpp/CppHelper.java | 37 ++++----- .../devtools/build/lib/rules/python/PyBinary.java | 6 +- 7 files changed, 58 insertions(+), 125 deletions(-) (limited to 'src/main') diff --git a/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java b/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java index 8aaaaa44e0..9a114f544f 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java @@ -960,6 +960,18 @@ public final class RuleContext extends TargetContext return result; } + public void initConfigurationMakeVariableContext( + Iterable makeVariableSuppliers) { + Preconditions.checkState(configurationMakeVariableContext == null); + configurationMakeVariableContext = + new ConfigurationMakeVariableContext( + this, getRule().getPackage(), getConfiguration(), makeVariableSuppliers); + } + + public void initConfigurationMakeVariableContext(MakeVariableSupplier... makeVariableSuppliers) { + initConfigurationMakeVariableContext(ImmutableList.copyOf(makeVariableSuppliers)); + } + /** Indicates whether a string list attribute should be tokenized. */ public enum Tokenize { YES, @@ -974,22 +986,7 @@ public final class RuleContext extends TargetContext * @return a list of strings containing the expanded and tokenized values for the attribute */ public ImmutableList getTokenizedStringListAttr(String attributeName) { - return getExpandedStringListAttr( - attributeName, Tokenize.YES, getConfigurationMakeVariableContext()); - } - - /** - * Gets an attribute of type STRING_LIST expanding Make variables, $(location) tags into the - * dependency location (see {@link LocationExpander} for details) and tokenizes the result. - * - * @param attributeName the name of the attribute to process - * @param makeVariableContext the make variable context - * @return a list of strings containing the expanded and tokenized values for the attribute - */ - public ImmutableList getTokenizedStringListAttr( - String attributeName, ConfigurationMakeVariableContext makeVariableContext) { - return getExpandedStringListAttr( - attributeName, Tokenize.YES, makeVariableContext); + return getExpandedStringListAttr(attributeName, Tokenize.YES); } /** @@ -1000,8 +997,7 @@ public final class RuleContext extends TargetContext * @return a list of strings containing the processed values for the attribute */ public ImmutableList getExpandedStringListAttr(String attributeName) { - return getExpandedStringListAttr( - attributeName, Tokenize.NO, getConfigurationMakeVariableContext()); + return getExpandedStringListAttr(attributeName, Tokenize.NO); } /** @@ -1009,13 +1005,11 @@ public final class RuleContext extends TargetContext * optionally tokenizes the result. * * @param attributeName the name of the attribute to process - * @param makeVariableContext the make variable context * @return a list of strings containing the processed values for the attribute */ private ImmutableList getExpandedStringListAttr( String attributeName, - Tokenize tokenize, - ConfigurationMakeVariableContext makeVariableContext) { + Tokenize tokenize) { if (!getRule().isAttrDefined(attributeName, Type.STRING_LIST)) { // TODO(bazel-team): This should be an error. return ImmutableList.of(); @@ -1029,27 +1023,21 @@ public final class RuleContext extends TargetContext new LocationExpander(this, LocationExpander.Options.ALLOW_DATA); for (String token : original) { - expandValue(tokens, attributeName, token, locationExpander, tokenize, makeVariableContext); + expandValue(tokens, attributeName, token, locationExpander, tokenize); } return ImmutableList.copyOf(tokens); } /** * Expands make variables in value and tokenizes the result into tokens. - * - * @param makeVariableContext the make variable context - *

This methods should be called only during initialization. */ public void tokenizeAndExpandMakeVars( List result, String attributeName, - String value, - ConfigurationMakeVariableContext makeVariableContext) { + String value) { LocationExpander locationExpander = new LocationExpander(this, Options.ALLOW_DATA, Options.EXEC_PATHS); - expandValue( - result, attributeName, value, locationExpander, Tokenize.YES, - makeVariableContext); + expandValue(result, attributeName, value, locationExpander, Tokenize.YES); } /** @@ -1060,12 +1048,11 @@ public final class RuleContext extends TargetContext String attributeName, String value, @Nullable LocationExpander locationExpander, - Tokenize tokenize, - ConfigurationMakeVariableContext makeVariableContext) { + Tokenize tokenize) { if (locationExpander != null) { value = locationExpander.expandAttribute(attributeName, value); } - value = expandMakeVariables(attributeName, value, makeVariableContext); + value = expandMakeVariables(attributeName, value); if (tokenize == Tokenize.YES) { try { ShellUtils.tokenize(tokens, value); @@ -1098,33 +1085,12 @@ public final class RuleContext extends TargetContext } /** - * Returns a (cached! read on) context that maps Make variable names (string) to values (string) - * without any extra {@link MakeVariableSupplier}. - * - *

Beware!!! {@link ConfigurationMakeVariableContext} instance is cached, so if you call it - * first with some list of {@link MakeVariableSupplier} and then with other list, you will always - * get the first instance back. TODO(hlopko): Extract Make variable expansion from RuleContext and - * fix all the callers - * - * @return a ConfigurationMakeVariableContext. + * Returns a cached context that maps Make variable names (string) to values (string) without any + * extra {@link MakeVariableSupplier}. */ public ConfigurationMakeVariableContext getConfigurationMakeVariableContext() { - return getConfigurationMakeVariableContext(ImmutableList.of()); - } - - /** - * Returns a (cached! read on) context that maps Make variable names (string) to values (string). - * - * @see #getConfigurationMakeVariableContext() to understand how the instance is cached! - * @param makeVariableSuppliers to be used with {@link ConfigurationMakeVariableContext} - * @return a ConfigurationMakeVariableContext. - */ - public ConfigurationMakeVariableContext getConfigurationMakeVariableContext( - Iterable makeVariableSuppliers) { if (configurationMakeVariableContext == null) { - configurationMakeVariableContext = - new ConfigurationMakeVariableContext( - this, getRule().getPackage(), getConfiguration(), makeVariableSuppliers); + initConfigurationMakeVariableContext(ImmutableList.of()); } return configurationMakeVariableContext; } @@ -1138,7 +1104,7 @@ public final class RuleContext extends TargetContext */ public String expandedMakeVariables(String attributeName) { String expression = attributes().get(attributeName, Type.STRING); - return expandMakeVariables(attributeName, expression, getConfigurationMakeVariableContext()); + return expandMakeVariables(attributeName, expression); } /** @@ -1181,8 +1147,7 @@ public final class RuleContext extends TargetContext public List expandedMakeVariablesList(String attrName) { List variables = new ArrayList<>(); for (String variable : attributes().get(attrName, Type.STRING_LIST)) { - variables.add( - expandMakeVariables(attrName, variable, getConfigurationMakeVariableContext())); + variables.add(expandMakeVariables(attrName, variable)); } return variables; } @@ -1194,16 +1159,13 @@ public final class RuleContext extends TargetContext * @param attrName the name of the attribute from which "expression" comes; used for error * reporting. * @param expression the string to expand. - * @param makeVariableContext the make variable context * @return the expansion of "expression", or null. */ @Nullable - public String expandSingleMakeVariable( - String attrName, - String expression, - ConfigurationMakeVariableContext makeVariableContext) { + public String expandSingleMakeVariable(String attrName, String expression) { try { - return MakeVariableExpander.expandSingleVariable(expression, makeVariableContext); + return MakeVariableExpander + .expandSingleVariable(expression, getConfigurationMakeVariableContext()); } catch (MakeVariableExpander.ExpansionException e) { attributeError(attrName, e.getMessage()); return expression; diff --git a/src/main/java/com/google/devtools/build/lib/analysis/RunfilesSupport.java b/src/main/java/com/google/devtools/build/lib/analysis/RunfilesSupport.java index d8cc52f7c2..7f4c12d14a 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/RunfilesSupport.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/RunfilesSupport.java @@ -395,10 +395,7 @@ public final class RunfilesSupport { ruleContext, executable, runfiles, - computeArgs( - ruleContext, - CommandLine.EMPTY, - ruleContext.getConfigurationMakeVariableContext())); + computeArgs(ruleContext, CommandLine.EMPTY)); } /** @@ -411,10 +408,7 @@ public final class RunfilesSupport { ruleContext, executable, runfiles, - computeArgs( - ruleContext, - CommandLine.of(appendingArgs), - ruleContext.getConfigurationMakeVariableContext())); + computeArgs(ruleContext, CommandLine.of(appendingArgs))); } /** @@ -427,26 +421,13 @@ public final class RunfilesSupport { ruleContext, executable, runfiles, - computeArgs(ruleContext, appendingArgs, ruleContext.getConfigurationMakeVariableContext())); - } - - public static RunfilesSupport withExecutable( - RuleContext ruleContext, - Runfiles runfiles, - Artifact executable, - ConfigurationMakeVariableContext makeVariableContext) { - return new RunfilesSupport( - ruleContext, - executable, - runfiles, - computeArgs(ruleContext, CommandLine.EMPTY, makeVariableContext)); + computeArgs(ruleContext, appendingArgs)); } private static CommandLine computeArgs( RuleContext ruleContext, - CommandLine additionalArgs, - ConfigurationMakeVariableContext makeVariableContext) { + CommandLine additionalArgs) { return CommandLine.concat( - ruleContext.getTokenizedStringListAttr("args", makeVariableContext), additionalArgs); + ruleContext.getTokenizedStringListAttr("args"), additionalArgs); } } diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcBinary.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcBinary.java index b341c4b825..742e5dd37c 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcBinary.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcBinary.java @@ -45,6 +45,7 @@ import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder; import com.google.devtools.build.lib.collect.nestedset.Order; import com.google.devtools.build.lib.packages.TargetUtils; import com.google.devtools.build.lib.rules.apple.ApplePlatform; +import com.google.devtools.build.lib.rules.cpp.CcCommon.CcFlagsSupplier; import com.google.devtools.build.lib.rules.cpp.CcLibraryHelper.Info; import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.FeatureConfiguration; import com.google.devtools.build.lib.rules.cpp.CppConfiguration.DynamicMode; @@ -166,6 +167,8 @@ public abstract class CcBinary implements RuleConfiguredTargetFactory { public static ConfiguredTarget init(CppSemantics semantics, RuleContext ruleContext, boolean fake) throws InterruptedException, RuleErrorException { ruleContext.checkSrcsSamePackage(true); + ruleContext.initConfigurationMakeVariableContext(new CcFlagsSupplier(ruleContext)); + CcCommon common = new CcCommon(ruleContext); CcToolchainProvider ccToolchain = common.getToolchain(); FdoSupportProvider fdoSupport = common.getFdoSupport(); diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCommon.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCommon.java index 602b5d4ed6..36278ef9c9 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCommon.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCommon.java @@ -150,8 +150,6 @@ public final class CcCommon { } public ImmutableList getCopts() { - Preconditions.checkState(hasAttribute("copts", Type.STRING_LIST)); - if (!getCoptsFilter(ruleContext).apply("-Wno-future-warnings")) { ruleContext.attributeWarning( "nocopts", @@ -164,8 +162,8 @@ public final class CcCommon { } return ImmutableList.builder() - .addAll(getPackageCopts(ruleContext)) - .addAll(CppHelper.getAttributeCopts(ruleContext, "copts")) + .addAll(CppHelper.getPackageCopts(ruleContext)) + .addAll(CppHelper.getAttributeCopts(ruleContext)) .build(); } @@ -336,11 +334,6 @@ public final class CcCommon { } } - private static ImmutableList getPackageCopts(RuleContext ruleContext) { - List unexpanded = ruleContext.getRule().getPackage().getDefaultCopts(); - return ImmutableList.copyOf(CppHelper.expandMakeVariables(ruleContext, "copts", unexpanded)); - } - /** Returns copts filter built from the make variable expanded nocopts attribute. */ Predicate getCoptsFilter() { return getCoptsFilter(ruleContext); diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibrary.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibrary.java index 0b4c2dfef2..96681370bd 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibrary.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibrary.java @@ -34,6 +34,7 @@ import com.google.devtools.build.lib.packages.AttributeMap; import com.google.devtools.build.lib.packages.BuildType; import com.google.devtools.build.lib.packages.ImplicitOutputsFunction; import com.google.devtools.build.lib.packages.RawAttributeMapper; +import com.google.devtools.build.lib.rules.cpp.CcCommon.CcFlagsSupplier; import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.FeatureConfiguration; import com.google.devtools.build.lib.rules.cpp.Link.LinkTargetType; import com.google.devtools.build.lib.rules.cpp.LinkerInputs.LibraryToLink; @@ -104,6 +105,8 @@ public abstract class CcLibrary implements RuleConfiguredTargetFactory { boolean linkStatic, boolean addDynamicRuntimeInputArtifactsToRunfiles) throws RuleErrorException, InterruptedException { + ruleContext.initConfigurationMakeVariableContext(new CcFlagsSupplier(ruleContext)); + final CcCommon common = new CcCommon(ruleContext); CcToolchainProvider ccToolchain = common.getToolchain(); FdoSupportProvider fdoSupport = common.getFdoSupport(); diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppHelper.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppHelper.java index feaad45b72..5bc125633e 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppHelper.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppHelper.java @@ -25,9 +25,7 @@ import com.google.devtools.build.lib.actions.ActionOwner; import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.MiddlemanFactory; import com.google.devtools.build.lib.analysis.AnalysisUtils; -import com.google.devtools.build.lib.analysis.ConfigurationMakeVariableContext; import com.google.devtools.build.lib.analysis.FileProvider; -import com.google.devtools.build.lib.analysis.MakeVariableSupplier; import com.google.devtools.build.lib.analysis.PlatformConfiguration; import com.google.devtools.build.lib.analysis.RuleConfiguredTargetBuilder; import com.google.devtools.build.lib.analysis.RuleContext; @@ -46,7 +44,6 @@ import com.google.devtools.build.lib.collect.nestedset.NestedSet; import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder; import com.google.devtools.build.lib.collect.nestedset.Order; import com.google.devtools.build.lib.packages.RuleErrorConsumer; -import com.google.devtools.build.lib.rules.cpp.CcCommon.CcFlagsSupplier; import com.google.devtools.build.lib.rules.cpp.CcLinkParams.Linkstamp; import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.FeatureConfiguration; import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.Tool; @@ -149,32 +146,25 @@ public class CppHelper { * @return a list of strings containing the expanded and tokenized values for the * attribute */ - // TODO(bazel-team): Move to CcCommon; refactor CcPlugin to use either CcLibraryHelper or - // CcCommon. - static List expandMakeVariables( + private static List expandMakeVariables( RuleContext ruleContext, String attributeName, List input) { boolean tokenization = !ruleContext.getFeatures().contains("no_copts_tokenization"); List tokens = new ArrayList<>(); - ImmutableList makeVariableSuppliers = - ImmutableList.of(new CcFlagsSupplier(ruleContext)); - ConfigurationMakeVariableContext makeVariableContext = - ruleContext.getConfigurationMakeVariableContext(makeVariableSuppliers); for (String token : input) { try { // Legacy behavior: tokenize all items. if (tokenization) { - ruleContext.tokenizeAndExpandMakeVars( - tokens, attributeName, token, makeVariableContext); + ruleContext.tokenizeAndExpandMakeVars(tokens, attributeName, token); } else { String exp = - ruleContext.expandSingleMakeVariable(attributeName, token, makeVariableContext); + ruleContext.expandSingleMakeVariable(attributeName, token); if (exp != null) { ShellUtils.tokenize(tokens, exp); } else { tokens.add( - ruleContext.expandMakeVariables(attributeName, token, makeVariableContext)); + ruleContext.expandMakeVariables(attributeName, token)); } } } catch (ShellUtils.TokenizationException e) { @@ -185,15 +175,22 @@ public class CppHelper { } /** - * Appends the tokenized values of the copts attribute to copts. + * Returns the tokenized values of the copts attribute to copts. */ - public static ImmutableList getAttributeCopts(RuleContext ruleContext, String attr) { + // Called from CcCommon and CcSupport (Google's internal version of proto_library). + public static ImmutableList getAttributeCopts(RuleContext ruleContext) { + String attr = "copts"; Preconditions.checkArgument(ruleContext.getRule().isAttrDefined(attr, Type.STRING_LIST)); List unexpanded = ruleContext.attributes().get(attr, Type.STRING_LIST); - return ImmutableList.copyOf(expandMakeVariables(ruleContext, attr, unexpanded)); } + // Called from CcCommon. + static ImmutableList getPackageCopts(RuleContext ruleContext) { + List unexpanded = ruleContext.getRule().getPackage().getDefaultCopts(); + return ImmutableList.copyOf(expandMakeVariables(ruleContext, "copts", unexpanded)); + } + /** * Expands attribute value either using label expansion * (if attemptLabelExpansion == {@code true} and it does not look like make @@ -202,9 +199,6 @@ public class CppHelper { public static List expandLinkopts( RuleContext ruleContext, String attrName, Iterable values) { List result = new ArrayList<>(); - ConfigurationMakeVariableContext makeVariableContext = - ruleContext.getConfigurationMakeVariableContext( - ImmutableList.of(new CcFlagsSupplier(ruleContext))); for (String value : values) { if (isLinkoptLabel(value)) { if (!expandLabel(ruleContext, result, value)) { @@ -215,8 +209,7 @@ public class CppHelper { .tokenizeAndExpandMakeVars( result, attrName, - value, - makeVariableContext); + value); } } return result; diff --git a/src/main/java/com/google/devtools/build/lib/rules/python/PyBinary.java b/src/main/java/com/google/devtools/build/lib/rules/python/PyBinary.java index e1e779a13b..56c5185dc0 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/python/PyBinary.java +++ b/src/main/java/com/google/devtools/build/lib/rules/python/PyBinary.java @@ -13,7 +13,6 @@ // limitations under the License. package com.google.devtools.build.lib.rules.python; -import com.google.common.collect.ImmutableList; import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.analysis.ConfiguredTarget; import com.google.devtools.build.lib.analysis.RuleConfiguredTargetBuilder; @@ -57,6 +56,7 @@ public abstract class PyBinary implements RuleConfiguredTargetFactory { static RuleConfiguredTargetBuilder init(RuleContext ruleContext, PythonSemantics semantics, PyCommon common) throws InterruptedException { + ruleContext.initConfigurationMakeVariableContext(new CcFlagsSupplier(ruleContext)); CcLinkParamsStore ccLinkParamsStore = initializeCcLinkParamStore(ruleContext); List srcs = common.validateSrcs(); @@ -91,9 +91,7 @@ public abstract class PyBinary implements RuleConfiguredTargetFactory { RunfilesSupport.withExecutable( ruleContext, defaultRunfiles, - common.getExecutable(), - ruleContext.getConfigurationMakeVariableContext( - ImmutableList.of(new CcFlagsSupplier(ruleContext)))); + common.getExecutable()); if (ruleContext.hasErrors()) { return null; -- cgit v1.2.3