java,runfiles: fix bugs in runfiles library
Bazel 0.11.0 releases a new Java runfiles library
in @bazel_tools. This commit fixes some bugs in
0.11.0rc1:
- The library no longer respects TEST_SRCDIR, so
now it's possible to run Bazel inside of a test
and build and run a mock java_binary, and that
java_binary will *not* pick up the test's
TEST_SRCDIR.
- The library now allows calling rlocation for
absolute paths, and just returns the path
itself. This is in accordance with how our Bash
rlocation() implementation works.
Change-Id: I471247d7538a76ea8162d2192add3f9733f844a8
PiperOrigin-RevId: 184990272
diff --git a/src/tools/runfiles/java/com/google/devtools/build/runfiles/Runfiles.java b/src/tools/runfiles/java/com/google/devtools/build/runfiles/Runfiles.java
index 4d44b35..c93772b 100644
--- a/src/tools/runfiles/java/com/google/devtools/build/runfiles/Runfiles.java
+++ b/src/tools/runfiles/java/com/google/devtools/build/runfiles/Runfiles.java
@@ -57,15 +57,15 @@
* key's value in {@code env}.
*
* <p>Otherwise this method returns a directory-based implementation. The directory's path is
- * defined by the "RUNFILES_DIR" or "TEST_SRCDIR" key's value in {@code env}, in this priority
- * order.
+ * defined by the value in {@code env} under the "RUNFILES_DIR" key, or if absent, then under the
+ * "JAVA_RUNFILES" key.
*
* <p>Note about performance: the manifest-based implementation eagerly reads and caches the whole
* manifest file upon instantiation.
*
* @throws IOException if RUNFILES_MANIFEST_ONLY=1 is in {@code env} but there's no
- * "RUNFILES_MANIFEST_FILE", or if neither "RUNFILES_DIR" nor "TEST_SRCDIR" is in {@code env},
- * or if some IO error occurs
+ * "RUNFILES_MANIFEST_FILE", "RUNFILES_DIR", or "JAVA_RUNFILES" key in {@code env} or their
+ * values are empty, or some IO error occurs
*/
public static Runfiles create(Map<String, String> env) throws IOException {
if (isManifestOnly(env)) {
@@ -89,16 +89,15 @@
*
* @param path runfiles-root-relative path of the runfile
* @throws IllegalArgumentException if {@code path} fails validation, for example if it's null or
- * empty, it's absolute or contains uplevel references
+ * empty, or contains uplevel references
*/
public final String rlocation(String path) {
Util.checkArgument(path != null);
Util.checkArgument(!path.isEmpty());
Util.checkArgument(!path.contains(".."), "path contains uplevel references: \"%s\"", path);
- Util.checkArgument(
- !new File(path).isAbsolute() && path.charAt(0) != File.separatorChar,
- "path is absolute: \"%s\"",
- path);
+ if (new File(path).isAbsolute() || path.charAt(0) == File.separatorChar) {
+ return path;
+ }
return rlocationChecked(path);
}
@@ -118,15 +117,13 @@
}
private static String getRunfilesDir(Map<String, String> env) throws IOException {
- // On Linux and macOS, Bazel sets RUNFILES_DIR and TEST_SRCDIR.
- // Google-internal Blaze sets only TEST_SRCDIR.
String value = env.get("RUNFILES_DIR");
if (Util.isNullOrEmpty(value)) {
- value = env.get("TEST_SRCDIR");
+ value = env.get("JAVA_RUNFILES");
}
if (Util.isNullOrEmpty(value)) {
throw new IOException(
- "Cannot find runfiles: $RUNFILES_DIR and $TEST_SRCDIR are both unset or empty");
+ "Cannot find runfiles: $RUNFILES_DIR and $JAVA_RUNFILES are both unset or empty");
}
return value;
}
diff --git a/src/tools/runfiles/java/com/google/devtools/build/runfiles/RunfilesTest.java b/src/tools/runfiles/java/com/google/devtools/build/runfiles/RunfilesTest.java
index 5b88231..c93ab34 100644
--- a/src/tools/runfiles/java/com/google/devtools/build/runfiles/RunfilesTest.java
+++ b/src/tools/runfiles/java/com/google/devtools/build/runfiles/RunfilesTest.java
@@ -30,6 +30,10 @@
@RunWith(JUnit4.class)
public final class RunfilesTest {
+ private static boolean isWindows() {
+ return File.separatorChar == '\\';
+ }
+
private void assertRlocationArg(Runfiles runfiles, String path, @Nullable String error)
throws Exception {
try {
@@ -48,13 +52,6 @@
assertRlocationArg(r, null, null);
assertRlocationArg(r, "", null);
assertRlocationArg(r, "foo/..", "contains uplevel");
- if (File.separatorChar == '/') {
- assertRlocationArg(r, "/foo", "is absolute");
- } else {
- assertRlocationArg(r, "\\foo", "is absolute");
- assertRlocationArg(r, "c:/foo", "is absolute");
- assertRlocationArg(r, "c:\\foo", "is absolute");
- }
}
@Test
@@ -66,9 +63,18 @@
"RUNFILES_MANIFEST_ONLY", "1",
"RUNFILES_MANIFEST_FILE", mf.path.toString(),
"RUNFILES_DIR", "ignored when RUNFILES_MANIFEST_ONLY=1",
- "TEST_SRCDIR", "ignored when RUNFILES_MANIFEST_ONLY=1"));
+ "JAVA_RUNFILES", "ignored when RUNFILES_DIR has a value",
+ "TEST_SRCDIR", "should always be ignored"));
assertThat(r.rlocation("a/b")).isEqualTo("c/d");
assertThat(r.rlocation("foo")).isNull();
+
+ if (isWindows()) {
+ assertThat(r.rlocation("\\foo")).isEqualTo("\\foo");
+ assertThat(r.rlocation("c:/foo")).isEqualTo("c:/foo");
+ assertThat(r.rlocation("c:\\foo")).isEqualTo("c:\\foo");
+ } else {
+ assertThat(r.rlocation("/foo")).isEqualTo("/foo");
+ }
}
}
@@ -79,7 +85,8 @@
ImmutableMap.of(
"RUNFILES_MANIFEST_FILE", "ignored when RUNFILES_MANIFEST_ONLY is not set to 1",
"RUNFILES_DIR", "runfiles/dir",
- "TEST_SRCDIR", "ignored when RUNFILES_DIR is set"));
+ "JAVA_RUNFILES", "ignored when RUNFILES_DIR has a value",
+ "TEST_SRCDIR", "should always be ignored"));
assertThat(r.rlocation("a/b")).isEqualTo("runfiles/dir/a/b");
assertThat(r.rlocation("foo")).isEqualTo("runfiles/dir/foo");
@@ -87,9 +94,41 @@
Runfiles.create(
ImmutableMap.of(
"RUNFILES_MANIFEST_FILE", "ignored when RUNFILES_MANIFEST_ONLY is not set to 1",
- "TEST_SRCDIR", "test/srcdir"));
- assertThat(r.rlocation("a/b")).isEqualTo("test/srcdir/a/b");
- assertThat(r.rlocation("foo")).isEqualTo("test/srcdir/foo");
+ "RUNFILES_DIR", "",
+ "JAVA_RUNFILES", "runfiles/dir",
+ "TEST_SRCDIR", "should always be ignored"));
+ assertThat(r.rlocation("a/b")).isEqualTo("runfiles/dir/a/b");
+ assertThat(r.rlocation("foo")).isEqualTo("runfiles/dir/foo");
+ }
+
+ @Test
+ public void testIgnoresTestSrcdirWhenJavaRunfilesIsUndefinedAndJustFails() throws Exception {
+ Runfiles.create(
+ ImmutableMap.of(
+ "RUNFILES_DIR", "non-empty",
+ "RUNFILES_MANIFEST_FILE", "ignored when RUNFILES_MANIFEST_ONLY is not set to 1",
+ "TEST_SRCDIR", "should always be ignored"));
+
+ Runfiles.create(
+ ImmutableMap.of(
+ "JAVA_RUNFILES", "non-empty",
+ "RUNFILES_MANIFEST_FILE", "ignored when RUNFILES_MANIFEST_ONLY is not set to 1",
+ "TEST_SRCDIR", "should always be ignored"));
+
+ try {
+ // The method must ignore TEST_SRCDIR, for the scenario when Bazel runs a test which itself
+ // runs Bazel to build and run java_binary. The java_binary should not pick up the test's
+ // TEST_SRCDIR.
+ Runfiles.create(
+ ImmutableMap.of(
+ "RUNFILES_DIR", "",
+ "JAVA_RUNFILES", "",
+ "RUNFILES_MANIFEST_FILE", "ignored when RUNFILES_MANIFEST_ONLY is not set to 1",
+ "TEST_SRCDIR", "should always be ignored"));
+ fail();
+ } catch (IOException e) {
+ assertThat(e).hasMessageThat().contains("$RUNFILES_DIR and $JAVA_RUNFILES");
+ }
}
@Test
