[8.5.0] Propagate errors correctly from WindowsFileSystem symlink methods.

src/main/java/com/google/devtools/build/lib/windows/BUILD

@@ -19,6 +19,8 @@ java_11_library(
     deps = [
         ":windows_path_operations",
         "//src/main/java/com/google/devtools/build/lib/jni",
+        "//src/main/java/com/google/devtools/build/lib/vfs",
+        "//src/main/java/com/google/devtools/build/lib/vfs:pathfragment",
     ],
 )
 

src/main/java/com/google/devtools/build/lib/windows/WindowsFileOperations.java

@@ -15,6 +15,8 @@
 package com.google.devtools.build.lib.windows;
 
 import com.google.devtools.build.lib.jni.JniLoader;
+import com.google.devtools.build.lib.vfs.FileSystem.NotASymlinkException;
+import com.google.devtools.build.lib.vfs.PathFragment;
 import java.io.FileNotFoundException;
 import java.io.IOException;
 import java.nio.file.AccessDeniedException;
@@ -49,34 +51,6 @@ private WindowsFileOperations() {
     // Prevent construction
   }
 
-  /** Result of {@link #readSymlinkOrJunction}. */
-  public static class ReadSymlinkOrJunctionResult {
-
-    /** Status code, indicating success or failure. */
-    public enum Status {
-      OK,
-      NOT_A_LINK,
-      ERROR
-    }
-
-    private String result;
-    private Status status;
-
-    public ReadSymlinkOrJunctionResult(Status s, String r) {
-      this.status = s;
-      this.result = r;
-    }
-
-    /** Result string (junction target) or error message (depending on {@link status}). */
-    public String getResult() {
-      return result;
-    }
-
-    public Status getStatus() {
-      return status;
-    }
-  }
-
   // Keep IS_SYMLINK_OR_JUNCTION_* values in sync with src/main/native/windows/file.cc.
   private static final int IS_SYMLINK_OR_JUNCTION_SUCCESS = 0;
   // IS_SYMLINK_OR_JUNCTION_ERROR = 1;
@@ -115,7 +89,6 @@ public Status getStatus() {
   private static final int READ_SYMLINK_OR_JUNCTION_ACCESS_DENIED = 2;
   private static final int READ_SYMLINK_OR_JUNCTION_DOES_NOT_EXIST = 3;
   private static final int READ_SYMLINK_OR_JUNCTION_NOT_A_LINK = 4;
-  private static final int READ_SYMLINK_OR_JUNCTION_UNKNOWN_LINK_TYPE = 5;
 
   private static native int nativeIsSymlinkOrJunction(
       String path, boolean[] result, String[] error);
@@ -140,8 +113,7 @@ public static boolean isSymlinkOrJunction(String path) throws IOException {
       case IS_SYMLINK_OR_JUNCTION_SUCCESS:
         return result[0];
       case IS_SYMLINK_OR_JUNCTION_DOES_NOT_EXIST:
-        error[0] = "path does not exist";
-        break;
+        throw new FileNotFoundException(path);
       default:
         // This is IS_SYMLINK_OR_JUNCTION_ERROR (1). The JNI code puts a custom message in
         // 'error[0]'.
@@ -225,34 +197,23 @@ public static void createSymlink(String name, String target) throws IOException
         String.format("Cannot create symlink (name=%s, target=%s): %s", name, target, error[0]));
   }
 
