Switch ExtraActionFactory and GenRuleBase to the Expander API
This changes the order in which expansions happen, which could change the
semantics in subtle ways.
PiperOrigin-RevId: 174518778
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/CommandHelper.java b/src/main/java/com/google/devtools/build/lib/analysis/CommandHelper.java
index 74d05a7..6f1c16b 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/CommandHelper.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/CommandHelper.java
@@ -159,23 +159,10 @@
}
/**
- * Resolves a command, and expands known locations for $(location) variables. This method supports
- * legacy heuristic label expansion, which replaces strings that look like labels with their
- * corresponding file names. Use {@link #resolveCommandAndExpandLabels} instead.
- */
- public String resolveCommandAndHeuristicallyExpandLabels(
- String command, @Nullable String attribute, boolean enableLegacyHeuristicLabelExpansion) {
- command = resolveCommandAndExpandLabels(command, attribute, false);
- if (enableLegacyHeuristicLabelExpansion) {
- command = expandLabels(command, labelMap);
- }
- return command;
- }
-
- /**
* Resolves a command, and expands known locations for $(location)
* variables.
*/
+ @Deprecated // Only exists to support a legacy Skylark API.
public String resolveCommandAndExpandLabels(
String command, @Nullable String attribute, boolean allowDataInLabel) {
LocationExpander expander;
@@ -201,7 +188,7 @@
* <p>If the expansion fails, an attribute error is reported and the original
* expression is returned.
*/
- private <T extends Iterable<Artifact>> String expandLabels(String expr, Map<Label, T> labelMap) {
+ public String expandLabelsHeuristically(String expr) {
try {
return LabelExpander.expand(expr, labelMap, ruleContext.getLabel());
} catch (LabelExpander.NotUniqueExpansionException nuee) {
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/Expander.java b/src/main/java/com/google/devtools/build/lib/analysis/Expander.java
index 9dc224a..e22ac2c 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/Expander.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/Expander.java
@@ -73,6 +73,14 @@
}
/**
+ * Returns a new instance that also expands locations, passing {@link Options#EXEC_PATHS} to the
+ * underlying {@link LocationTemplateContext}.
+ */
+ public Expander withExecLocations() {
+ return withLocations(Options.EXEC_PATHS);
+ }
+
+ /**
* Returns a new instance that also expands locations, passing the given location map, as well as
* {@link Options#EXEC_PATHS} to the underlying {@link LocationTemplateContext}.
*/
diff --git a/src/main/java/com/google/devtools/build/lib/rules/extra/ExtraActionFactory.java b/src/main/java/com/google/devtools/build/lib/rules/extra/ExtraActionFactory.java
index 47411e1..1eb434e 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/extra/ExtraActionFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/extra/ExtraActionFactory.java
@@ -54,8 +54,7 @@
List<String>outputTemplates =
context.attributes().get("out_templates", Type.STRING_LIST);
- String command = commandHelper.resolveCommandAndExpandLabels(
- context.attributes().get("cmd", Type.STRING), "cmd", /*allowDataInLabel=*/true);
+ String command = context.attributes().get("cmd", Type.STRING);
// This is a bit of a hack. We want to run the MakeVariableExpander first, so we expand $ on
// variables that are expanded below with $$, which gets reverted to $ by the
// MakeVariableExpander. This allows us to expand package-specific make variables in the
@@ -67,7 +66,10 @@
command = command.replace("$(output ", "$$(output ");
ConfigurationMakeVariableContext makeVariableContext = new ConfigurationMakeVariableContext(
context, context.getTarget().getPackage(), context.getConfiguration());
- command = context.getExpander(makeVariableContext).expand("cmd", command);
+ command = context
+ .getExpander(makeVariableContext)
+ .withDataExecLocations()
+ .expand("cmd", command);
boolean requiresActionOutput =
context.attributes().get("requires_action_output", Type.BOOLEAN);
diff --git a/src/main/java/com/google/devtools/build/lib/rules/genrule/GenRuleBase.java b/src/main/java/com/google/devtools/build/lib/rules/genrule/GenRuleBase.java
index d4aabb4..d9104d3 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/genrule/GenRuleBase.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/genrule/GenRuleBase.java
@@ -82,18 +82,6 @@
}
/**
- * Returns an {@link Iterable} of {@link NestedSet}s, which will be added to the genrule's inputs
- * using the {@link NestedSetBuilder#addTransitive} method.
- *
- * <p>GenRule implementations can override this method to better control what inputs are needed
- * for specific command inputs.
- */
- protected Iterable<NestedSet<Artifact>> getExtraInputArtifacts(
- RuleContext ruleContext, String command) {
- return ImmutableList.of();
- }
-
- /**
* Returns {@code true} if the rule should be stamped.
*
* <p>Genrule implementations can set this based on the rule context, including by defining their
@@ -153,17 +141,22 @@
return null;
}
- String baseCommand = commandHelper.resolveCommandAndHeuristicallyExpandLabels(
- ruleContext.attributes().get("cmd", Type.STRING),
- "cmd",
- ruleContext.attributes().get("heuristic_label_expansion", Type.BOOLEAN));
+ String baseCommand = ruleContext.attributes().get("cmd", Type.STRING);
+ // Expand template variables and functions.
+ String command = ruleContext
+ .getExpander(new CommandResolverContext(ruleContext, resolvedSrcs, filesToBuild))
+ .withExecLocations()
+ .expand("cmd", baseCommand);
- // Adds the genrule environment setup script before the actual shell command
- String command = String.format("source %s; %s",
+ // Heuristically expand things that look like labels.
+ if (ruleContext.attributes().get("heuristic_label_expansion", Type.BOOLEAN)) {
+ command = commandHelper.expandLabelsHeuristically(command);
+ }
+
+ // Add the genrule environment setup script before the actual shell command.
+ command = String.format("source %s; %s",
ruleContext.getPrerequisiteArtifact("$genrule_setup", Mode.HOST).getExecPath(),
- baseCommand);
-
- command = resolveCommand(command, ruleContext, resolvedSrcs, filesToBuild);
+ command);
String messageAttr = ruleContext.attributes().get("message", Type.STRING);
String message = messageAttr.isEmpty() ? "Executing genrule" : messageAttr;
@@ -207,10 +200,6 @@
inputs.addTransitive(JavaHelper.getHostJavabaseInputs(ruleContext));
}
- for (NestedSet<Artifact> extraInputs : getExtraInputArtifacts(ruleContext, baseCommand)) {
- inputs.addTransitive(extraInputs);
- }
-
if (isStampingEnabled(ruleContext)) {
inputs.add(ruleContext.getAnalysisEnvironment().getStableWorkspaceStatusArtifact());
inputs.add(ruleContext.getAnalysisEnvironment().getVolatileWorkspaceStatusArtifact());
@@ -265,19 +254,6 @@
}
/**
- * Resolves any variables, including make and genrule-specific variables, in the command and
- * returns the expanded command.
- *
- * <p>GenRule implementations may override this method to perform additional expansions.
- */
- protected String resolveCommand(String command, final RuleContext ruleContext,
- final NestedSet<Artifact> resolvedSrcs, final NestedSet<Artifact> filesToBuild) {
- return ruleContext
- .getExpander(new CommandResolverContext(ruleContext, resolvedSrcs, filesToBuild))
- .expand("cmd", command);
- }
-
- /**
* Implementation of {@link ConfigurationMakeVariableContext} used to expand variables in a
* genrule command string.
*/
diff --git a/src/test/java/com/google/devtools/build/lib/bazel/rules/genrule/GenRuleCommandSubstitutionTest.java b/src/test/java/com/google/devtools/build/lib/bazel/rules/genrule/GenRuleCommandSubstitutionTest.java
index 8b2b1d4..bef6e1c 100644
--- a/src/test/java/com/google/devtools/build/lib/bazel/rules/genrule/GenRuleCommandSubstitutionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/bazel/rules/genrule/GenRuleCommandSubstitutionTest.java
@@ -93,7 +93,7 @@
eventCollector.clear();
genrule("$(location foo bar");
- assertExpansionFails("unterminated $(location) expression", "//test");
+ assertExpansionFails("unterminated variable reference", "//test");
genrule("$(location");
assertExpansionFails("unterminated variable reference", "//test");
@@ -221,7 +221,7 @@
eventCollector.clear();
genrule("$(locations foo bar");
- assertExpansionFails("unterminated $(locations) expression", "//test");
+ assertExpansionFails("unterminated variable reference", "//test");
genrule("$(locations");
assertExpansionFails("unterminated variable reference", "//test");
