[7.1.0] Handle symlinks in a more consistent manner in UploadManifest.

src/main/java/com/google/devtools/build/lib/remote/UploadManifest.java

@@ -71,7 +71,6 @@
 import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Map;
-import java.util.Set;
 import java.util.stream.Collectors;
 import javax.annotation.Nullable;
 
@@ -151,7 +150,13 @@ private static Timestamp instantToTimestamp(Instant instant) {
 
   /**
    * Create an UploadManifest from an ActionResult builder and an exec root. The ActionResult
-   * builder is populated through a call to {@link #addFile(Digest, Path)}.
+   * builder is populated through a call to {@link #addFiles(Collection)}.
+   *
+   * @param followSymlinks whether a non-dangling relative symlink should be transparently
+   *     dereferenced and uploaded as the file or directory it points to; other forms of symlink are
+   *     always uploaded as such.
+   * @param allowDanglingSymlinks whether an uploaded symlink should be allowed to dangle.
+   * @param allowAbsoluteSymlinks whether an uploaded symlink should be allowed to be absolute.
    */
   @VisibleForTesting
   public UploadManifest(
@@ -181,70 +186,89 @@ private void setStdoutStderr(FileOutErr outErr) throws IOException {
   }
 
   /**
-   * Add a collection of files or directories to the UploadManifest. Adding a directory has the
-   * effect of 1) uploading a {@link Tree} protobuf message from which the whole structure of the
-   * directory, including the descendants, can be reconstructed and 2) uploading all the
-   * non-directory descendant files.
+   * Add a collection of files, directories or symlinks to the manifest.
+   *
+   * <p>Adding a directory has the effect of:
+   *
+   * <ol>
+   *   <li>uploading a {@link Tree} protobuf message from which the whole structure of the
+   *       directory, including the descendants, can be reconstructed.
+   *   <li>uploading all of the non-directory descendant files.
+   * </ol>
+   *
+   * <p>Note that the manifest describes the outcome of a spawn, not of an action. In particular,
+   * it's possible for an output to be missing or to have been created with an unsuitable file type
+   * for the corresponding {@link Artifact} (e.g., a directory where a file was expected, or a
+   * non-symlink where a symlink was expected). Outputs are always uploaded according to the
+   * filesystem state, possibly after applying the transformation implied by {@link followSymlinks}.
+   * A type mismatch may later cause execution to fail, but that's an action-level concern.
    */
   @VisibleForTesting
   void addFiles(Collection<Path> files) throws ExecException, IOException {
+    // TODO(tjgq): Non-dangling absolute symlinks are uploaded as the file or directory they point
+    // to even when followSymlinks is false. This is inconsistent with the treatment of relative
+    // symlinks, but fixing it would require an incompatible change.
     for (Path file : files) {
       // TODO(ulfjack): Maybe pass in a SpawnResult here, add a list of output files to that, and
       // rely on the local spawn runner to stat the files, instead of statting here.
-      FileStatus stat = file.statIfFound(Symlinks.NOFOLLOW);
+      FileStatus statNoFollow = file.statIfFound(Symlinks.NOFOLLOW);
       // TODO(#6547): handle the case where the parent directory of the output file is an
       // output symlink.
-      if (stat == null) {
-        // We ignore requested results that have not been generated by the action.
+      if (statNoFollow == null) {
+        // Ignore missing outputs.
         continue;
       }
-      if (stat.isDirectory()) {
-        addDirectory(file);
-      } else if (stat.isFile() && !stat.isSpecialFile()) {
-        Digest digest = digestUtil.compute(file, stat.getSize());
+      if (statNoFollow.isFile() && !statNoFollow.isSpecialFile()) {
+        Digest digest = digestUtil.compute(file, statNoFollow.getSize());
         addFile(digest, file);
-      } else if (stat.isSymbolicLink()) {
+        continue;
+      }
+      if (statNoFollow.isDirectory()) {
+        addDirectory(file);
+        continue;
+      }
+      if (statNoFollow.isSymbolicLink()) {
         PathFragment target = file.readSymbolicLink();
         // Need to resolve the symbolic link to know what to add, file or directory.
         FileStatus statFollow = file.statIfFound(Symlinks.FOLLOW);
         if (statFollow == null) {
-          if (allowDanglingSymlinks) {
-            if (target.isAbsolute() && !allowAbsoluteSymlinks) {
-              throw new IOException(
-                  String.format(
-                      "Action output %s is an absolute symbolic link to %s, which is not allowed by"
-                          + " the remote cache",
-                      file, target));
+          // Symlink uploaded as a symlink. Report it as a file since we don't know any better.
+          checkDanglingSymlinkAllowed(file, target);
+          if (target.isAbsolute()) {
+            checkAbsoluteSymlinkAllowed(file, target);
+          }
+          addFileSymbolicLink(file, target);
+          continue;
+        }
+        if (statFollow.isFile() && !statFollow.isSpecialFile()) {
+          if (followSymlinks || target.isAbsolute()) {
+            // Symlink to file uploaded as a file.
+            addFile(digestUtil.compute(file), file);
+          } else {
+            // Symlink to file uploaded as a symlink.
+            if (target.isAbsolute()) {
+              checkAbsoluteSymlinkAllowed(file, target);
             }
-            // Report symlink to a file since we don't know any better.
             addFileSymbolicLink(file, target);
-          } else {
-            throw new IOException(
-                String.format(
-                    "Action output %s is a dangling symbolic link to %s. ", file, target));
           }
-        } else if (statFollow.isSpecialFile()) {
-          illegalOutput(file);
-        } else {
-          Preconditions.checkState(
-              statFollow.isFile() || statFollow.isDirectory(), "Unknown stat type for %s", file);
-          if (!followSymlinks && !target.isAbsolute()) {
-            if (statFollow.isFile()) {
-              addFileSymbolicLink(file, target);
-            } else {
-              addDirectorySymbolicLink(file, target);
-            }
+          continue;
+        }
+        if (statFollow.isDirectory()) {
+          if (followSymlinks || target.isAbsolute()) {
+            // Symlink to directory uploaded as a directory.
+            addDirectory(file);
           } else {
-            if (statFollow.isFile()) {
-              addFile(digestUtil.compute(file), file);
-            } else {
-              addDirectory(file);
+            // Symlink to directory uploaded as a symlink.
+            if (target.isAbsolute()) {
+              checkAbsoluteSymlinkAllowed(file, target);
             }
+            addDirectorySymbolicLink(file, target);
           }
+          continue;
         }
-      } else {
-        illegalOutput(file);
       }
+      // Special file or dereferenced symlink to special file.
+      rejectSpecialFile(file);
     }
   }
 
@@ -321,7 +345,7 @@ private void addFile(Digest digest, Path file) throws IOException {
       Tree.getDescriptor().findFieldByName("children").getNumber();
 
   private void addDirectory(Path dir) throws ExecException, IOException {
-    Set<ByteString> directories = new LinkedHashSet<>();
+    LinkedHashSet<ByteString> directories = new LinkedHashSet<>();
     var ignored = computeDirectory(dir, directories);
 
     // Convert individual Directory messages to a Tree message. As we want the
@@ -350,7 +374,16 @@ private void addDirectory(Path dir) throws ExecException, IOException {
     digestToBlobs.put(digest, data);
   }
 
-  private ByteString computeDirectory(Path path, Set<ByteString> directories)
+  /**
+   * Computes the {@link Directory} message describing the transitive contents of a directory.
+   *
+   * @param path the directory path.
+   * @param directories an output parameter accepting the wire-format {@link Directory} messages for
+   *     every visited (sub-)directory of {@code path}, including {@code path} itself, in a
+   *     deterministic topological order (children before parents).
+   * @return the wire-format {@link Directory} message for {@code path}.
+   */
+  private ByteString computeDirectory(Path path, LinkedHashSet<ByteString> directories)
       throws ExecException, IOException {
     Directory.Builder b = Directory.newBuilder();
 
@@ -360,52 +393,83 @@ private ByteString computeDirectory(Path path, Set<ByteString> directories)
     for (Dirent dirent : sortedDirent) {
       String name = dirent.getName();
       Path child = path.getRelative(name);
+      if (dirent.getType() == Dirent.Type.FILE) {
+        Digest digest = digestUtil.compute(child);
+        b.addFilesBuilder().setName(name).setDigest(digest).setIsExecutable(child.isExecutable());
+        digestToFile.put(digest, child);
+        continue;
+      }
       if (dirent.getType() == Dirent.Type.DIRECTORY) {
         ByteString dir = computeDirectory(child, directories);
         b.addDirectoriesBuilder().setName(name).setDigest(digestUtil.compute(dir.toByteArray()));
-      } else if (dirent.getType() == Dirent.Type.SYMLINK) {
+        continue;
+      }
+      if (dirent.getType() == Dirent.Type.SYMLINK) {
         PathFragment target = child.readSymbolicLink();
-        if (!followSymlinks && !target.isAbsolute()) {
-          // Whether it is dangling or not, we're passing it on.
+        FileStatus statFollow = child.statIfFound(Symlinks.FOLLOW);
+        if (statFollow == null || (!followSymlinks && !target.isAbsolute())) {
+          // Symlink uploaded as a symlink.
+          if (statFollow == null) {
+            checkDanglingSymlinkAllowed(child, target);
+          }
+          if (target.isAbsolute()) {
+            checkAbsoluteSymlinkAllowed(child, target);
+          }
           b.addSymlinksBuilder().setName(name).setTarget(target.toString());
           continue;
         }
-        // Need to resolve the symbolic link now to know whether to upload a file or a directory.
-        FileStatus statFollow = child.statIfFound(Symlinks.FOLLOW);
-        if (statFollow == null) {
-          throw new IOException(
-              String.format("Action output %s is a dangling symbolic link to %s ", child, target));
-        }
         if (statFollow.isFile() && !statFollow.isSpecialFile()) {
+          // Symlink to file uploaded as a file.
           Digest digest = digestUtil.compute(child);
           b.addFilesBuilder().setName(name).setDigest(digest).setIsExecutable(child.isExecutable());
           digestToFile.put(digest, child);
-        } else if (statFollow.isDirectory()) {
+          continue;
+        }
+        if (statFollow.isDirectory()) {
+          // Symlink to directory uploaded as a directory.
           ByteString dir = computeDirectory(child, directories);
           b.addDirectoriesBuilder().setName(name).setDigest(digestUtil.compute(dir.toByteArray()));
-        } else {
-          illegalOutput(child);
+          continue;
         }
-      } else if (dirent.getType() == Dirent.Type.FILE) {
-        Digest digest = digestUtil.compute(child);
-        b.addFilesBuilder().setName(name).setDigest(digest).setIsExecutable(child.isExecutable());
-        digestToFile.put(digest, child);
-      } else {
-        illegalOutput(child);
       }
+      // Special file or dereferenced symlink to special file.
+      rejectSpecialFile(child);
     }
 
     ByteString directory = b.build().toByteString();
     directories.add(directory);
     return directory;
   }
 
-  private void illegalOutput(Path path) throws ExecException {
+  private void checkDanglingSymlinkAllowed(Path file, PathFragment target) throws IOException {
+    if (!allowDanglingSymlinks) {
+      throw new IOException(
+          String.format(
+              "Spawn output %s is a dangling symbolic link to %s, which is not allowed by"
+                  + " --noincompatible_remote_dangling_symlinks",
+              file, target));
+    }
+  }
+
+  private void checkAbsoluteSymlinkAllowed(Path file, PathFragment target) throws IOException {
+    if (!allowAbsoluteSymlinks) {
+      throw new IOException(
+          String.format(
+              "Spawn output %s is an absolute symbolic link to %s, which is not allowed by"
+                  + " the remote cache",
+              file, target));
+    }
+  }
+
+  private void rejectSpecialFile(Path path) throws ExecException {
+    // TODO(tjgq): Consider treating special files as regular, following Skyframe.
+    // (On the other hand, they seem to be only useful for testing purposes, so we might instead
+    // want to forbid them entirely.)
     String message =
         String.format(
-            "Output %s is a special file. Only regular files, directories or symlinks may be "
+            "Spawn output %s is a special file. Only regular files, directories or symlinks may be "
                 + "uploaded to a remote cache.",
-            remotePathResolver.localPathToOutputPath(path));
+            path);
 
     FailureDetail failureDetail =
         FailureDetail.newBuilder()

src/main/java/com/google/devtools/build/lib/remote/options/RemoteOptions.java

@@ -407,8 +407,9 @@ public RemoteBuildEventUploadModeConverter() {
       effectTags = {OptionEffectTag.EXECUTION},
       metadataTags = {OptionMetadataTag.INCOMPATIBLE_CHANGE},
       help =
-          "If set to true, Bazel will upload symlinks as such to a remote or disk cache. Otherwise,"
-              + " non-dangling symlinks will be uploaded as the file or directory they point to.")
+          "If set to true, Bazel will always upload symlinks as such to a remote or disk cache."
+              + " Otherwise, non-dangling relative symlinks (and only those) will be uploaded as"
+              + " the file or directory they point to.")
   public boolean incompatibleRemoteSymlinks;
 
   @Option(