-  public static ReadSymlinkOrJunctionResult readSymlinkOrJunction(String name) {
+  public static String readSymlinkOrJunction(String name) throws IOException {
     String[] target = new String[] {null};
     String[] error = new String[] {null};
     switch (nativeReadSymlinkOrJunction(WindowsPathOperations.asLongPath(name), target, error)) {
       case READ_SYMLINK_OR_JUNCTION_SUCCESS:
-        return new ReadSymlinkOrJunctionResult(
-            ReadSymlinkOrJunctionResult.Status.OK,
-            WindowsPathOperations.removeUncPrefixAndUseSlashes(target[0]));
+        return WindowsPathOperations.removeUncPrefixAndUseSlashes(target[0]);
       case READ_SYMLINK_OR_JUNCTION_ACCESS_DENIED:
-        error[0] = "access is denied";
-        break;
+        throw new AccessDeniedException(name);
       case READ_SYMLINK_OR_JUNCTION_DOES_NOT_EXIST:
-        error[0] = "path does not exist";
-        break;
+        throw new FileNotFoundException(name);
       case READ_SYMLINK_OR_JUNCTION_NOT_A_LINK:
-        return new ReadSymlinkOrJunctionResult(
-            ReadSymlinkOrJunctionResult.Status.NOT_A_LINK, "path is not a link");
-      case READ_SYMLINK_OR_JUNCTION_UNKNOWN_LINK_TYPE:
-        error[0] = "unknown link type";
-        break;
+        throw new NotASymlinkException(PathFragment.create(name));
       default:
         // This is READ_SYMLINK_OR_JUNCTION_ERROR (1). The JNI code puts a custom message in
         // 'error[0]'.
-        break;
+        throw new IOException(String.format("Cannot read link (name=%s): %s", name, error[0]));
     }
-    return new ReadSymlinkOrJunctionResult(
-        ReadSymlinkOrJunctionResult.Status.ERROR,
-        String.format("Cannot read link (name=%s): %s", name, error[0]));
   }
 
   public static boolean deletePath(String path) throws IOException {

src/main/java/com/google/devtools/build/lib/windows/WindowsFileSystem.java

@@ -103,15 +103,9 @@ protected void createSymbolicLink(PathFragment linkPath, PathFragment targetFrag
   @Override
   protected PathFragment readSymbolicLink(PathFragment path) throws IOException {
     java.nio.file.Path nioPath = getNioPath(path);
-    WindowsFileOperations.ReadSymlinkOrJunctionResult result =
-        WindowsFileOperations.readSymlinkOrJunction(nioPath.toString());
-    if (result.getStatus() == WindowsFileOperations.ReadSymlinkOrJunctionResult.Status.OK) {
-      return PathFragment.create(StringEncoding.platformToInternal(result.getResult()));
-    }
-    if (result.getStatus() == WindowsFileOperations.ReadSymlinkOrJunctionResult.Status.NOT_A_LINK) {
-      throw new NotASymlinkException(path);
-    }
-    throw new IOException(result.getResult());
+    return PathFragment.create(
+        StringEncoding.platformToInternal(
+            WindowsFileOperations.readSymlinkOrJunction(nioPath.toString())));
   }
 
   @Override

src/main/native/windows/file.cc

@@ -607,7 +607,10 @@ int ReadSymlinkOrJunction(const wstring& path, wstring* result,
       return ReadSymlinkOrJunctionResult::kNotALink;
     }
     default:
-      return ReadSymlinkOrJunctionResult::kUnknownLinkType;
+      *error =
+          MakeErrorMessage(WSTR(__FILE__), __LINE__, L"ReadSymlinkOrJunction",
+                           path, L"unsupported link type");
+      return ReadSymlinkOrJunctionResult::kError;
   }
 }
 

src/main/native/windows/file.h

@@ -133,7 +133,6 @@ struct ReadSymlinkOrJunctionResult {
     kAccessDenied = 2,
     kDoesNotExist = 3,
     kNotALink = 4,
-    kUnknownLinkType = 5,
   };
 };
 

src/main/tools/build-runfiles-windows.cc

