From 31e4479a98a31bba4dab5b5c836328515ab0af6e Mon Sep 17 00:00:00 2001 From: gregce Date: Mon, 27 Apr 2020 09:10:40 -0700 Subject: [PATCH] cquery --show_config_fragments: capture Make variables. Any Make variable "$(foo)" is equivalent to requiring "--define foo". This works for native rule logic and Starlark rules routing through https://docs.bazel.build/versions/master/skylark/lib/ctx.html#expand_make_variables, but not Starlark rules routing through https://docs.bazel.build/versions/master/skylark/lib/ctx.html#var. Supports https://github.com/bazelbuild/bazel/issues/10613. Added refactoring TODO in https://github.com/bazelbuild/bazel/issues/11221. PiperOrigin-RevId: 308632503 --- .../devtools/build/lib/analysis/Expander.java | 33 ++++- .../build/lib/analysis/RuleContext.java | 36 ++++- .../build/lib/analysis/stringtemplate/BUILD | 4 + .../analysis/stringtemplate/Expansion.java | 32 +++++ .../stringtemplate/TemplateExpander.java | 39 ++--- .../proto/ProtoCompileActionBuilder.java | 35 ++--- .../analysis/RequiredConfigFragmentsTest.java | 135 ++++++++++++++++++ .../analysis/RuleConfiguredTargetTest.java | 45 ------ .../stringtemplate/TemplateExpanderTest.java | 58 +++++--- 9 files changed, 310 insertions(+), 107 deletions(-) create mode 100644 src/main/java/com/google/devtools/build/lib/analysis/stringtemplate/Expansion.java create mode 100644 src/test/java/com/google/devtools/build/lib/analysis/RequiredConfigFragmentsTest.java 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 91c72719ddaf99..9cbdeff61931e9 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 @@ -16,7 +16,9 @@ import com.google.common.collect.ImmutableCollection; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; +import com.google.common.collect.ImmutableSortedSet; import com.google.devtools.build.lib.actions.Artifact; +import com.google.devtools.build.lib.analysis.stringtemplate.Expansion; 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; @@ -25,6 +27,7 @@ import com.google.devtools.build.lib.shell.ShellUtils; import java.util.ArrayList; import java.util.List; +import java.util.TreeSet; import javax.annotation.Nullable; /** @@ -35,6 +38,8 @@ public final class Expander { private final RuleContext ruleContext; private final TemplateContext templateContext; @Nullable ImmutableMap> labelMap; + /* Which variables were looked up over this instance's lifetime? */ + private final TreeSet lookedUpVariables; Expander(RuleContext ruleContext, TemplateContext templateContext) { this(ruleContext, templateContext, /* labelMap= */ null); @@ -44,9 +49,20 @@ public final class Expander { RuleContext ruleContext, TemplateContext templateContext, @Nullable ImmutableMap> labelMap) { + this(ruleContext, templateContext, labelMap, /*lookedUpVariables=*/ null); + } + + Expander( + RuleContext ruleContext, + TemplateContext templateContext, + @Nullable ImmutableMap> labelMap, + @Nullable TreeSet lookedUpVariables) { this.ruleContext = ruleContext; this.templateContext = templateContext; this.labelMap = labelMap; + // TODO(https://github.com/bazelbuild/bazel/issues/11221): Eliminate all methods that construct + // an Expander from an existing Expander. These make it hard to keep lookeduUpVariables correct. + this.lookedUpVariables = lookedUpVariables == null ? new TreeSet<>() : lookedUpVariables; } /** @@ -57,7 +73,7 @@ private Expander withLocations(boolean execPaths, boolean allowData) { TemplateContext newTemplateContext = new LocationTemplateContext( templateContext, ruleContext, labelMap, execPaths, allowData, false); - return new Expander(ruleContext, newTemplateContext, labelMap); + return new Expander(ruleContext, newTemplateContext, labelMap, lookedUpVariables); } /** @@ -85,7 +101,7 @@ public Expander withExecLocations( TemplateContext newTemplateContext = new LocationTemplateContext( templateContext, ruleContext, locations, true, false, windowsPath); - return new Expander(ruleContext, newTemplateContext); + return new Expander(ruleContext, newTemplateContext, labelMap, lookedUpVariables); } public Expander withExecLocations(ImmutableMap> locations) { @@ -142,7 +158,9 @@ public String expand(String attributeName) { */ public String expand(@Nullable String attributeName, String expression) { try { - return TemplateExpander.expand(expression, templateContext); + Expansion expansion = TemplateExpander.expand(expression, templateContext); + lookedUpVariables.addAll(expansion.lookedUpVariables()); + return expansion.expansion(); } catch (ExpansionException e) { if (attributeName == null) { ruleContext.ruleError(e.getMessage()); @@ -216,4 +234,13 @@ public String expandSingleMakeVariable(String attrName, String expression) { return expression; } } + + /** + * Which variables were looked up over this {@link Expander}'s lifetime? + * + *

