Allow most forms of symlink inside a remotely produced tree artifact,…

… matching a locally produced one.

This CL harmonizes the criteria for permitting a symlink for locally and remotely produced tree artifacts, namely:

1. A symlink to an absolute path is allowed.
2. A symlink to a relative path is allowed, as long as the target is inside the tree artifact.
3. The target path must exist (a consequence of tree artifacts transparently dereferencing symlinks; see also #15454).

as enforced by TreeArtifactValue#visitTree (see https://cs.opensource.google/bazel/bazel/+/master:src/main/java/com/google/devtools/build/lib/skyframe/TreeArtifactValue.java;l=585;drc=de9d1f59915a36229978d46b78a22c9e5389db92).

The admonition about symlinks potentially compromising hermeticity is still valid, but there's little gain in making the behavior divergent between local and remote execution. I don't believe we can go in the other direction and extend the restriction to cover local execution, as it would break existing projects.

The --incompatible_remote_disallow_symlink_in_tree_artifact flag is deleted. It was added at a time when symlink resolution was extremely unreliable when building without the bytes; the state of symlink handling has improved a lot since then. I'm deleting the flag entirely (as opposed to making it a no-op) because it was only introduced in the Bazel 7 tree and hasn't made it into a stable release yet.

Makes #18284 obsolete.

PiperOrigin-RevId: 587691220
Change-Id: I470a8fc523bdbb7577ad5a564b6b2c0acab17d7d

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

@@ -1234,23 +1234,6 @@ 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);
       }
     }

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

@@ -423,17 +423,6 @@ public RemoteBuildEventUploadModeConverter() {
               + " outputs are allowed to dangle.")
   public boolean incompatibleRemoteDanglingSymlinks;
 
-  @Option(
-      name = "incompatible_remote_disallow_symlink_in_tree_artifact",
-      defaultValue = "true",
-      documentationCategory = OptionDocumentationCategory.REMOTE,
-      effectTags = {OptionEffectTag.EXECUTION},
-      metadataTags = {OptionMetadataTag.INCOMPATIBLE_CHANGE},
-      help =
-          "If set to true, a remotely executed action cannot produce a tree artifact containing a"
-              + " relative symlink. Absolute symlinks are never allowed irrespective of this flag.")
-  public boolean incompatibleRemoteDisallowSymlinkInTreeArtifact;
-
   @Option(
       name = "remote_cache_compression",
       oldName = "experimental_remote_cache_compression",

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

@@ -698,12 +698,13 @@ public void downloadOutputs_outputSymlinksCompatibility_success() throws Excepti
   }
 
   @Test
-  public void downloadOutputs_relativeSymlinkInDirectory_success() throws Exception {
+  public void downloadOutputs_symlinksInDirectory_success() throws Exception {
     Tree tree =
         Tree.newBuilder()
             .setRoot(
                 Directory.newBuilder()
-                    .addSymlinks(SymlinkNode.newBuilder().setName("link").setTarget("../foo")))
+                    .addSymlinks(SymlinkNode.newBuilder().setName("rel").setTarget("foo"))
+                    .addSymlinks(SymlinkNode.newBuilder().setName("abs").setTarget("/bar")))
             .build();
     Digest treeDigest = cache.addContents(remoteActionExecutionContext, tree.toByteArray());
     ActionResult.Builder builder = ActionResult.newBuilder();
@@ -712,7 +713,6 @@ public void downloadOutputs_relativeSymlinkInDirectory_success() throws Exceptio
         RemoteActionResult.createFromCache(CachedActionResult.remote(builder.build()));
     Spawn spawn = newSpawnFromResult(result);
     FakeSpawnExecutionContext context = newSpawnExecutionContext(spawn);
-    remoteOptions.incompatibleRemoteDisallowSymlinkInTreeArtifact = false;
     RemoteExecutionService service = newRemoteExecutionService(remoteOptions);
     RemoteAction action = service.buildRemoteAction(spawn, context);
     createOutputDirectories(spawn);
@@ -722,9 +722,12 @@ public void downloadOutputs_relativeSymlinkInDirectory_success() throws Exceptio
     // Doesn't check for dangling links, hence download succeeds.
     service.downloadOutputs(action, result);
 
-    Path path = execRoot.getRelative("outputs/dir/link");
-    assertThat(path.isSymbolicLink()).isTrue();
-    assertThat(path.readSymbolicLink()).isEqualTo(PathFragment.create("../foo"));
+    Path relPath = execRoot.getRelative("outputs/dir/rel");
+    assertThat(relPath.isSymbolicLink()).isTrue();
+    assertThat(relPath.readSymbolicLink()).isEqualTo(PathFragment.create("foo"));
+    Path absPath = execRoot.getRelative("outputs/dir/abs");
+    assertThat(absPath.isSymbolicLink()).isTrue();
+    assertThat(absPath.readSymbolicLink()).isEqualTo(PathFragment.create("/bar"));
     assertThat(context.isLockOutputFilesCalled()).isTrue();
   }
 
@@ -771,37 +774,6 @@ public void downloadOutputs_absoluteDirectorySymlink_success() throws Exception
     assertThat(context.isLockOutputFilesCalled()).isTrue();
   }
 
