[6.3.0] Support remote symlink outputs when building without the byte…

…s. (#18476) Fixes #13355. PiperOrigin-RevId: 534008963 Change-Id: Ia4f22f16960bcdae86c1a8820e3d47a3689876d8
bazelbuild · May 23, 2023 · c422565 · c422565
1 parent 92cc903
commit c422565
Show file tree

Hide file tree

Showing 2 changed files with 76 additions and 61 deletions.
diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java
@@ -844,12 +844,6 @@ private void injectRemoteArtifacts(RemoteAction action, ActionResultMetadata met
 
     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(),
@@ -1082,7 +1076,8 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re
 
     ImmutableList.Builder<ListenableFuture<FileMetadata>> downloadsBuilder =
         ImmutableList.builder();
-    boolean downloadOutputs = shouldDownloadOutputsFor(result, metadata);
+
+    boolean downloadOutputs = shouldDownloadOutputsFor(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
@@ -1102,10 +1097,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();
@@ -1139,31 +1130,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 the symlinks
-          // locally is asking for trouble. Sadly, we did start permitting relative symlinks at some
-          // point, so we can only ban the absolute ones.
-          // See https://github.com/bazelbuild/bazel/issues/16361.
-          if (symlink.target().isAbsolute()) {
-            throw new IOException(
-                String.format(
-                    "Unsupported absolute symlink '%s' inside tree artifact '%s'",
-                    symlink.path(), entry.getKey()));
-          }
-          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;
@@ -1197,6 +1163,31 @@ 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 the symlinks
+        // locally is asking for trouble. Sadly, we did start permitting relative symlinks at some
+        // point, so we can only ban the absolute ones.
+        // See https://github.com/bazelbuild/bazel/issues/16361.
+        if (symlink.target().isAbsolute()) {
+          throw new IOException(
+              String.format(
+                  "Unsupported absolute symlink '%s' inside tree artifact '%s'",
+                  symlink.path(), entry.getKey()));
+        }
+        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()) {
@@ -1237,8 +1228,7 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re
     return null;
   }
 
-  private boolean shouldDownloadOutputsFor(
-      RemoteActionResult result, ActionResultMetadata metadata) {
+  private boolean shouldDownloadOutputsFor(RemoteActionResult result) {
     if (remoteOptions.remoteOutputsMode.downloadAllOutputs()) {
       return true;
     }
@@ -1247,16 +1237,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;
-    }
     return false;
   }
 

diff --git a/src/test/shell/bazel/remote/build_without_the_bytes_test.sh b/src/test/shell/bazel/remote/build_without_the_bytes_test.sh
@@ -360,7 +360,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
@@ -409,25 +409,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
-  cat > a/input.txt <<'EOF'
-Input file
+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 = [sym],
+    command = "ln -s {} {}".format(ctx.attr.target, sym.path),
+  )
+  return DefaultInfo(files = depset([sym]))
+
+symlink = rule(
+  implementation = _impl,
+  attrs = {
+    "target": attr.string(),
+  },
+)
 EOF
-  cat > a/BUILD <<'EOF'
-genrule(
-  name = "foo",
-  srcs = ["input.txt"],
-  outs = ["output.txt", "output_symlink", "output_symlink_2"],
-  cmd = "cp $< $(location :output.txt) && ln -s output.txt $(location output_symlink) && ln -s output.txt $(location output_symlink_2)",
+
+  cat > pkg/BUILD <<EOF
+load(":defs.bzl", "symlink")
+symlink(
+  name = "sym",
+  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:foo >& $TEST_log || fail "Expected build of //a:foo 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