@@ -113,9 +113,6 @@ bool ReadSymlink(const wstring& abs_path, wstring* target, wstring* error) {
     case bazel::windows::ReadSymlinkOrJunctionResult::kNotALink:
       *error = L"path is not a link";
       break;
-    case bazel::windows::ReadSymlinkOrJunctionResult::kUnknownLinkType:
-      *error = L"unknown link type";
-      break;
     default:
       // This is bazel::windows::ReadSymlinkOrJunctionResult::kError (1).
       // The JNI code puts a custom message in 'error'.

src/test/java/com/google/devtools/build/lib/windows/WindowsFileOperationsTest.java

@@ -15,13 +15,15 @@
 package com.google.devtools.build.lib.windows;
 
 import static com.google.common.truth.Truth.assertThat;
+import static org.junit.Assert.assertThrows;
 import static org.junit.Assert.fail;
 
 import com.google.common.collect.ImmutableMap;
 import com.google.devtools.build.lib.testutil.TestSpec;
 import com.google.devtools.build.lib.util.OS;
 import com.google.devtools.build.lib.windows.util.WindowsTestUtil;
 import java.io.File;
+import java.io.FileNotFoundException;
 import java.io.IOException;
 import java.nio.file.Files;
 import java.util.Arrays;
@@ -76,12 +78,9 @@ public void testSymlinkCreation() throws Exception {
     // Assert deleting the symlink does not remove the target file.
     assertThat(WindowsFileOperations.deletePath(symlinkFile.toString())).isTrue();
     assertThat(helloFile.exists()).isTrue();
-    try {
-      WindowsFileOperations.isSymlinkOrJunction(symlinkFile.toString());
-      fail("Expected to throw: Symlink should no longer exist.");
-    } catch (IOException e) {
-      assertThat(e).hasMessageThat().contains("path does not exist");
-    }
+    assertThrows(
+        FileNotFoundException.class,
+        () -> WindowsFileOperations.isSymlinkOrJunction(symlinkFile.toString()));
   }
 
   @Test
@@ -143,12 +142,9 @@ public void testIsJunction() throws Exception {
     assertThat(WindowsFileOperations.isSymlinkOrJunction(root + "\\longtargetpath\\file2.txt"))
         .isFalse();
     assertThat(WindowsFileOperations.isSymlinkOrJunction(root + "\\longta~1\\file2.txt")).isFalse();
-    try {
-      WindowsFileOperations.isSymlinkOrJunction(root + "\\non-existent");
-      fail("expected to throw");
-    } catch (IOException e) {
-      assertThat(e.getMessage()).contains("path does not exist");
-    }
+    assertThrows(
+        FileNotFoundException.class,
+        () -> WindowsFileOperations.isSymlinkOrJunction(root + "\\non-existent"));
     assertThat(Arrays.asList(new File(root + "/shrtpath/a").list())).containsExactly("file1.txt");
     assertThat(Arrays.asList(new File(root + "/shrtpath/b").list())).containsExactly("file2.txt");
     assertThat(Arrays.asList(new File(root + "/shrtpath/c").list())).containsExactly("file2.txt");

src/test/java/com/google/devtools/build/lib/windows/WindowsFileSystemTest.java

@@ -15,7 +15,7 @@
 package com.google.devtools.build.lib.windows;
 
 import static com.google.common.truth.Truth.assertThat;
-import static org.junit.Assert.fail;
+import static org.junit.Assert.assertThrows;
 
 import com.google.common.base.Function;
 import com.google.common.collect.ImmutableList;
@@ -24,12 +24,13 @@
 import com.google.devtools.build.lib.testutil.TestSpec;
 import com.google.devtools.build.lib.util.OS;
 import com.google.devtools.build.lib.vfs.DigestHashFunction;
+import com.google.devtools.build.lib.vfs.FileSystem.NotASymlinkException;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import com.google.devtools.build.lib.vfs.Symlinks;
 import com.google.devtools.build.lib.windows.util.WindowsTestUtil;
 import java.io.File;
-import java.io.IOException;
+import java.io.FileNotFoundException;
 import java.nio.file.Files;
 import java.util.Arrays;
 import java.util.HashMap;
@@ -166,12 +167,10 @@ public void testIsJunction() throws Exception {
         .isFalse();
     assertThat(WindowsFileSystem.isSymlinkOrJunction(new File(root, "longta~1/file2.txt")))
         .isFalse();
-    try {
-      WindowsFileSystem.isSymlinkOrJunction(new File(root, "non-existent"));
-      fail("expected failure");
-    } catch (IOException e) {
-      assertThat(e.getMessage()).contains("path does not exist");
-    }
+
+    assertThrows(
+        FileNotFoundException.class,
+        () -> WindowsFileSystem.isSymlinkOrJunction(new File(root, "non-existent")));
 
     assertThat(Arrays.asList(new File(root + "/shrtpath/a").list())).containsExactly("file1.txt");
     assertThat(Arrays.asList(new File(root + "/shrtpath/b").list())).containsExactly("file2.txt");
@@ -364,26 +363,15 @@ public void testReadJunction() throws Exception {
     assertThat(dirPath.isSymbolicLink()).isFalse();
     assertThat(juncPath.isSymbolicLink()).isTrue();
 
-    try {
-      testUtil.createVfsPath(fs, "does-not-exist").readSymbolicLink();
-      fail("expected exception");
-    } catch (IOException expected) {
-      assertThat(expected).hasMessageThat().matches(".*path does not exist");
-    }
+    assertThrows(
+        FileNotFoundException.class,
+        () -> testUtil.createVfsPath(fs, "does-not-exist").readSymbolicLink());
 
-    try {
-      testUtil.createVfsPath(fs, "dir\\hello.txt").readSymbolicLink();
-      fail("expected exception");
-    } catch (IOException expected) {
-      assertThat(expected).hasMessageThat().matches(".*is not a symlink");
-    }
+    assertThrows(
+        NotASymlinkException.class,
+        () -> testUtil.createVfsPath(fs, "dir\\hello.txt").readSymbolicLink());
 
-    try {
-      dirPath.readSymbolicLink();
-      fail("expected exception");
-    } catch (IOException expected) {
-      assertThat(expected).hasMessageThat().matches(".*is not a symlink");
-    }
+    assertThrows(NotASymlinkException.class, () -> dirPath.readSymbolicLink());
 
     assertThat(juncPath.readSymbolicLink()).isEqualTo(dirPath.asFragment());
   }