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

2 files changed, 61 insertions, 25 deletions

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 4d44b35205..c93772bff0 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 @@ public abstract class Runfiles {
    * 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 @@ public abstract class Runfiles {
    *
    * @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 @@ public abstract class Runfiles {
   }
 
   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 5b882318f3..c93ab34d9b 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 @@ import org.junit.runners.JUnit4;
 @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 @@ public final class RunfilesTest {
     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 @@ public final class RunfilesTest {
                   "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 @@ public final class RunfilesTest {
             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 @@ public final class RunfilesTest {
         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