Change CommandHelper to use TemplateExpander directly This is a partial rollback of https://github.com/bazelbuild/bazel/commit/e8d32b7c922f65539b74357711d5ad6b70934115, only the CommandHelper change. Progress on #2475. PiperOrigin-RevId: 179413908
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 85dfc7a..cd7a8f3 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
@@ -24,6 +24,9 @@ import com.google.devtools.build.lib.actions.ExecutionRequirements; import com.google.devtools.build.lib.actions.RunfilesSupplier; import com.google.devtools.build.lib.analysis.actions.FileWriteAction; +import com.google.devtools.build.lib.analysis.stringtemplate.ExpansionException; +import com.google.devtools.build.lib.analysis.stringtemplate.TemplateContext; +import com.google.devtools.build.lib.analysis.stringtemplate.TemplateExpander; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder; import com.google.devtools.build.lib.syntax.SkylarkList; @@ -163,24 +166,26 @@ } /** - * Resolves a command, and expands known locations for $(location) - * variables. + * 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; - if (allowDataInLabel) { - expander = LocationExpander.withExecPathsAndData(ruleContext, labelMap); - } else { - expander = LocationExpander.withExecPaths(ruleContext, labelMap); + public String expandForSkylark( + String command, @Nullable String attribute, + TemplateContext templateContext, boolean expandLocations) { + try { + if (expandLocations) { + templateContext = new LocationTemplateContext( + templateContext, ruleContext, labelMap, true, false); + } + return TemplateExpander.expand(command, templateContext); + } catch (ExpansionException e) { + if (attribute != null) { + ruleContext.attributeError(attribute, e.getMessage()); + } else { + ruleContext.ruleError(e.getMessage()); + } + return command; } - if (attribute != null) { - command = expander.expandAttribute(attribute, command); - } else { - command = expander.expand(command); - } - return command; } /**
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleContext.java b/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleContext.java index 20bb7b8..7887eea 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleContext.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleContext.java
@@ -39,6 +39,7 @@ import com.google.devtools.build.lib.analysis.config.FragmentCollection; import com.google.devtools.build.lib.analysis.configuredtargets.RuleConfiguredTarget.Mode; import com.google.devtools.build.lib.analysis.stringtemplate.ExpansionException; +import com.google.devtools.build.lib.analysis.stringtemplate.TemplateContext; import com.google.devtools.build.lib.analysis.test.InstrumentedFilesCollector; import com.google.devtools.build.lib.analysis.test.InstrumentedFilesProvider; import com.google.devtools.build.lib.cmdline.Label; @@ -1117,27 +1118,30 @@ + "Additional variables may come from other places, such as configurations. Note that " + "this function is experimental.") public String expandMakeVariables(String attributeName, String command, - final Map<String, String> additionalSubstitutions) throws EvalException { + Map<String, String> additionalSubstitutions) throws EvalException { checkMutable("expand_make_variables"); - ConfigurationMakeVariableContext makeVariableContext = - new ConfigurationMakeVariableContext( - // TODO(lberki): This should be removed. But only after either verifying that no one - // uses it or providing an alternative. - ruleContext.getMakeVariables(ImmutableList.of(":cc_toolchain")), - ruleContext.getRule().getPackage(), - ruleContext.getConfiguration()) { - @Override - public String lookupVariable(String variableName) throws ExpansionException { - if (additionalSubstitutions.containsKey(variableName)) { - return additionalSubstitutions.get(variableName); - } else { - return super.lookupVariable(variableName); - } - } - }; - return ruleContext.getExpander(makeVariableContext).expand(attributeName, command); + TemplateContext templateContext = getConfigurationMakeVariableContext(additionalSubstitutions); + return ruleContext.getExpander(templateContext).expand(attributeName, command); } + TemplateContext getConfigurationMakeVariableContext( + final Map<String, String> additionalSubstitutions) { + return new ConfigurationMakeVariableContext( + // TODO(lberki): This should be removed. But only after either verifying that no one + // uses it or providing an alternative. + ruleContext.getMakeVariables(ImmutableList.of(":cc_toolchain")), + ruleContext.getRule().getPackage(), + ruleContext.getConfiguration()) { + @Override + public String lookupVariable(String variableName) throws ExpansionException { + if (additionalSubstitutions.containsKey(variableName)) { + return additionalSubstitutions.get(variableName); + } else { + return super.lookupVariable(variableName); + } + } + }; + } FilesToRunProvider getExecutableRunfiles(Artifact executable) { return attributesCollection.getExecutableRunfilesMap().get(executable);
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleImplementationFunctions.java b/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleImplementationFunctions.java index 5a0ea68..eccd577 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleImplementationFunctions.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleImplementationFunctions.java
@@ -25,6 +25,7 @@ import com.google.devtools.build.lib.analysis.RunfilesProvider; import com.google.devtools.build.lib.analysis.TransitiveInfoCollection; import com.google.devtools.build.lib.analysis.configuredtargets.AbstractConfiguredTarget; +import com.google.devtools.build.lib.analysis.stringtemplate.TemplateContext; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.events.Location; import com.google.devtools.build.lib.skylarkinterface.Param; @@ -709,15 +710,13 @@ ImmutableMap.copyOf(labelDict)); String attribute = Type.STRING.convertOptional(attributeUnchecked, "attribute", ruleLabel); - if (expandLocations) { - command = helper.resolveCommandAndExpandLabels( - command, attribute, /*allowDataInLabel=*/false); - } + TemplateContext templateContext = TemplateContext.EMPTY; if (!EvalUtils.isNullOrNone(makeVariablesUnchecked)) { Map<String, String> makeVariables = Type.STRING_DICT.convert(makeVariablesUnchecked, "make_variables", ruleLabel); - command = ctx.expandMakeVariables(attribute, command, makeVariables); + templateContext = ctx.getConfigurationMakeVariableContext(makeVariables); } + command = helper.expandForSkylark(command, attribute, templateContext, expandLocations); List<Artifact> inputs = new ArrayList<>(); inputs.addAll(helper.getResolvedTools());
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/stringtemplate/TemplateContext.java b/src/main/java/com/google/devtools/build/lib/analysis/stringtemplate/TemplateContext.java index dc6d275..179af87 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/stringtemplate/TemplateContext.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/stringtemplate/TemplateContext.java
@@ -18,6 +18,17 @@ * each template variable and function. */ public interface TemplateContext { + public static final TemplateContext EMPTY = new TemplateContext() { + @Override + public String lookupVariable(String name) throws ExpansionException { + throw new ExpansionException(String.format("$(%s) not defined", name)); + } + + @Override + public String lookupFunction(String name, String param) throws ExpansionException { + throw new ExpansionException(String.format("$(%s) not defined", name)); + } + }; /** * Returns the expansion of the specified template variable.
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/LocationExpanderIntegrationTest.java b/src/test/java/com/google/devtools/build/lib/analysis/ExpanderIntegrationTest.java similarity index 74% rename from src/test/java/com/google/devtools/build/lib/analysis/LocationExpanderIntegrationTest.java rename to src/test/java/com/google/devtools/build/lib/analysis/ExpanderIntegrationTest.java index 1db06a8..382ee60 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/LocationExpanderIntegrationTest.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/ExpanderIntegrationTest.java
@@ -16,15 +16,16 @@ import static com.google.common.truth.Truth.assertThat; +import com.google.common.collect.ImmutableMap; import com.google.devtools.build.lib.analysis.util.BuildViewTestCase; import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; -/** Integration tests for {@link LocationExpander}. */ +/** Integration tests for {@link Expander}. */ @RunWith(JUnit4.class) -public class LocationExpanderIntegrationTest extends BuildViewTestCase { +public class ExpanderIntegrationTest extends BuildViewTestCase { @Before public void createFiles() throws Exception { @@ -40,10 +41,10 @@ " deps = [':files'])"); } - private LocationExpander makeExpander(String label) throws Exception { + private Expander makeExpander(String label) throws Exception { ConfiguredTarget target = getConfiguredTarget(label); RuleContext ruleContext = getRuleContext(target); - return LocationExpander.withRunfilesPaths(ruleContext); + return ruleContext.getExpander().withExecLocations(ImmutableMap.of()); } @Test @@ -57,9 +58,9 @@ "sh_library(name='lib',", " deps = [':files'])"); - LocationExpander expander = makeExpander("//spaces:lib"); + Expander expander = makeExpander("//spaces:lib"); String input = "foo $(locations :files) bar"; - String result = expander.expand(input); + String result = expander.expand(null, input); assertThat(result).isEqualTo("foo 'spaces/file with space A' 'spaces/file with space B' bar"); } @@ -71,14 +72,14 @@ "genrule(name='foo', outs=['foo.txt'], cmd='never executed')", "sh_library(name='lib', srcs=[':foo'])"); - LocationExpander expander = makeExpander("//expansion:lib"); - assertThat(expander.expand("foo $(execpath :foo) bar")) + Expander expander = makeExpander("//expansion:lib"); + assertThat(expander.expand("<attribute>", "foo $(execpath :foo) bar")) .matches("foo .*-out/.*/expansion/foo\\.txt bar"); - assertThat(expander.expand("foo $(execpaths :foo) bar")) + assertThat(expander.expand("<attribute>", "foo $(execpaths :foo) bar")) .matches("foo .*-out/.*/expansion/foo\\.txt bar"); - assertThat(expander.expand("foo $(rootpath :foo) bar")) + assertThat(expander.expand("<attribute>", "foo $(rootpath :foo) bar")) .matches("foo expansion/foo.txt bar"); - assertThat(expander.expand("foo $(rootpaths :foo) bar")) + assertThat(expander.expand("<attribute>", "foo $(rootpaths :foo) bar")) .matches("foo expansion/foo.txt bar"); } @@ -89,10 +90,10 @@ "genrule(name='foo', outs=['foo.txt', 'bar.txt'], cmd='never executed')", "sh_library(name='lib', srcs=[':foo'])"); - LocationExpander expander = makeExpander("//expansion:lib"); - assertThat(expander.expand("foo $(execpaths :foo) bar")) + Expander expander = makeExpander("//expansion:lib"); + assertThat(expander.expand("<attribute>", "foo $(execpaths :foo) bar")) .matches("foo .*-out/.*/expansion/bar\\.txt .*-out/.*/expansion/foo\\.txt bar"); - assertThat(expander.expand("foo $(rootpaths :foo) bar")) + assertThat(expander.expand("<attribute>", "foo $(rootpaths :foo) bar")) .matches("foo expansion/bar.txt expansion/foo.txt bar"); } }