-  @Test
-  public void downloadOutputs_absoluteSymlinkInDirectory_error() throws Exception {
-    Tree tree =
-        Tree.newBuilder()
-            .setRoot(
-                Directory.newBuilder()
-                    .addSymlinks(SymlinkNode.newBuilder().setName("link").setTarget("/foo")))
-            .build();
-    Digest treeDigest = cache.addContents(remoteActionExecutionContext, tree.toByteArray());
-    ActionResult.Builder builder = ActionResult.newBuilder();
-    builder.addOutputDirectoriesBuilder().setPath("outputs/dir").setTreeDigest(treeDigest);
-    RemoteActionResult result =
-        RemoteActionResult.createFromCache(CachedActionResult.remote(builder.build()));
-    Spawn spawn = newSpawnFromResult(result);
-    FakeSpawnExecutionContext context = newSpawnExecutionContext(spawn);
-    RemoteExecutionService service = newRemoteExecutionService();
-    RemoteAction action = service.buildRemoteAction(spawn, context);
-    createOutputDirectories(spawn);
-    when(remoteOutputChecker.shouldDownloadOutput(ArgumentMatchers.<PathFragment>any()))
-        .thenReturn(true);
-
-    IOException expected =
-        assertThrows(IOException.class, () -> service.downloadOutputs(action, result));
-
-    assertThat(expected.getSuppressed()).isEmpty();
-    assertThat(expected)
-        .hasMessageThat()
-        .isEqualTo("Unsupported symlink 'link' inside tree artifact 'outputs/dir'");
-    assertThat(context.isLockOutputFilesCalled()).isTrue();
-  }
-
   @Test
   public void downloadOutputs_symlinkCollision_error() throws Exception {
     ActionResult.Builder builder = ActionResult.newBuilder();

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

@@ -935,7 +935,6 @@ function test_symlinks_in_directory() {
     # for one of the declared outputs which is not supported when BwoB.
     bazel build \
           --incompatible_remote_symlinks \
-          --noincompatible_remote_disallow_symlink_in_tree_artifact \
           --noincompatible_disallow_unsound_directory_outputs \
           --remote_executor=grpc://localhost:${worker_port} \
           --remote_download_all \
@@ -961,7 +960,6 @@ function test_symlinks_in_directory_cache_only() {
     set_symlinks_in_directory_testfixtures
     bazel build \
           --incompatible_remote_symlinks \
-          --noincompatible_remote_disallow_symlink_in_tree_artifact \
           --noincompatible_disallow_unsound_directory_outputs \
           --remote_cache=grpc://localhost:${worker_port} \
           --remote_download_all \
@@ -972,7 +970,6 @@ function test_symlinks_in_directory_cache_only() {
     bazel clean # Get rid of local results, rely on remote cache.
     bazel build \
           --incompatible_remote_symlinks \
-          --noincompatible_remote_disallow_symlink_in_tree_artifact \
           --noincompatible_disallow_unsound_directory_outputs \
           --remote_cache=grpc://localhost:${worker_port} \
           --remote_download_all \
@@ -2454,22 +2451,23 @@ function test_create_tree_artifact_outputs_remote_cache() {
   [[ -d bazel-bin/pkg/a/empty_dir ]] || fail "expected directory to exist"
 }
 
-function test_symlink_in_tree_artifact() {
+function test_symlinks_in_tree_artifact() {
   mkdir -p pkg
 
-  cat > pkg/defs.bzl <<EOF
+  cat > pkg/defs.bzl <<'EOF'
 def _impl(ctx):
   d = ctx.actions.declare_directory(ctx.label.name)
   ctx.actions.run_shell(
     outputs = [d],
-    command = "cd %s && touch foo && ln -s foo sym" % d.path,
+    command = "cd $1 && mkdir dir && touch dir/file.txt && ln -s dir dsym && ln -s dir/file.txt fsym",
+    arguments = [d.path],
   )
   return DefaultInfo(files = depset([d]))
 
 tree = rule(implementation = _impl)
 EOF
 
-  cat > pkg/BUILD <<EOF
+  cat > pkg/BUILD <<'EOF'
 load(":defs.bzl", "tree")
 
 tree(name = "tree")
@@ -2478,17 +2476,15 @@ EOF
   bazel build \
       --spawn_strategy=remote \
       --remote_executor=grpc://localhost:${worker_port} \
-      --noincompatible_remote_disallow_symlink_in_tree_artifact \
       //pkg:tree &>$TEST_log || fail "Expected build to succeed"
 
-  bazel clean
-
-  bazel build \
-      --spawn_strategy=remote \
-      --remote_executor=grpc://localhost:${worker_port} \
-      //pkg:tree &>$TEST_log && fail "Expected build to fail"
+  if [[ "$(readlink bazel-bin/pkg/tree/fsym)" != "dir/file.txt" ]]; then
+    fail "expected bazel-bin/pkg/tree/fsym to be a symlink to dir/file.txt"
+  fi
 
-  expect_log "Unsupported symlink 'sym' inside tree artifact"
+  if [[ "$(readlink bazel-bin/pkg/tree/dsym)" != "dir" ]]; then
+    fail "expected bazel-bin/tree/dsym to be a symlink to dir"
+  fi
 }
 
 # Runs coverage with `cc_test` and RE then checks the coverage file is returned.