Rewrite LocationExpander
Split up the functionality into separate classes, and test each independently.
(Keep one integration test to make sure it still works together.)
This is in preparation for adding another location function for runfiles paths.
Currently we have to decide ahead of time whether to expand artifacts as exec
paths or root-relative (runfiles) paths, but there are cases where we can't
make that decision ahead of time and / or need both to coexist, even in the
same attribute.
Progress on #2475.
PiperOrigin-RevId: 170691666
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
new file mode 100644
index 0000000..6dc4574
--- /dev/null
+++ b/src/test/java/com/google/devtools/build/lib/analysis/LocationExpanderIntegrationTest.java
@@ -0,0 +1,66 @@
+// 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 new LocationExpander(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");
+ }
+}
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/LocationExpanderTest.java b/src/test/java/com/google/devtools/build/lib/analysis/LocationExpanderTest.java
index 173c900..ae23f50 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/LocationExpanderTest.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/LocationExpanderTest.java
@@ -16,78 +16,90 @@
import static com.google.common.truth.Truth.assertThat;
-import com.google.devtools.build.lib.analysis.util.BuildViewTestCase;
-import org.junit.Before;
+import com.google.devtools.build.lib.packages.RuleErrorConsumer;
+import java.util.ArrayList;
+import java.util.List;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;
/** Unit tests for {@link LocationExpander}. */
@RunWith(JUnit4.class)
-public class LocationExpanderTest extends BuildViewTestCase {
+public class LocationExpanderTest {
+ private static final class Capture implements RuleErrorConsumer {
+ private final List<String> warnsOrErrors = new ArrayList<>();
- @Before
- public void createFiles() throws Exception {
- // Set up a rule to test expansion in.
- scratch.file("files/fileA");
- scratch.file("files/fileB");
+ @Override
+ public void ruleWarning(String message) {
+ warnsOrErrors.add("WARN: " + message);
+ }
- scratch.file(
- "files/BUILD",
- "filegroup(name='files',",
- " srcs = ['fileA', 'fileB'])",
- "sh_library(name='lib',",
- " deps = [':files'])");
+ @Override
+ public void ruleError(String message) {
+ warnsOrErrors.add("ERROR: " + message);
+ }
+
+ @Override
+ public void attributeWarning(String attrName, String message) {
+ warnsOrErrors.add("WARN-" + attrName + ": " + message);
+ }
+
+ @Override
+ public void attributeError(String attrName, String message) {
+ warnsOrErrors.add("ERROR-" + attrName + ": " + message);
+ }
}
- private LocationExpander makeExpander(String label) throws Exception {
- ConfiguredTarget target = getConfiguredTarget(label);
- RuleContext ruleContext = getRuleContext(target);
- return new LocationExpander(ruleContext);
+ private LocationExpander makeExpander(RuleErrorConsumer ruleErrorConsumer) throws Exception {
+ return new LocationExpander(
+ ruleErrorConsumer,
+ (String s) -> "one(" + s + ")",
+ (String s) -> "more(" + s + ")");
+ }
+
+ private String expand(String input) throws Exception {
+ return makeExpander(new Capture()).expand(input);
}
@Test
- public void location_absolute() throws Exception {
- LocationExpander expander = makeExpander("//files:files");
- String input = "foo $(location //files:fileA) bar";
- String result = expander.expand(input);
-
- assertThat(result).isEqualTo("foo files/fileA bar");
+ public void noExpansion() throws Exception {
+ assertThat(expand("abc")).isEqualTo("abc");
}
@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");
+ 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)---");
}
@Test
- public void location_relative() throws Exception {
- LocationExpander expander = makeExpander("//files:files");
- String input = "foo $(location :fileA) bar";
- String result = expander.expand(input);
-
- assertThat(result).isEqualTo("foo files/fileA bar");
+ public void twoInOne() throws Exception {
+ assertThat(expand("$(location a) $(locations b)")).isEqualTo("one(a) more(b)");
}
@Test
- public void locations_relative() throws Exception {
- LocationExpander expander = makeExpander("//files:lib");
- String input = "foo $(locations :files) bar";
- String result = expander.expand(input);
+ public void notAFunction() throws Exception {
+ assertThat(expand("$(locationz a)")).isEqualTo("$(locationz a)");
+ }
- assertThat(result).isEqualTo("foo files/fileA files/fileB bar");
+ @Test
+ public void missingClosingParen() throws Exception {
+ Capture capture = new Capture();
+ String value = makeExpander(capture).expand("foo $(location a");
+ // In case of an error, no location expansion is performed.
+ assertThat(value).isEqualTo("foo $(location a");
+ assertThat(capture.warnsOrErrors).containsExactly("ERROR: unterminated $(location) expression");
+ }
+
+ // In case of errors, the exact return value is unspecified. However, we don't want to
+ // accidentally change the behavior even in this unspecified case - that's why I added a test
+ // here.
+ @Test
+ public void noExpansionOnError() throws Exception {
+ Capture capture = new Capture();
+ String value = makeExpander(capture).expand("foo $(location a) $(location a");
+ assertThat(value).isEqualTo("foo $(location a) $(location a");
+ assertThat(capture.warnsOrErrors).containsExactly("ERROR: unterminated $(location) expression");
}
}
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/LocationFunctionTest.java b/src/test/java/com/google/devtools/build/lib/analysis/LocationFunctionTest.java
new file mode 100644
index 0000000..676255f
--- /dev/null
+++ b/src/test/java/com/google/devtools/build/lib/analysis/LocationFunctionTest.java
@@ -0,0 +1,189 @@
+// 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 static org.junit.Assert.fail;
+
+import com.google.common.base.Suppliers;
+import com.google.devtools.build.lib.actions.Artifact;
+import com.google.devtools.build.lib.actions.Root;
+import com.google.devtools.build.lib.analysis.LocationExpander.LocationFunction;
+import com.google.devtools.build.lib.cmdline.Label;
+import com.google.devtools.build.lib.vfs.FileSystem;
+import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem;
+import java.util.Arrays;
+import java.util.Collection;
+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), Root.asDerivedRoot(fs.getPath("/exec"), fs.getPath("/exec/out")));
+ } else {
+ return new Artifact(fs.getPath(path), Root.asSourceRoot(fs.getPath("/exec")));
+ }
+ }
+
+ @Test
+ public void absoluteAndRelativeLabels() throws Exception {
+ LocationFunction func =
+ new LocationFunctionBuilder("//foo", false).add("//foo", "/exec/src/bar").build();
+ assertThat(func.apply("//foo")).isEqualTo("src/bar");
+ assertThat(func.apply(":foo")).isEqualTo("src/bar");
+ assertThat(func.apply("foo")).isEqualTo("src/bar");
+ }
+
+ @Test
+ public void pathUnderExecRootUsesDotSlash() throws Exception {
+ LocationFunction func =
+ new LocationFunctionBuilder("//foo", false).add("//foo", "/exec/bar").build();
+ assertThat(func.apply("//foo")).isEqualTo("./bar");
+ }
+
+ @Test
+ public void noSuchLabel() throws Exception {
+ LocationFunction func = new LocationFunctionBuilder("//foo", false).build();
+ try {
+ func.apply("//bar");
+ fail();
+ } catch (IllegalStateException expected) {
+ assertThat(expected).hasMessageThat()
+ .isEqualTo(
+ "label '//bar:bar' in $(location) expression is not a declared prerequisite of this "
+ + "rule");
+ }
+ }
+
+ @Test
+ public void emptyList() throws Exception {
+ LocationFunction func = new LocationFunctionBuilder("//foo", false).add("//foo").build();
+ try {
+ func.apply("//foo");
+ fail();
+ } catch (IllegalStateException expected) {
+ assertThat(expected).hasMessageThat()
+ .isEqualTo("label '//foo:foo' in $(location) expression expands to no files");
+ }
+ }
+
+ @Test
+ public void tooMany() throws Exception {
+ LocationFunction func =
+ new LocationFunctionBuilder("//foo", false).add("//foo", "/exec/1", "/exec/2").build();
+ try {
+ func.apply("//foo");
+ fail();
+ } catch (IllegalStateException expected) {
+ assertThat(expected).hasMessageThat()
+ .isEqualTo(
+ "label '//foo:foo' in $(location) expression expands to more than one file, "
+ + "please use $(locations //foo:foo) instead. Files (at most 5 shown) are: "
+ + "[./1, ./2]");
+ }
+ }
+
+ @Test
+ public void noSuchLabelMultiple() throws Exception {
+ LocationFunction func = new LocationFunctionBuilder("//foo", true).build();
+ try {
+ func.apply("//bar");
+ fail();
+ } catch (IllegalStateException expected) {
+ assertThat(expected).hasMessageThat()
+ .isEqualTo(
+ "label '//bar:bar' in $(locations) expression is not a declared prerequisite of this "
+ + "rule");
+ }
+ }
+
+ @Test
+ public void fileWithSpace() throws Exception {
+ LocationFunction func =
+ new LocationFunctionBuilder("//foo", false).add("//foo", "/exec/file/with space").build();
+ assertThat(func.apply("//foo")).isEqualTo("'file/with space'");
+ }
+
+ @Test
+ public void multipleFiles() throws Exception {
+ LocationFunction func = new LocationFunctionBuilder("//foo", true)
+ .add("//foo", "/exec/foo/bar", "/exec/out/foo/foobar")
+ .build();
+ assertThat(func.apply("//foo")).isEqualTo("foo/bar foo/foobar");
+ }
+
+ @Test
+ public void filesWithSpace() throws Exception {
+ LocationFunction func = new LocationFunctionBuilder("//foo", true)
+ .add("//foo", "/exec/file/with space", "/exec/file/with spaces ")
+ .build();
+ assertThat(func.apply("//foo")).isEqualTo("'file/with space' 'file/with spaces '");
+ }
+
+ @Test
+ public void execPath() throws Exception {
+ LocationFunction func = new LocationFunctionBuilder("//foo", true)
+ .setExecPaths(true)
+ .add("//foo", "/exec/bar", "/exec/out/foobar")
+ .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<>();
+
+ 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 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(LocationFunctionTest.this::makeArtifact)
+ .collect(Collectors.toList()));
+ return this;
+ }
+ }
+}
