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
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 8aaaaa4..9a114f5 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 @@
return result;
}
+ public void initConfigurationMakeVariableContext(
+ Iterable<? extends MakeVariableSupplier> 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 @@
* @return a list of strings containing the expanded and tokenized values for the attribute
*/
public ImmutableList<String> 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<String> getTokenizedStringListAttr(
- String attributeName, ConfigurationMakeVariableContext makeVariableContext) {
- return getExpandedStringListAttr(
- attributeName, Tokenize.YES, makeVariableContext);
+ return getExpandedStringListAttr(attributeName, Tokenize.YES);
}
/**
@@ -1000,8 +997,7 @@
* @return a list of strings containing the processed values for the attribute
*/
public ImmutableList<String> getExpandedStringListAttr(String attributeName) {
- return getExpandedStringListAttr(
- attributeName, Tokenize.NO, getConfigurationMakeVariableContext());
+ return getExpandedStringListAttr(attributeName, Tokenize.NO);
}
/**
@@ -1009,13 +1005,11 @@
* 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<String> 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 @@
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
- * <p>This methods should be called only during initialization.
*/
public void tokenizeAndExpandMakeVars(
List<String> 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 @@
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 @@
}
/**
- * Returns a (cached! read on) context that maps Make variable names (string) to values (string)
- * without any extra {@link MakeVariableSupplier}.
- *
- * <p>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.<MakeVariableSupplier>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<? extends MakeVariableSupplier> makeVariableSuppliers) {
if (configurationMakeVariableContext == null) {
- configurationMakeVariableContext =
- new ConfigurationMakeVariableContext(
- this, getRule().getPackage(), getConfiguration(), makeVariableSuppliers);
+ initConfigurationMakeVariableContext(ImmutableList.<MakeVariableSupplier>of());
}
return configurationMakeVariableContext;
}
@@ -1138,7 +1104,7 @@
*/
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 List<String> expandedMakeVariablesList(String attrName) {
List<String> 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 @@
* @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 d8cc52f..7f4c12d 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 @@
ruleContext,
executable,
runfiles,
- computeArgs(
- ruleContext,
- CommandLine.EMPTY,
- ruleContext.getConfigurationMakeVariableContext()));
+ computeArgs(ruleContext, CommandLine.EMPTY));
}
/**
@@ -411,10 +408,7 @@
ruleContext,
executable,
runfiles,
- computeArgs(
- ruleContext,
- CommandLine.of(appendingArgs),
- ruleContext.getConfigurationMakeVariableContext()));
+ computeArgs(ruleContext, CommandLine.of(appendingArgs)));
}
/**
@@ -427,26 +421,13 @@
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 b341c4b..742e5dd 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.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 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 602b5d4..36278ef 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 ImmutableList<String> getCopts() {
- Preconditions.checkState(hasAttribute("copts", Type.STRING_LIST));
-
if (!getCoptsFilter(ruleContext).apply("-Wno-future-warnings")) {
ruleContext.attributeWarning(
"nocopts",
@@ -164,8 +162,8 @@
}
return ImmutableList.<String>builder()
- .addAll(getPackageCopts(ruleContext))
- .addAll(CppHelper.getAttributeCopts(ruleContext, "copts"))
+ .addAll(CppHelper.getPackageCopts(ruleContext))
+ .addAll(CppHelper.getAttributeCopts(ruleContext))
.build();
}
@@ -336,11 +334,6 @@
}
}
- private static ImmutableList<String> getPackageCopts(RuleContext ruleContext) {
- List<String> 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<String> 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 0b4c2df..9668137 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.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 @@
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 feaad45..5bc1256 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.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.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 @@
* @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<String> expandMakeVariables(
+ private static List<String> expandMakeVariables(
RuleContext ruleContext, String attributeName, List<String> input) {
boolean tokenization =
!ruleContext.getFeatures().contains("no_copts_tokenization");
List<String> tokens = new ArrayList<>();
- ImmutableList<? extends MakeVariableSupplier> 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 @@
}
/**
- * Appends the tokenized values of the copts attribute to copts.
+ * Returns the tokenized values of the copts attribute to copts.
*/
- public static ImmutableList<String> getAttributeCopts(RuleContext ruleContext, String attr) {
+ // Called from CcCommon and CcSupport (Google's internal version of proto_library).
+ public static ImmutableList<String> getAttributeCopts(RuleContext ruleContext) {
+ String attr = "copts";
Preconditions.checkArgument(ruleContext.getRule().isAttrDefined(attr, Type.STRING_LIST));
List<String> unexpanded = ruleContext.attributes().get(attr, Type.STRING_LIST);
-
return ImmutableList.copyOf(expandMakeVariables(ruleContext, attr, unexpanded));
}
+ // Called from CcCommon.
+ static ImmutableList<String> getPackageCopts(RuleContext ruleContext) {
+ List<String> 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 static List<String> expandLinkopts(
RuleContext ruleContext, String attrName, Iterable<String> values) {
List<String> 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 @@
.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 e1e779a..56c5185 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 @@
static RuleConfiguredTargetBuilder init(RuleContext ruleContext, PythonSemantics semantics,
PyCommon common) throws InterruptedException {
+ ruleContext.initConfigurationMakeVariableContext(new CcFlagsSupplier(ruleContext));
CcLinkParamsStore ccLinkParamsStore = initializeCcLinkParamStore(ruleContext);
List<Artifact> srcs = common.validateSrcs();
@@ -91,9 +91,7 @@
RunfilesSupport.withExecutable(
ruleContext,
defaultRunfiles,
- common.getExecutable(),
- ruleContext.getConfigurationMakeVariableContext(
- ImmutableList.of(new CcFlagsSupplier(ruleContext))));
+ common.getExecutable());
if (ruleContext.hasErrors()) {
return null;