The returned set is guaranteed alphabetically ordered. + */ + public ImmutableSortedSet lookedUpVariables() { + return ImmutableSortedSet.copyOf(lookedUpVariables); + } } 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 0bccc31037ef2b..a68be1d2bcd3a5 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 @@ -99,6 +99,7 @@ import com.google.devtools.build.lib.util.StringUtil; import com.google.devtools.build.lib.vfs.FileSystemUtils; import com.google.devtools.build.lib.vfs.PathFragment; +import java.util.ArrayList; import java.util.Collection; import java.util.HashMap; import java.util.HashSet; @@ -201,6 +202,7 @@ public ImmutableList getFiles() { @Nullable private final ToolchainCollection toolchainContexts; private final ConstraintSemantics constraintSemantics; private final ImmutableSet requiredConfigFragments; + private final List makeVariableExpanders = new ArrayList<>(); private ActionOwner actionOwner; private final SymbolGenerator actionOwnerSymbolGenerator; @@ -1200,15 +1202,21 @@ public void initConfigurationMakeVariableContext(MakeVariableSupplier... makeVar } public Expander getExpander(TemplateContext templateContext) { - return new Expander(this, templateContext); + Expander expander = new Expander(this, templateContext); + makeVariableExpanders.add(expander); + return expander; } public Expander getExpander() { - return new Expander(this, getConfigurationMakeVariableContext()); + Expander expander = new Expander(this, getConfigurationMakeVariableContext()); + makeVariableExpanders.add(expander); + return expander; } public Expander getExpander(ImmutableMap> labelMap) { - return new Expander(this, getConfigurationMakeVariableContext(), labelMap); + Expander expander = new Expander(this, getConfigurationMakeVariableContext(), labelMap); + makeVariableExpanders.add(expander); + return expander; } /** @@ -1250,8 +1258,26 @@ public ConstraintSemantics getConstraintSemantics() { return constraintSemantics; } - public ImmutableSet getRequiredConfigFragments() { - return requiredConfigFragments; + /** + * Returns the configuration fragments this rule uses. + * + *

Returned results are alphabetically ordered. + */ + public ImmutableSortedSet getRequiredConfigFragments() { + ImmutableSortedSet.Builder ans = ImmutableSortedSet.naturalOrder(); + ans.addAll(requiredConfigFragments); + for (Expander makeVariableExpander : makeVariableExpanders) { + for (String makeVariable : makeVariableExpander.lookedUpVariables()) { + // User-defined make values may be set either in "--define foo=bar" or in a vardef in the + // rule's package. Both are equivalent for these purposes, since in both cases setting + // "--define foo=bar" impacts the rule's output. + if (getRule().getPackage().getMakeEnvironment().containsKey(makeVariable) + || getConfiguration().getCommandLineBuildVariables().containsKey(makeVariable)) { + ans.add("--define:" + makeVariable); + } + } + } + return ans.build(); } public Map getTargetExecProperties() { diff --git a/src/main/java/com/google/devtools/build/lib/analysis/stringtemplate/BUILD b/src/main/java/com/google/devtools/build/lib/analysis/stringtemplate/BUILD index ade8a86c4ab1d7..363969121cf7fc 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/stringtemplate/BUILD +++ b/src/main/java/com/google/devtools/build/lib/analysis/stringtemplate/BUILD @@ -11,4 +11,8 @@ filegroup( java_library( name = "stringtemplate", srcs = glob(["*.java"]), + deps = [ + "//third_party:auto_value", + "//third_party:guava", + ], ) diff --git a/src/main/java/com/google/devtools/build/lib/analysis/stringtemplate/Expansion.java b/src/main/java/com/google/devtools/build/lib/analysis/stringtemplate/Expansion.java new file mode 100644 index 00000000000000..46960d58be3a10 --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/analysis/stringtemplate/Expansion.java @@ -0,0 +1,32 @@ +// Copyright 2020 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.stringtemplate; + +import com.google.auto.value.AutoValue; +import com.google.common.collect.ImmutableSet; + +/** + * Holds the result of expanding a string with make variables: both the new (expanded) string and + * the set of variables that were expanded. + */ +@AutoValue +public abstract class Expansion { + public static Expansion create(String expansion, ImmutableSet lookedUpVariables) { + return new AutoValue_Expansion(expansion, lookedUpVariables); + } + + public abstract String expansion(); + + public abstract ImmutableSet lookedUpVariables(); +} diff --git a/src/main/java/com/google/devtools/build/lib/analysis/stringtemplate/TemplateExpander.java b/src/main/java/com/google/devtools/build/lib/analysis/stringtemplate/TemplateExpander.java index 692c4e0eec8b92..5f55f66ffd46de 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/stringtemplate/TemplateExpander.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/stringtemplate/TemplateExpander.java @@ -13,6 +13,8 @@ // limitations under the License. package com.google.devtools.build.lib.analysis.stringtemplate; +import com.google.common.collect.ImmutableSet; + /** * Simple string template expansion. String templates consist of text interspersed with * $(variable) or $(function value) references, which are replaced by @@ -29,6 +31,16 @@ private TemplateExpander(String expression) { offset = 0; } + /** + * If the string contains a single variable, return the expansion of that variable. Otherwise, + * return null. + */ + public static String expandSingleVariable(String expression, TemplateContext context) + throws ExpansionException { + String var = new TemplateExpander(expression).getSingleVariable(); + return (var != null) ? context.lookupVariable(var) : null; + } + /** * Expands all references to template variables embedded within string "expr", using the provided * {@link TemplateContext} instance to expand individual variables. @@ -38,26 +50,16 @@ private TemplateExpander(String expression) { * @return the expansion of "expr" * @throws ExpansionException if "expr" contained undefined or ill-formed variables references */ - public static String expand(String expression, TemplateContext context) + public static Expansion expand(String expression, TemplateContext context) throws ExpansionException { if (expression.indexOf('$') < 0) { - return expression; + return Expansion.create(expression, ImmutableSet.of()); } return expand(expression, context, 0); } - /** - * If the string contains a single variable, return the expansion of that variable. - * Otherwise, return null. - */ - public static String expandSingleVariable(String expression, TemplateContext context) - throws ExpansionException { - String var = new TemplateExpander(expression).getSingleVariable(); - return (var != null) ? context.lookupVariable(var) : null; - } - // Helper method for counting recursion depth. - private static String expand(String expression, TemplateContext context, int depth) + private static Expansion expand(String expression, TemplateContext context, int depth) throws ExpansionException { if (depth > 10) { // plenty! throw new ExpansionException( @@ -66,8 +68,9 @@ private static String expand(String expression, TemplateContext context, int dep return new TemplateExpander(expression).expand(context, depth); } - private String expand(TemplateContext context, int depth) throws ExpansionException { + private Expansion expand(TemplateContext context, int depth) throws ExpansionException { StringBuilder result = new StringBuilder(); + ImmutableSet.Builder lookedUpVariables = ImmutableSet.builder(); while (offset < length) { char c = buffer[offset]; if (c == '$') { // variable @@ -82,10 +85,13 @@ private String expand(TemplateContext context, int depth) throws ExpansionExcept int spaceIndex = var.indexOf(' '); if (spaceIndex < 0) { String value = context.lookupVariable(var); + lookedUpVariables.add(var); // To prevent infinite recursion for the ignored shell variables if (!value.equals(var)) { // recursively expand using Make's ":=" semantics: - value = expand(value, context, depth + 1); + Expansion expansion = expand(value, context, depth + 1); + value = expansion.expansion(); + lookedUpVariables.addAll(expansion.lookedUpVariables()); } result.append(value); } else { @@ -93,6 +99,7 @@ private String expand(TemplateContext context, int depth) throws ExpansionExcept // Trim the string to remove leading and trailing whitespace. String param = var.substring(spaceIndex + 1).trim(); String value = context.lookupFunction(name, param); + lookedUpVariables.add(name); result.append(value); } } @@ -101,7 +108,7 @@ private String expand(TemplateContext context, int depth) throws ExpansionExcept } offset++; } - return result.toString(); + return Expansion.create(result.toString(), lookedUpVariables.build()); } /** diff --git a/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoCompileActionBuilder.java b/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoCompileActionBuilder.java index c5045efdfbde78..1a297662646452 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoCompileActionBuilder.java +++ b/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoCompileActionBuilder.java @@ -165,23 +165,24 @@ static class LazyCommandLineExpansion extends LazyString { public String toString() { try { return TemplateExpander.expand( - template, - new TemplateContext() { - @Override - public String lookupVariable(String name) - throws ExpansionException { - CharSequence value = variableValues.get(name); - if (value == null) { - throw new ExpansionException(String.format("$(%s) not defined", name)); - } - return value.toString(); - } - - @Override - public String lookupFunction(String name, String param) throws ExpansionException { - throw new ExpansionException(String.format("$(%s) not defined", name)); - } - }); + template, + new TemplateContext() { + @Override + public String lookupVariable(String name) throws ExpansionException { + CharSequence value = variableValues.get(name); + if (value == null) { + throw new ExpansionException(String.format("$(%s) not defined", name)); + } + return value.toString(); + } + + @Override + public String lookupFunction(String name, String param) + throws ExpansionException { + throw new ExpansionException(String.format("$(%s) not defined", name)); + } + }) + .expansion(); } catch (ExpansionException e) { // Squeelch. We don't throw this exception in the lookupMakeVariable implementation above, // and we can't report it here anyway, because this code will typically execute in the diff --git a/src/test/java/com/google/devtools/build/lib/analysis/RequiredConfigFragmentsTest.java b/src/test/java/com/google/devtools/build/lib/analysis/RequiredConfigFragmentsTest.java new file mode 100644 index 00000000000000..1a1af3a5435b31 --- /dev/null +++ b/src/test/java/com/google/devtools/build/lib/analysis/RequiredConfigFragmentsTest.java @@ -0,0 +1,135 @@ +// Copyright 2020 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.ImmutableSet; +import com.google.common.collect.ImmutableSortedSet; +import com.google.devtools.build.lib.analysis.util.BuildViewTestCase; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +/** Tests for {@link RequiredConfigFragmentsProvider}. */ +@RunWith(JUnit4.class) +public final class RequiredConfigFragmentsTest extends BuildViewTestCase { + @Test + public void provideTransitiveRequiredFragmentsMode() throws Exception { + useConfiguration("--include_config_fragments_provider=transitive"); + scratch.file( + "a/BUILD", + "config_setting(name = 'config', values = {'start_end_lib': '1'})", + "py_library(name = 'pylib', srcs = ['pylib.py'])", + "cc_library(name = 'a', srcs = ['A.cc'], data = [':pylib'])"); + + ImmutableSet ccLibTransitiveFragments = + getConfiguredTarget("//a:a") + .getProvider(RequiredConfigFragmentsProvider.class) + .getRequiredConfigFragments(); + assertThat(ccLibTransitiveFragments).containsAtLeast("CppConfiguration", "PythonConfiguration"); + + ImmutableSet configSettingTransitiveFragments = + getConfiguredTarget("//a:config") + .getProvider(RequiredConfigFragmentsProvider.class) + .getRequiredConfigFragments(); + assertThat(configSettingTransitiveFragments).contains("CppOptions"); + } + + @Test + public void provideDirectRequiredFragmentsMode() throws Exception { + useConfiguration("--include_config_fragments_provider=direct"); + scratch.file( + "a/BUILD", + "config_setting(name = 'config', values = {'start_end_lib': '1'})", + "py_library(name = 'pylib', srcs = ['pylib.py'])", + "cc_library(name = 'a', srcs = ['A.cc'], data = [':pylib'])"); + + ImmutableSet ccLibDirectFragments = + getConfiguredTarget("//a:a") + .getProvider(RequiredConfigFragmentsProvider.class) + .getRequiredConfigFragments(); + assertThat(ccLibDirectFragments).contains("CppConfiguration"); + assertThat(ccLibDirectFragments).doesNotContain("PythonConfiguration"); + + ImmutableSet configSettingDirectFragments = + getConfiguredTarget("//a:config") + .getProvider(RequiredConfigFragmentsProvider.class) + .getRequiredConfigFragments(); + assertThat(configSettingDirectFragments).contains("CppOptions"); + } + + /** + * Helper method that returns a combined set of the common fragments all genrules require plus + * instance-specific requirements passed here. + */ + private ImmutableSortedSet genRuleFragments(String... targetSpecificRequirements) + throws Exception { + scratch.file( + "base_genrule/BUILD", + "genrule(", + " name = 'base_genrule',", + " srcs = [],", + " outs = ['base_genrule.out'],", + " cmd = 'echo hi > $@')"); + ImmutableSortedSet.Builder builder = ImmutableSortedSet.naturalOrder(); + builder.add(targetSpecificRequirements); + builder.addAll( + getConfiguredTarget("//base_genrule") + .getProvider(RequiredConfigFragmentsProvider.class) + .getRequiredConfigFragments()); + return builder.build(); + } + + @Test + public void requiresMakeVariablesSuppliedByDefine() throws Exception { + useConfiguration("--include_config_fragments_provider=direct", "--define", "myvar=myval"); + scratch.file( + "a/BUILD", + "genrule(", + " name = 'myrule',", + " srcs = [],", + " outs = ['myrule.out'],", + " cmd = 'echo $(myvar) $(COMPILATION_MODE) > $@')"); + ImmutableSet requiredFragments = + getConfiguredTarget("//a:myrule") + .getProvider(RequiredConfigFragmentsProvider.class) + .getRequiredConfigFragments(); + assertThat(requiredFragments) + .containsExactlyElementsIn(genRuleFragments("--define:myvar")) + .inOrder(); + } + + @Test + public void starlarkExpandMakeVariables() throws Exception { + useConfiguration("--include_config_fragments_provider=direct", "--define", "myvar=myval"); + scratch.file( + "a/defs.bzl", + "def _impl(ctx):", + " print(ctx.expand_make_variables('dummy attribute', 'string with $(myvar)!', {}))", + "", + "simple_rule = rule(", + " implementation = _impl,", + " attrs = {}", + ")"); + scratch.file("a/BUILD", "load('//a:defs.bzl', 'simple_rule')", "simple_rule(name = 'simple')"); + ImmutableSet requiredFragments = + getConfiguredTarget("//a:simple") + .getProvider(RequiredConfigFragmentsProvider.class) + .getRequiredConfigFragments(); + assertThat(requiredFragments) + .containsExactlyElementsIn(genRuleFragments("--define:myvar")) + .inOrder(); + } +} diff --git a/src/test/java/com/google/devtools/build/lib/analysis/RuleConfiguredTargetTest.java b/src/test/java/com/google/devtools/build/lib/analysis/RuleConfiguredTargetTest.java index 9c6714c17b3d06..7a13d29ed26cbd 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/RuleConfiguredTargetTest.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/RuleConfiguredTargetTest.java @@ -384,49 +384,4 @@ public void testRulesDontProvideRequiredFragmentsByDefault() throws Exception { .getRequiredFragmentOptions()) .isEmpty(); } - - @Test - public void testProvideTransitiveRequiredFragmentsMode() throws Exception { - useConfiguration("--include_config_fragments_provider=transitive"); - scratch.file( - "a/BUILD", - "config_setting(name = 'config', values = {'start_end_lib': '1'})", - "py_library(name = 'pylib', srcs = ['pylib.py'])", - "cc_library(name = 'a', srcs = ['A.cc'], data = [':pylib'])"); - - ImmutableSet ccLibTransitiveFragments = - getConfiguredTarget("//a:a") - .getProvider(RequiredConfigFragmentsProvider.class) - .getRequiredConfigFragments(); - assertThat(ccLibTransitiveFragments).containsAtLeast("CppConfiguration", "PythonConfiguration"); - - ImmutableSet configSettingTransitiveFragments = - getConfiguredTarget("//a:config") - .getProvider(RequiredConfigFragmentsProvider.class) - .getRequiredConfigFragments(); - assertThat(configSettingTransitiveFragments).contains("CppOptions"); - } - - @Test - public void testProvideDirectRequiredFragmentsMode() throws Exception { - useConfiguration("--include_config_fragments_provider=direct"); - scratch.file( - "a/BUILD", - "config_setting(name = 'config', values = {'start_end_lib': '1'})", - "py_library(name = 'pylib', srcs = ['pylib.py'])", - "cc_library(name = 'a', srcs = ['A.cc'], data = [':pylib'])"); - - ImmutableSet ccLibTransitiveFragments = - getConfiguredTarget("//a:a") - .getProvider(RequiredConfigFragmentsProvider.class) - .getRequiredConfigFragments(); - assertThat(ccLibTransitiveFragments).contains("CppConfiguration"); - assertThat(ccLibTransitiveFragments).doesNotContain("PythonConfiguration"); - - ImmutableSet configSettingTransitiveFragments = - getConfiguredTarget("//a:config") - .getProvider(RequiredConfigFragmentsProvider.class) - .getRequiredConfigFragments(); - assertThat(configSettingTransitiveFragments).contains("CppOptions"); - } } diff --git a/src/test/java/com/google/devtools/build/lib/analysis/stringtemplate/TemplateExpanderTest.java b/src/test/java/com/google/devtools/build/lib/analysis/stringtemplate/TemplateExpanderTest.java index 012128d1b692fd..deedbe5a0fc014 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/stringtemplate/TemplateExpanderTest.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/stringtemplate/TemplateExpanderTest.java @@ -17,6 +17,7 @@ import static org.junit.Assert.assertThrows; import static org.junit.Assert.fail; +import com.google.common.collect.ImmutableSet; import java.util.HashMap; import java.util.Map; import java.util.function.Function; @@ -63,7 +64,7 @@ public final void createContext() throws Exception { context = new TemplateContextImpl(); } - private String expand(String value) throws ExpansionException { + private Expansion expand(String value) throws ExpansionException { return TemplateExpander.expand(value, context); } @@ -86,20 +87,27 @@ public void testVariableExpansion() throws Exception { context.vars.put("^", "src1 src2 dep1 dep2"); context.vars.put("@D", "outdir"); context.vars.put("BINDIR", "bindir"); + context.vars.put("CUSTOMVAR", "custom val"); - assertThat(expand("$(SRCS)")).isEqualTo("src1 src2"); - assertThat(expand("$<")).isEqualTo("src1"); - assertThat(expand("$(OUTS)")).isEqualTo("out1 out2"); - assertThat(expand("$(@)")).isEqualTo("out1"); - assertThat(expand("$@")).isEqualTo("out1"); - assertThat(expand("$@,")).isEqualTo("out1,"); + assertThat(expand("$(SRCS)")).isEqualTo(Expansion.create("src1 src2", ImmutableSet.of("SRCS"))); + assertThat(expand("$<")).isEqualTo(Expansion.create("src1", ImmutableSet.of("<"))); + assertThat(expand("$(OUTS)")).isEqualTo(Expansion.create("out1 out2", ImmutableSet.of("OUTS"))); + assertThat(expand("$(@)")).isEqualTo(Expansion.create("out1", ImmutableSet.of("@"))); + assertThat(expand("$@")).isEqualTo(Expansion.create("out1", ImmutableSet.of("@"))); + assertThat(expand("$@,")).isEqualTo(Expansion.create("out1,", ImmutableSet.of("@"))); + assertThat(expand("$(CUSTOMVAR)")) + .isEqualTo(Expansion.create("custom val", ImmutableSet.of("CUSTOMVAR"))); - assertThat(expand("$(SRCS) $(OUTS)")).isEqualTo("src1 src2 out1 out2"); + assertThat(expand("$(SRCS) $(OUTS)")) + .isEqualTo(Expansion.create("src1 src2 out1 out2", ImmutableSet.of("SRCS", "OUTS"))); - assertThat(expand("cmd")).isEqualTo("cmd"); - assertThat(expand("cmd $(SRCS),")).isEqualTo("cmd src1 src2,"); - assertThat(expand("label1 $(SRCS),")).isEqualTo("label1 src1 src2,"); - assertThat(expand(":label1 $(SRCS),")).isEqualTo(":label1 src1 src2,"); + assertThat(expand("cmd")).isEqualTo(Expansion.create("cmd", ImmutableSet.of())); + assertThat(expand("cmd $(SRCS),")) + .isEqualTo(Expansion.create("cmd src1 src2,", ImmutableSet.of("SRCS"))); + assertThat(expand("label1 $(SRCS),")) + .isEqualTo(Expansion.create("label1 src1 src2,", ImmutableSet.of("SRCS"))); + assertThat(expand(":label1 $(SRCS),")) + .isEqualTo(Expansion.create(":label1 src1 src2,", ImmutableSet.of("SRCS"))); } @Test @@ -113,9 +121,12 @@ public void testFunctionExpansion() throws Exception { context.functions.put("foo", (String p) -> "FOO(" + p + ")"); context.vars.put("bar", "bar"); - assertThat(expand("$(foo baz)")).isEqualTo("FOO(baz)"); - assertThat(expand("$(bar) $(foo baz)")).isEqualTo("bar FOO(baz)"); - assertThat(expand("xyz$(foo baz)zyx")).isEqualTo("xyzFOO(baz)zyx"); + assertThat(expand("$(foo baz)")) + .isEqualTo(Expansion.create("FOO(baz)", ImmutableSet.of("foo"))); + assertThat(expand("$(bar) $(foo baz)")) + .isEqualTo(Expansion.create("bar FOO(baz)", ImmutableSet.of("bar", "foo"))); + assertThat(expand("xyz$(foo baz)zyx")) + .isEqualTo(Expansion.create("xyzFOO(baz)zyx", ImmutableSet.of("foo"))); } @Test @@ -155,7 +166,8 @@ public void testRecursiveExpansion() throws Exception { // Expansion is recursive: $(recursive) -> $(SRCS) -> "src1 src2" context.vars.put("SRCS", "src1 src2"); context.vars.put("recursive", "$(SRCS)"); - assertThat(expand("$(recursive)")).isEqualTo("src1 src2"); + assertThat(expand("$(recursive)")) + .isEqualTo(Expansion.create("src1 src2", ImmutableSet.of("recursive", "SRCS"))); } @Test @@ -165,7 +177,8 @@ public void testRecursiveExpansionDoesNotSpanExpansionBoundaries() throws Except context.vars.put("SRCS", "src1 src2"); context.vars.put("recur2a", "$$"); context.vars.put("recur2b", "(SRCS)"); - assertThat(expand("$(recur2a)$(recur2b)")).isEqualTo("$(SRCS)"); + assertThat(expand("$(recur2a)$(recur2b)")) + .isEqualTo(Expansion.create("$(SRCS)", ImmutableSet.of("recur2a", "recur2b"))); } @Test @@ -200,15 +213,18 @@ public void testErrors() throws Exception { @Test public void testDollarDollar() throws Exception { assertThat(expand("for file in a b c;do echo $$file;done")) - .isEqualTo("for file in a b c;do echo $file;done"); - assertThat(expand("$${file%:.*8}")).isEqualTo("${file%:.*8}"); - assertThat(expand("$$(basename file)")).isEqualTo("$(basename file)"); + .isEqualTo(Expansion.create("for file in a b c;do echo $file;done", ImmutableSet.of())); + assertThat(expand("$${file%:.*8}")) + .isEqualTo(Expansion.create("${file%:.*8}", ImmutableSet.of())); + assertThat(expand("$$(basename file)")) + .isEqualTo(Expansion.create("$(basename file)", ImmutableSet.of())); } // Regression test: check that the parameter is trimmed before expanding. @Test public void testFunctionExpansionIsTrimmed() throws Exception { context.functions.put("foo", (String p) -> "FOO(" + p + ")"); - assertThat(expand("$(foo baz )")).isEqualTo("FOO(baz)"); + assertThat(expand("$(foo baz )")) + .isEqualTo(Expansion.create("FOO(baz)", ImmutableSet.of("foo"))); } }