Support remote symlink outputs when building without the bytes.

Fixes #13355.

PiperOrigin-RevId: 534008963
Change-Id: Ia4f22f16960bcdae86c1a8820e3d47a3689876d8

Author: tjgq

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

@@ -866,12 +866,6 @@ private void injectRemoteArtifacts(
 
     for (Map.Entry<Path, DirectoryMetadata> entry : metadata.directories()) {
       DirectoryMetadata directory = entry.getValue();
-      if (!directory.symlinks().isEmpty()) {
-        throw new IOException(
-            "Symlinks in action outputs are not yet supported by "
-                + "--experimental_remote_download_outputs=minimal");
-      }
-
       for (FileMetadata file : directory.files()) {
         remoteActionFileSystem.injectRemoteFile(
             file.path().asFragment(),
@@ -1162,7 +1156,8 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re
 
     ImmutableList.Builder<ListenableFuture<FileMetadata>> downloadsBuilder =
         ImmutableList.builder();
-    boolean downloadOutputs = shouldDownloadOutputsFor(action, result, metadata);
+
+    boolean downloadOutputs = shouldDownloadOutputsFor(action, result);
 
     // Download into temporary paths, then move everything at the end.
     // This avoids holding the output lock while downloading, which would prevent the local branch
@@ -1182,10 +1177,6 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re
       checkState(
           result.getExitCode() == 0,
           "injecting remote metadata is only supported for successful actions (exit code 0).");
-      checkState(
-          metadata.symlinks.isEmpty(),
-          "Symlinks in action outputs are not yet supported by"
-              + " --remote_download_outputs=minimal");
     }
 
     FileOutErr tmpOutErr = outErr.childOutErr();
@@ -1219,38 +1210,6 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re
 
     if (downloadOutputs) {
       moveOutputsToFinalLocation(downloads, realToTmpPath);
-
-      List<SymlinkMetadata> symlinksInDirectories = new ArrayList<>();
-      for (Entry<Path, DirectoryMetadata> entry : metadata.directories()) {
-        for (SymlinkMetadata symlink : entry.getValue().symlinks()) {
-          // Symlinks should not be allowed inside directories because their semantics are unclear:
-          // tree artifacts are defined as a collection of regular files, and resolving a remotely
-          // produced symlink against the local filesystem is asking for trouble.
-          //
-          // Sadly, we started permitting relative symlinks at some point, so we have to allow them
-          // until the --incompatible_remote_disallow_symlink_in_tree_artifact flag is flipped.
-          // Absolute symlinks, on the other hand, have never been allowed.
-          //
-          // See also https://github.com/bazelbuild/bazel/issues/16361 for potential future work
-          // to allow *unresolved* symlinks in a tree artifact.
-          boolean isAbsolute = symlink.target().isAbsolute();
-          if (remoteOptions.incompatibleRemoteDisallowSymlinkInTreeArtifact || isAbsolute) {
-            throw new IOException(
-                String.format(
-                    "Unsupported symlink '%s' inside tree artifact '%s'",
-                    symlink.path().relativeTo(entry.getKey()),
-                    entry.getKey().relativeTo(execRoot)));
-          }
-          symlinksInDirectories.add(symlink);
-        }
-      }
-
-      Iterable<SymlinkMetadata> symlinks =
-          Iterables.concat(metadata.symlinks(), symlinksInDirectories);
-
-      // Create the symbolic links after all downloads are finished, because dangling symlinks
-      // might not be supported on all platforms.
-      createSymlinks(symlinks);
     } else {
       ActionInput inMemoryOutput = null;
       Digest inMemoryOutputDigest = null;
@@ -1285,6 +1244,37 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re
       }
     }
 
+    List<SymlinkMetadata> symlinksInDirectories = new ArrayList<>();
+    for (Entry<Path, DirectoryMetadata> entry : metadata.directories()) {
+      for (SymlinkMetadata symlink : entry.getValue().symlinks()) {
+        // Symlinks should not be allowed inside directories because their semantics are unclear:
+        // tree artifacts are defined as a collection of regular files, and resolving a remotely
+        // produced symlink against the local filesystem is asking for trouble.
+        //
+        // Sadly, we started permitting relative symlinks at some point, so we have to allow them
+        // until the --incompatible_remote_disallow_symlink_in_tree_artifact flag is flipped.
+        // Absolute symlinks, on the other hand, have never been allowed.
+        //
+        // See also https://github.com/bazelbuild/bazel/issues/16361 for potential future work
+        // to allow *unresolved* symlinks in a tree artifact.
+        boolean isAbsolute = symlink.target().isAbsolute();
+        if (remoteOptions.incompatibleRemoteDisallowSymlinkInTreeArtifact || isAbsolute) {
+          throw new IOException(
+              String.format(
+                  "Unsupported symlink '%s' inside tree artifact '%s'",
+                  symlink.path().relativeTo(entry.getKey()), entry.getKey().relativeTo(execRoot)));
+        }
+        symlinksInDirectories.add(symlink);
+      }
+    }
+
+    Iterable<SymlinkMetadata> symlinks =
+        Iterables.concat(metadata.symlinks(), symlinksInDirectories);
+
+    // Create the symbolic links after all downloads are finished, because dangling symlinks
+    // might not be supported on all platforms.
+    createSymlinks(symlinks);
+
     if (result.success()) {
       // Check that all mandatory outputs are created.
       for (ActionInput output : action.getSpawn().getOutputFiles()) {
@@ -1325,8 +1315,7 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re
     return null;
   }
 
-  private boolean shouldDownloadOutputsFor(
-      RemoteAction action, RemoteActionResult result, ActionResultMetadata metadata) {
+  private boolean shouldDownloadOutputsFor(RemoteAction action, RemoteActionResult result) {
     if (remoteOptions.remoteOutputsMode.downloadAllOutputs()) {
       return true;
     }
@@ -1335,16 +1324,6 @@ private boolean shouldDownloadOutputsFor(
     if (result.getExitCode() != 0) {
       return true;
     }
-    // Symlinks in actions output are not yet supported with BwoB.
-    if (!metadata.symlinks().isEmpty()) {
-      report(
-          Event.warn(
-              String.format(
-                  "Symlinks in action outputs are not yet supported by --remote_download_minimal,"
-                      + " falling back to downloading all action outputs due to output symlink %s",
-                  Iterables.get(metadata.symlinks(), 0).path())));
-      return true;
-    }
 
     checkNotNull(remoteOutputChecker, "remoteOutputChecker must not be null");
     for (var output : action.getSpawn().getOutputFiles()) {

src/test/shell/bazel/remote/build_without_the_bytes_test.sh

@@ -370,7 +370,7 @@ EOF
 # Test that --remote_download_toplevel fetches inputs to symlink actions. In
 # particular, cc_binary links against a symlinked imported .so file, and only
 # the symlink is in the runfiles.
-function test_downloads_toplevel_symlinks() {
+function test_downloads_toplevel_symlink_action() {
   if [[ "$PLATFORM" == "darwin" ]]; then
     # TODO(b/37355380): This test is disabled due to RemoteWorker not supporting
     # setting SDKROOT and DEVELOPER_DIR appropriately, as is required of
@@ -419,49 +419,60 @@ EOF
   ./bazel-bin/a/foo${EXE_EXT} || fail "bazel-bin/a/foo${EXE_EXT} failed to run"
 }
 
-function test_symlink_outputs_warning_with_minimal() {
-  mkdir -p a
-  touch a/file1.txt a/file2.txt
-  cat > a/defs.bzl <<'EOF'
-def _impl(ctx):
-  commands = []
-  outputs = []
-  for target, name in ctx.attr.symlink_map.items():
-    sym = ctx.actions.declare_symlink(name)
-    file = target.files.to_list()[0]
-    outputs.append(sym)
-    commands.append("ln -s {} {}".format(file.path, sym.path))
+function setup_symlink_output() {
+  mkdir -p pkg
 
+  cat > pkg/defs.bzl <<EOF
+def _impl(ctx):
+  sym = ctx.actions.declare_symlink(ctx.label.name)
   ctx.actions.run_shell(
-    outputs = outputs,
-    command = " && ".join(commands),
+    outputs = [sym],
+    command = "ln -s {} {}".format(ctx.attr.target, sym.path),
   )
+  return DefaultInfo(files = depset([sym]))
 
-  return DefaultInfo(files = depset(outputs))
-
-symlinks = rule(
+symlink = rule(
   implementation = _impl,
   attrs = {
-    "symlink_map": attr.label_keyed_string_dict(allow_files = True),
+    "target": attr.string(),
   },
 )
 EOF
-  cat > a/BUILD <<'EOF'
-load(":defs.bzl", "symlinks")
-symlinks(
+
+  cat > pkg/BUILD <<EOF
+load(":defs.bzl", "symlink")
+symlink(
   name = "sym",
-  symlink_map = {
-    "file1.txt": "sym1",
-    "file2.txt": "sym2",
-  },
+  target = "target.txt",
 )
 EOF
+}
+
+function test_downloads_toplevel_non_dangling_symlink_output() {
+  setup_symlink_output
+  touch pkg/target.txt
+
+  bazel build \
+    --remote_executor=grpc://localhost:${worker_port} \
+    --remote_download_toplevel \
+    //pkg:sym >& $TEST_log || fail "Expected build of //pkg:sym to succeed"
+
+  if [[ "$(readlink bazel-bin/pkg/sym)" != "target.txt" ]]; then
+    fail "Expected bazel-bin/pkg/sym to be a symlink pointing to target.txt"
+  fi
+}
+
+function test_downloads_toplevel_dangling_symlink_output() {
+  setup_symlink_output
 
   bazel build \
     --remote_executor=grpc://localhost:${worker_port} \
     --remote_download_minimal \
-    //a:sym >& $TEST_log || fail "Expected build of //a:sym to succeed"
-  expect_log "Symlinks in action outputs are not yet supported"
+    //pkg:sym >& $TEST_log || fail "Expected build of //pkg:sym to succeed"
+
+  if [[ "$(readlink bazel-bin/pkg/sym)" != "target.txt" ]]; then
+    fail "Expected bazel-bin/pkg/sym to be a symlink pointing to target.txt"
+  fi
 }
 
 # Regression test that --remote_download_toplevel does not crash when the