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 before rather than after stat-ing it and trusting the
file type contained in the stat info.

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 a helper java_binary to create symlinks rather than `ln -s`, which
  doesn't seem to be able to create unresolved symlinks on Windows.

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);
+ // Handle the case where a symlink is expected before stat-ing the output so that the case where
+ // it incorrectly is a file triggers the expected NotASymlinkException.
+ 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

@@ -97,6 +97,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;
@@ -1534,8 +1535,15 @@ private boolean checkOutputs(
  if (output.isTreeArtifact()) {
  reportOutputTreeArtifactErrors(action, output, reporter, e);
  } else {
- // Are all exceptions caught due to missing files?
- reportMissingOutputFile(action, output, reporter, output.getPath().isSymbolicLink(), e);
+ if (e instanceof NotASymlinkException) {
+ reporter.handle(Event.error(
+ action.getOwner().getLocation(),
+ String.format("declared output '%s' is not a symlink", output.prettyPrint())));
+ } else {
+ // 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

@@ -51,7 +51,7 @@ public DigestHashFunction getDigestFunction() {
  /**
  * An exception thrown when attempting to resolve an ordinary file as a symlink.
  */
- protected static final class NotASymlinkException extends IOException {
+ 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());