Allow unresolved symlink to absolute path in remote action outputs.

Fixes bazelbuild#16290.

PiperOrigin-RevId: 478048404
Change-Id: Ibc5b7f681a520dab6945cb18d976440c9f2e62a7

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

@@ -742,14 +742,6 @@ private void moveOutputsToFinalLocation(
 
   private void createSymlinks(Iterable<SymlinkMetadata> symlinks) throws IOException {
     for (SymlinkMetadata symlink : symlinks) {
-      if (symlink.target().isAbsolute()) {
-        // We do not support absolute symlinks as outputs.
-        throw new IOException(
-            String.format(
-                "Action output %s is a symbolic link to an absolute path %s. "
-                    + "Symlinks to absolute paths in action outputs are not supported.",
-                symlink.path(), symlink.target()));
-      }
       Preconditions.checkNotNull(
               symlink.path().getParentDirectory(),
               "Failed creating directory and parents for %s",
@@ -1116,14 +1108,27 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re
 
       List<SymlinkMetadata> symlinksInDirectories = new ArrayList<>();
       for (Entry<Path, DirectoryMetadata> entry : metadata.directories()) {
-        symlinksInDirectories.addAll(entry.getValue().symlinks());
+        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
+      // might not be supported on all platforms.
       createSymlinks(symlinks);
     } else {
       // TODO(bazel-team): We should unify this if-block to rely on downloadOutputs above but, as of

src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java

@@ -526,7 +526,7 @@ public void downloadOutputs_relativeSymlinkInDirectory_success() throws Exceptio
   }
 
   @Test
-  public void downloadOutputs_absoluteFileSymlink_error() throws Exception {
+  public void downloadOutputs_absoluteFileSymlink_success() throws Exception {
     ActionResult.Builder builder = ActionResult.newBuilder();
     builder.addOutputFileSymlinksBuilder().setPath("outputs/foo").setTarget("/abs/link");
     RemoteActionResult result =
@@ -536,16 +536,16 @@ public void downloadOutputs_absoluteFileSymlink_error() throws Exception {
     RemoteExecutionService service = newRemoteExecutionService();
     RemoteAction action = service.buildRemoteAction(spawn, context);
 
-    IOException expected =
-        assertThrows(IOException.class, () -> service.downloadOutputs(action, result));
+    service.downloadOutputs(action, result);
 
-    assertThat(expected).hasMessageThat().contains("/abs/link");
-    assertThat(expected).hasMessageThat().contains("absolute path");
+    Path path = execRoot.getRelative("outputs/foo");
+    assertThat(path.isSymbolicLink()).isTrue();
+    assertThat(path.readSymbolicLink()).isEqualTo(PathFragment.create("/abs/link"));
     assertThat(context.isLockOutputFilesCalled()).isTrue();
   }
 
   @Test
-  public void downloadOutputs_absoluteDirectorySymlink_error() throws Exception {
+  public void downloadOutputs_absoluteDirectorySymlink_success() throws Exception {
     ActionResult.Builder builder = ActionResult.newBuilder();
     builder.addOutputDirectorySymlinksBuilder().setPath("outputs/foo").setTarget("/abs/link");
     RemoteActionResult result =
@@ -555,11 +555,10 @@ public void downloadOutputs_absoluteDirectorySymlink_error() throws Exception {
     RemoteExecutionService service = newRemoteExecutionService();
     RemoteAction action = service.buildRemoteAction(spawn, context);
 
-    IOException expected =
-        assertThrows(IOException.class, () -> service.downloadOutputs(action, result));
+    service.downloadOutputs(action, result);
 
-    assertThat(expected).hasMessageThat().contains("/abs/link");
-    assertThat(expected).hasMessageThat().contains("absolute path");
+    Path path = execRoot.getRelative("outputs/foo");
+    assertThat(path.readSymbolicLink()).isEqualTo(PathFragment.create("/abs/link"));
     assertThat(context.isLockOutputFilesCalled()).isTrue();
   }
 
@@ -586,8 +585,7 @@ public void downloadOutputs_absoluteSymlinkInDirectory_error() throws Exception
 
     assertThat(expected.getSuppressed()).isEmpty();
     assertThat(expected).hasMessageThat().contains("outputs/dir/link");
-    assertThat(expected).hasMessageThat().contains("/foo");
-    assertThat(expected).hasMessageThat().contains("absolute path");
+    assertThat(expected).hasMessageThat().contains("absolute symlink");
     assertThat(context.isLockOutputFilesCalled()).isTrue();
   }
 

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

@@ -2974,33 +2974,48 @@ function test_external_cc_test_sibling_repository_layout() {
       @other_repo//test >& $TEST_log || fail "Test should pass"
 }
 
-function test_unresolved_symlink_input() {
+function do_test_unresolved_symlink() {
+  local -r strategy=$1
+  local -r link_target=$2
+
   mkdir -p symlink
   touch symlink/BUILD
-  cat > symlink/symlink.bzl <<EOF
-def _dangling_symlink_impl(ctx):
-    symlink = ctx.actions.declare_symlink(ctx.label.name)
+  cat > symlink/symlink.bzl <<'EOF'
+def _unresolved_symlink_impl(ctx):
+  symlink = ctx.actions.declare_symlink(ctx.label.name)
+
+  if ctx.attr.strategy == "internal":
     ctx.actions.symlink(
-        output = symlink,
-        target_path = ctx.attr.link_target,
+      output = symlink,
+      target_path = ctx.attr.link_target,
     )
-    return DefaultInfo(files = depset([symlink]))
+  elif ctx.attr.strategy == "spawn":
+    ctx.actions.run_shell(
+      outputs = [symlink],
+      command = "ln -s $1 $2",
+      arguments = [ctx.attr.link_target, symlink.path],
+    )
+
+  return DefaultInfo(files = depset([symlink]))
 
-dangling_symlink = rule(
-    implementation = _dangling_symlink_impl,
-    attrs = {"link_target": attr.string()},
+unresolved_symlink = rule(
+  implementation = _unresolved_symlink_impl,
+  attrs = {
+    "link_target": attr.string(mandatory = True),
+    "strategy": attr.string(values = ["internal", "spawn"], mandatory = True),
+  },
 )
 EOF
 
   mkdir -p pkg
-  cat > pkg/BUILD <<'EOF'
-load("//symlink:symlink.bzl", "dangling_symlink")
-dangling_symlink(name="a", link_target="non/existent")
+  cat > pkg/BUILD <<EOF
+load("//symlink:symlink.bzl", "unresolved_symlink")
+unresolved_symlink(name="a", link_target="$link_target", strategy="$strategy")
 genrule(
     name = "b",
     srcs = [":a"],
     outs = ["b.txt"],
-    cmd = "readlink $(location :a) > $@",
+    cmd = "readlink \$(location :a) > \$@",
 )
 EOF
 
@@ -3011,7 +3026,10 @@ EOF
     --spawn_strategy=remote \
     --remote_executor=grpc://localhost:${worker_port} \
     //pkg:b &>$TEST_log || fail "expected build to succeed"
-  [[ $(cat bazel-bin/pkg/b.txt) == non/existent ]] || fail "expected symlink target to be non/existent"
+
+  if [[ "$(cat bazel-bin/pkg/b.txt)" != "$link_target" ]]; then
+    fail "expected symlink target to be $link_target"
+  fi
 
   bazel clean --expunge
   bazel \
@@ -3022,7 +3040,26 @@ EOF
     --remote_executor=grpc://localhost:${worker_port} \
     --experimental_remote_merkle_tree_cache \
     //pkg:b &>$TEST_log || fail "expected build to succeed with Merkle tree cache"
-  [[ $(cat bazel-bin/pkg/b.txt) == non/existent ]] || fail "expected symlink target to be non/existent"
+
+  if [[ "$(cat bazel-bin/pkg/b.txt)" != "$link_target" ]]; then
+    fail "expected symlink target to be $link_target"
+  fi
+}
+
+function test_unresolved_symlink_internal_relative() {
+  do_test_unresolved_symlink internal non/existent
+}
+
+function test_unresolved_symlink_internal_absolute() {
+  do_test_unresolved_symlink internal /non/existent
+}
+
+function test_unresolved_symlink_spawn_relative() {
+  do_test_unresolved_symlink spawn non/existent
+}
+
+function test_unresolved_symlink_spawn_absolute() {
+  do_test_unresolved_symlink spawn /non/existent
 }
 
 run_suite "Remote execution and remote cache tests"