Fix artifact/symlink mismatch detection

The check in ActionMetadataHandler that an output declared to be a
symlink is indeed created as a symlink by the action was ineffective as
it would only run if a stat of the output showed that is already was a
symlink. The test only passed by accident since it runs sandboxed and
the sandbox setup would call Path.readSymbolicLink on what it expects to
be a symlink. As this does not happen on Windows, the test correctly
fails there.

This is fixed by calling Path.readSymbolicLink on the output path of
an expected symlink and handling errors appropriately rather than
assuming the type returned by stat matches the artifact type (file vs symlink).

With this issue fixed, bazel_symlink_test can be enabled on Windows with
the following test-only changes:
* Pass --windows_enable_symlinks as a startup option.
* Use relative instead of absolute symlink targets as these have
  consistent shape under Unix and Windows.
* Use python to create symlinks rather than `ln -s`, which doesn't seem
  to be able to create unresolved symlinks on Windows.

Work towards #10298

Closes #15700.

PiperOrigin-RevId: 464635914
Change-Id: I8516e59d75fab4a1498f1fea5ad22553c55275f6

src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java

@@ -600,6 +600,12 @@ private static FileArtifactValue fileArtifactValueFromArtifact(
     checkState(!artifact.isTreeArtifact() && !artifact.isMiddlemanArtifact(), artifact);
 
     Path pathNoFollow = artifactPathResolver.toPath(artifact);
+    // If we expect a symlink, we can readlink it directly and handle errors appropriately - there
+    // is no need for the stat below.
+    if (artifact.isSymlink()) {
+      return FileArtifactValue.createForUnresolvedSymlink(pathNoFollow);
+    }
+
     RootedPath rootedPathNoFollow =
         RootedPath.toRootedPath(
             artifactPathResolver.transformRoot(artifact.getRoot().getRoot()),
@@ -618,10 +624,6 @@ private static FileArtifactValue fileArtifactValueFromArtifact(
           rootedPathNoFollow, statNoFollow, digestWillBeInjected, xattrProvider, tsgm);
     }
 
-    if (artifact.isSymlink()) {
-      return FileArtifactValue.createForUnresolvedSymlink(pathNoFollow);
-    }
-
     // We use FileStatus#isSymbolicLink over Path#isSymbolicLink to avoid the unnecessary stat
     // done by the latter.  We need to protect against symlink cycles since
     // ArtifactFileMetadata#value assumes it's dealing with a file that's not in a symlink cycle.

src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java

@@ -98,6 +98,7 @@
 import com.google.devtools.build.lib.util.DetailedExitCode;
 import com.google.devtools.build.lib.util.io.FileOutErr;
 import com.google.devtools.build.lib.vfs.FileSystem;
+import com.google.devtools.build.lib.vfs.FileSystem.NotASymlinkException;
 import com.google.devtools.build.lib.vfs.OutputService;
 import com.google.devtools.build.lib.vfs.OutputService.ActionFileSystemType;
 import com.google.devtools.build.lib.vfs.Path;
@@ -1541,8 +1542,13 @@ private boolean checkOutputs(
           success = false;
           if (output.isTreeArtifact()) {
             reportOutputTreeArtifactErrors(action, output, reporter, e);
+          } else if (output.isSymlink() && e instanceof NotASymlinkException) {
+            reporter.handle(
+                Event.error(
+                    action.getOwner().getLocation(),
+                    String.format("declared output '%s' is not a symlink", output.prettyPrint())));
           } else {
-            // Are all exceptions caught due to missing files?
+            // Are all other exceptions caught due to missing files?
             reportMissingOutputFile(action, output, reporter, output.getPath().isSymbolicLink(), e);
           }
         }

src/main/java/com/google/devtools/build/lib/vfs/FileSystem.java

@@ -49,10 +49,8 @@ public DigestHashFunction getDigestFunction() {
     return digestFunction;
   }
 
-  /**
-   * An exception thrown when attempting to resolve an ordinary file as a symlink.
-   */
-  protected static final class NotASymlinkException extends IOException {
+  /** An exception thrown when attempting to resolve an ordinary file as a symlink. */
+  public static final class NotASymlinkException extends IOException {
     public NotASymlinkException(PathFragment path) {
       super(path.getPathString() + " is not a symlink");
     }

src/main/java/com/google/devtools/build/lib/vfs/Path.java

@@ -492,8 +492,8 @@ public void createSymbolicLink(PathFragment target) throws IOException {
    * an {@link UnsupportedOperationException} if the link points to a non-existent file.
    *
    * @return the content (i.e. target) of the symbolic link
-   * @throws IOException if the current path is not a symbolic link, or the contents of the link
-   *     could not be read for any reason
+   * @throws FileSystem.NotASymlinkException if the current path is not a symbolic link.
+   * @throws IOException if the contents of the link could not be read for any reason
    */
   public PathFragment readSymbolicLink() throws IOException {
     return fileSystem.readSymbolicLink(asFragment());
@@ -504,8 +504,8 @@ public PathFragment readSymbolicLink() throws IOException {
    * are intentionally left underspecified otherwise to permit efficient implementations.
    *
    * @return the content (i.e. target) of the symbolic link
-   * @throws IOException if the current path is not a symbolic link, or the contents of the link
-   *     could not be read for any reason
+   * @throws FileSystem.NotASymlinkException if the current path is not a symbolic link.
+   * @throws IOException if the contents of the link could not be read for any reason
    */
   public PathFragment readSymbolicLinkUnchecked() throws IOException {
     return fileSystem.readSymbolicLinkUnchecked(asFragment());