Skip to content

Commit

Permalink
Change LocationFunction to not extend Function.
Browse files Browse the repository at this point in the history 
This is necessary for subsequent changes to the apply method for diamond splitting, which will require apply() to take more than one argument. But it seems like the only reason LocationFunction ever extended Function was for tests and so this is an improvement on its own.

RELNOTES: None
PiperOrigin-RevId: 194796136
  • Loading branch information
@dkelmer
dkelmer authored and Copybara-Service committed Apr 30, 2018
1 parent 493a4b1 commit 25d5efc
Show file tree
Hide file tree
Showing 4 changed files with 68 additions and 59 deletions.
18 changes: 11 additions & 7 deletions src/main/java/com/google/devtools/build/lib/analysis/LocationExpander.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@
import java.util.Map;
import java.util.Set;
import java.util.TreeSet;
import java.util.function.Function;
import javax.annotation.Nullable;

/**
Expand Down Expand Up @@ -67,12 +66,12 @@ public final class LocationExpander {
private static final boolean USE_EXEC_PATHS = true;

private final RuleErrorConsumer ruleErrorConsumer;
private final ImmutableMap<String, Function<String, String>> functions;
private final ImmutableMap<String, LocationFunction> functions;

@VisibleForTesting
LocationExpander(
RuleErrorConsumer ruleErrorConsumer,
Map<String, Function<String, String>> functions) {
Map<String, LocationFunction> functions) {
this.ruleErrorConsumer = ruleErrorConsumer;
this.functions = ImmutableMap.copyOf(functions);
}
Expand Down Expand Up @@ -223,7 +222,7 @@ private String expand(String value, ErrorReporter reporter) {
}

@VisibleForTesting
static final class LocationFunction implements Function<String, String> {
static final class LocationFunction {
private static final int MAX_PATHS_SHOWN = 5;

private final Label root;
Expand All @@ -242,7 +241,12 @@ static final class LocationFunction implements Function<String, String> {
this.multiple = multiple;
}

@Override
/**
* Looks up the label-like string in the locationMap and returns the resolved path string.
*
* @param arg The label-like string to be expanded, e.g. ":foo" or "//foo:bar"
* @return The expanded value
*/
public String apply(String arg) {
Label label;
try {
Expand Down Expand Up @@ -327,9 +331,9 @@ private String functionName() {
}
}

static ImmutableMap<String, Function<String, String>> allLocationFunctions(
static ImmutableMap<String, LocationFunction> allLocationFunctions(
Label root, Supplier<Map<Label, Collection<Artifact>>> locationMap, boolean execPaths) {
return new ImmutableMap.Builder<String, Function<String, String>>()
return new ImmutableMap.Builder<String, LocationFunction>()
.put("location", new LocationFunction(root, locationMap, execPaths, EXACTLY_ONE))
.put("locations", new LocationFunction(root, locationMap, execPaths, ALLOW_MULTIPLE))
.put("rootpath", new LocationFunction(root, locationMap, USE_ROOT_PATHS, EXACTLY_ONE))
Expand Down
6 changes: 3 additions & 3 deletions src/main/java/com/google/devtools/build/lib/analysis/LocationTemplateContext.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,12 @@
import com.google.common.collect.ImmutableCollection;
import com.google.common.collect.ImmutableMap;
import com.google.devtools.build.lib.actions.Artifact;
import com.google.devtools.build.lib.analysis.LocationExpander.LocationFunction;
import com.google.devtools.build.lib.analysis.stringtemplate.ExpansionException;
import com.google.devtools.build.lib.analysis.stringtemplate.TemplateContext;
import com.google.devtools.build.lib.cmdline.Label;
import java.util.Collection;
import java.util.Map;
import java.util.function.Function;
import javax.annotation.Nullable;

/**
Expand All @@ -46,7 +46,7 @@
*/
final class LocationTemplateContext implements TemplateContext {
private final TemplateContext delegate;
private final ImmutableMap<String, Function<String, String>> functions;
private final ImmutableMap<String, LocationFunction> functions;

private LocationTemplateContext(
TemplateContext delegate,
Expand Down Expand Up @@ -80,7 +80,7 @@ public String lookupVariable(String name) throws ExpansionException {
@Override
public String lookupFunction(String name, String param) throws ExpansionException {
try {
Function<String, String> f = functions.get(name);
LocationFunction f = functions.get(name);
if (f != null) {
return f.apply(param);
}
Expand Down
27 changes: 19 additions & 8 deletions src/test/java/com/google/devtools/build/lib/analysis/LocationExpanderTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,11 @@
import static com.google.common.truth.Truth.assertThat;

import com.google.common.collect.ImmutableMap;
import com.google.devtools.build.lib.analysis.LocationExpander.LocationFunction;
import com.google.devtools.build.lib.packages.AbstractRuleErrorConsumer;
import com.google.devtools.build.lib.packages.RuleErrorConsumer;
import java.util.ArrayList;
import java.util.List;
import java.util.function.Function;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;
Expand Down Expand Up @@ -60,11 +60,22 @@ public boolean hasErrors() {
}

private LocationExpander makeExpander(RuleErrorConsumer ruleErrorConsumer) throws Exception {

LocationFunction f1 = new LocationFunctionBuilder("//a", false)
.setExecPaths(false)
.add("//a", "/exec/src/a")
.build();

LocationFunction f2 = new LocationFunctionBuilder("//b", true)
.setExecPaths(false)
.add("//b", "/exec/src/b")
.build();

return new LocationExpander(
ruleErrorConsumer,
ImmutableMap.<String, Function<String, String>>of(
"location", (String s) -> "one(" + s + ")",
"locations", (String s) -> "more(" + s + ")"));
ImmutableMap.<String, LocationFunction>of(
"location", f1,
"locations", f2));
}

private String expand(String input) throws Exception {
Expand All @@ -78,14 +89,14 @@ public void noExpansion() throws Exception {

@Test
public void oneOrMore() throws Exception {
assertThat(expand("$(location a)")).isEqualTo("one(a)");
assertThat(expand("$(locations b)")).isEqualTo("more(b)");
assertThat(expand("---$(location a)---")).isEqualTo("---one(a)---");
assertThat(expand("$(location a)")).isEqualTo("src/a");
assertThat(expand("$(locations b)")).isEqualTo("src/b");
assertThat(expand("---$(location a)---")).isEqualTo("---src/a---");
}

@Test
public void twoInOne() throws Exception {
assertThat(expand("$(location a) $(locations b)")).isEqualTo("one(a) more(b)");
assertThat(expand("$(location a) $(locations b)")).isEqualTo("src/a src/b");
}

@Test
Expand Down
76 changes: 35 additions & 41 deletions src/test/java/com/google/devtools/build/lib/analysis/LocationFunctionTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,31 +30,13 @@
import java.util.HashMap;
import java.util.Map;
import java.util.stream.Collectors;
import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;

/** Unit tests for {@link LocationExpander.LocationFunction}. */
@RunWith(JUnit4.class)
public class LocationFunctionTest {
private FileSystem fs;

@Before
public void createFileSystem() throws Exception {
fs = new InMemoryFileSystem();
}

private Artifact makeArtifact(String path) {
if (path.startsWith("/exec/out")) {
return new Artifact(
fs.getPath(path),
ArtifactRoot.asDerivedRoot(fs.getPath("/exec"), fs.getPath("/exec/out")));
} else {
return new Artifact(
fs.getPath(path), ArtifactRoot.asSourceRoot(Root.fromPath(fs.getPath("/exec"))));
}
}

@Test
public void absoluteAndRelativeLabels() throws Exception {
Expand Down Expand Up @@ -159,34 +141,46 @@ public void execPath() throws Exception {
.build();
assertThat(func.apply("//foo")).isEqualTo("./bar out/foobar");
}
}

private final class LocationFunctionBuilder {
private final Label root;
private final boolean multiple;
private boolean execPaths;
private final Map<Label, Collection<Artifact>> labelMap = new HashMap<>();
final class LocationFunctionBuilder {
private final Label root;
private final boolean multiple;
private boolean execPaths;
private final Map<Label, Collection<Artifact>> labelMap = new HashMap<>();

LocationFunctionBuilder(String rootLabel, boolean multiple) {
this.root = Label.parseAbsoluteUnchecked(rootLabel);
this.multiple = multiple;
}
LocationFunctionBuilder(String rootLabel, boolean multiple) {
this.root = Label.parseAbsoluteUnchecked(rootLabel);
this.multiple = multiple;
}

public LocationFunction build() {
return new LocationFunction(root, Suppliers.ofInstance(labelMap), execPaths, multiple);
}
public LocationFunction build() {
return new LocationFunction(root, Suppliers.ofInstance(labelMap), execPaths, multiple);
}

public LocationFunctionBuilder setExecPaths(boolean execPaths) {
this.execPaths = execPaths;
return this;
}
public LocationFunctionBuilder setExecPaths(boolean execPaths) {
this.execPaths = execPaths;
return this;
}

public LocationFunctionBuilder add(String label, String... paths) {
labelMap.put(
Label.parseAbsoluteUnchecked(label),
Arrays.stream(paths)
.map(LocationFunctionBuilder::makeArtifact)
.collect(Collectors.toList()));
return this;
}

public LocationFunctionBuilder add(String label, String... paths) {
labelMap.put(
Label.parseAbsoluteUnchecked(label),
Arrays.stream(paths)
.map(LocationFunctionTest.this::makeArtifact)
.collect(Collectors.toList()));
return this;
private static Artifact makeArtifact(String path) {
FileSystem fs = new InMemoryFileSystem();
if (path.startsWith("/exec/out")) {
return new Artifact(
fs.getPath(path),
ArtifactRoot.asDerivedRoot(fs.getPath("/exec"), fs.getPath("/exec/out")));
} else {
return new Artifact(
fs.getPath(path), ArtifactRoot.asSourceRoot(Root.fromPath(fs.getPath("/exec"))));
}
}
}

0 comments on commit 25d5efc

Please sign in to comment.