Skip to content

Commit

Permalink
Change CommandHelper to use TemplateExpander directly
Browse files Browse the repository at this point in the history 
This is a partial rollback of e8d32b7, only the CommandHelper change.

Progress on #2475.

PiperOrigin-RevId: 179413908
  • Loading branch information
@ulfjack
ulfjack authored and Copybara-Service committed Dec 18, 2017
1 parent dd11a0e commit 2f10da0
Show file tree
Hide file tree
Showing 5 changed files with 72 additions and 52 deletions.
35 changes: 20 additions & 15 deletions src/main/java/com/google/devtools/build/lib/analysis/CommandHelper.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -163,24 +166,26 @@ private static Collection<Artifact> mapGet(Map<Label, Collection<Artifact>> map,
}

/**
* 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;
}

/**
Expand Down
40 changes: 22 additions & 18 deletions src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleContext.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -1117,27 +1118,30 @@ public boolean checkPlaceholders(String template, SkylarkList<Object> allowedPla
+ "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);
Expand Down
9 changes: 4 additions & 5 deletions ...va/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleImplementationFunctions.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -709,15 +710,13 @@ public Tuple<Object> invoke(
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());

Expand Down
11 changes: 11 additions & 0 deletions src/main/java/com/google/devtools/build/lib/analysis/stringtemplate/TemplateContext.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
29 changes: 15 additions & 14 deletions ...ysis/LocationExpanderIntegrationTest.java → ...lib/analysis/ExpanderIntegrationTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -40,10 +41,10 @@ public void createFiles() throws Exception {
" 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
Expand All @@ -57,9 +58,9 @@ public void locations_spaces() throws Exception {
"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");
}
Expand All @@ -71,14 +72,14 @@ public void otherPathExpansion() throws Exception {
"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");
}

Expand All @@ -89,10 +90,10 @@ public void otherPathMultiExpansion() throws Exception {
"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");
}
}

0 comments on commit 2f10da0

Please sign in to comment.