From 2f10da0db062e023b1f0f8222f8545467b29ae4e Mon Sep 17 00:00:00 2001 From: ulfjack Date: Mon, 18 Dec 2017 06:09:06 -0800 Subject: 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 --- .../devtools/build/lib/analysis/CommandHelper.java | 35 ++++---- .../lib/analysis/skylark/SkylarkRuleContext.java | 40 +++++---- .../SkylarkRuleImplementationFunctions.java | 9 +- .../analysis/stringtemplate/TemplateContext.java | 11 +++ .../lib/analysis/ExpanderIntegrationTest.java | 99 ++++++++++++++++++++++ .../analysis/LocationExpanderIntegrationTest.java | 98 --------------------- 6 files changed, 156 insertions(+), 136 deletions(-) create mode 100644 src/test/java/com/google/devtools/build/lib/analysis/ExpanderIntegrationTest.java delete mode 100644 src/test/java/com/google/devtools/build/lib/analysis/LocationExpanderIntegrationTest.java (limited to 'src') 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 85dfc7a043..cd7a8f38b7 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.Artifact; 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 @@ public final class CommandHelper { } /** - * 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); - } - if (attribute != null) { - command = expander.expandAttribute(attribute, command); - } else { - command = expander.expand(command); + 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; } - 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 20bb7b895a..7887eea3d2 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.BuildConfiguration; 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 @@ public final class SkylarkRuleContext implements SkylarkValue { + "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 additionalSubstitutions) throws EvalException { + Map 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 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 5a0ea681bd..eccd577659 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.Runfiles; 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 @@ public class SkylarkRuleImplementationFunctions { 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 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 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 dc6d27598d..179af8746a 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 @@ package com.google.devtools.build.lib.analysis.stringtemplate; * 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/ExpanderIntegrationTest.java b/src/test/java/com/google/devtools/build/lib/analysis/ExpanderIntegrationTest.java new file mode 100644 index 0000000000..382ee60bc5 --- /dev/null +++ b/src/test/java/com/google/devtools/build/lib/analysis/ExpanderIntegrationTest.java @@ -0,0 +1,99 @@ +// Copyright 2017 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.devtools.build.lib.analysis; + +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 Expander}. */ +@RunWith(JUnit4.class) +public class ExpanderIntegrationTest extends BuildViewTestCase { + + @Before + public void createFiles() throws Exception { + // Set up a rule to test expansion in. + scratch.file("files/fileA"); + scratch.file("files/fileB"); + + scratch.file( + "files/BUILD", + "filegroup(name='files',", + " srcs = ['fileA', 'fileB'])", + "sh_library(name='lib',", + " deps = [':files'])"); + } + + private Expander makeExpander(String label) throws Exception { + ConfiguredTarget target = getConfiguredTarget(label); + RuleContext ruleContext = getRuleContext(target); + return ruleContext.getExpander().withExecLocations(ImmutableMap.of()); + } + + @Test + public void locations_spaces() throws Exception { + scratch.file("spaces/file with space A"); + scratch.file("spaces/file with space B"); + scratch.file( + "spaces/BUILD", + "filegroup(name='files',", + " srcs = ['file with space A', 'file with space B'])", + "sh_library(name='lib',", + " deps = [':files'])"); + + Expander expander = makeExpander("//spaces:lib"); + String input = "foo $(locations :files) bar"; + String result = expander.expand(null, input); + + assertThat(result).isEqualTo("foo 'spaces/file with space A' 'spaces/file with space B' bar"); + } + + @Test + public void otherPathExpansion() throws Exception { + scratch.file( + "expansion/BUILD", + "genrule(name='foo', outs=['foo.txt'], cmd='never executed')", + "sh_library(name='lib', srcs=[':foo'])"); + + Expander expander = makeExpander("//expansion:lib"); + assertThat(expander.expand("", "foo $(execpath :foo) bar")) + .matches("foo .*-out/.*/expansion/foo\\.txt bar"); + assertThat(expander.expand("", "foo $(execpaths :foo) bar")) + .matches("foo .*-out/.*/expansion/foo\\.txt bar"); + assertThat(expander.expand("", "foo $(rootpath :foo) bar")) + .matches("foo expansion/foo.txt bar"); + assertThat(expander.expand("", "foo $(rootpaths :foo) bar")) + .matches("foo expansion/foo.txt bar"); + } + + @Test + public void otherPathMultiExpansion() throws Exception { + scratch.file( + "expansion/BUILD", + "genrule(name='foo', outs=['foo.txt', 'bar.txt'], cmd='never executed')", + "sh_library(name='lib', srcs=[':foo'])"); + + Expander expander = makeExpander("//expansion:lib"); + assertThat(expander.expand("", "foo $(execpaths :foo) bar")) + .matches("foo .*-out/.*/expansion/bar\\.txt .*-out/.*/expansion/foo\\.txt bar"); + assertThat(expander.expand("", "foo $(rootpaths :foo) bar")) + .matches("foo expansion/bar.txt expansion/foo.txt bar"); + } +} diff --git a/src/test/java/com/google/devtools/build/lib/analysis/LocationExpanderIntegrationTest.java b/src/test/java/com/google/devtools/build/lib/analysis/LocationExpanderIntegrationTest.java deleted file mode 100644 index 1db06a8883..0000000000 --- a/src/test/java/com/google/devtools/build/lib/analysis/LocationExpanderIntegrationTest.java +++ /dev/null @@ -1,98 +0,0 @@ -// Copyright 2017 The Bazel Authors. All rights reserved. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package com.google.devtools.build.lib.analysis; - -import static com.google.common.truth.Truth.assertThat; - -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}. */ -@RunWith(JUnit4.class) -public class LocationExpanderIntegrationTest extends BuildViewTestCase { - - @Before - public void createFiles() throws Exception { - // Set up a rule to test expansion in. - scratch.file("files/fileA"); - scratch.file("files/fileB"); - - scratch.file( - "files/BUILD", - "filegroup(name='files',", - " srcs = ['fileA', 'fileB'])", - "sh_library(name='lib',", - " deps = [':files'])"); - } - - private LocationExpander makeExpander(String label) throws Exception { - ConfiguredTarget target = getConfiguredTarget(label); - RuleContext ruleContext = getRuleContext(target); - return LocationExpander.withRunfilesPaths(ruleContext); - } - - @Test - public void locations_spaces() throws Exception { - scratch.file("spaces/file with space A"); - scratch.file("spaces/file with space B"); - scratch.file( - "spaces/BUILD", - "filegroup(name='files',", - " srcs = ['file with space A', 'file with space B'])", - "sh_library(name='lib',", - " deps = [':files'])"); - - LocationExpander expander = makeExpander("//spaces:lib"); - String input = "foo $(locations :files) bar"; - String result = expander.expand(input); - - assertThat(result).isEqualTo("foo 'spaces/file with space A' 'spaces/file with space B' bar"); - } - - @Test - public void otherPathExpansion() throws Exception { - scratch.file( - "expansion/BUILD", - "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")) - .matches("foo .*-out/.*/expansion/foo\\.txt bar"); - assertThat(expander.expand("foo $(execpaths :foo) bar")) - .matches("foo .*-out/.*/expansion/foo\\.txt bar"); - assertThat(expander.expand("foo $(rootpath :foo) bar")) - .matches("foo expansion/foo.txt bar"); - assertThat(expander.expand("foo $(rootpaths :foo) bar")) - .matches("foo expansion/foo.txt bar"); - } - - @Test - public void otherPathMultiExpansion() throws Exception { - scratch.file( - "expansion/BUILD", - "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")) - .matches("foo .*-out/.*/expansion/bar\\.txt .*-out/.*/expansion/foo\\.txt bar"); - assertThat(expander.expand("foo $(rootpaths :foo) bar")) - .matches("foo expansion/bar.txt expansion/foo.txt bar"); - } -} -- cgit v1.2.3