From 94338402aedcab55cb146ffe49feac032a0204ac Mon Sep 17 00:00:00 2001
From: Tiago Quelhas <tjgq@google.com>
Date: Thu, 7 Dec 2023 19:03:44 +0100
Subject: [PATCH] [6.5.0] Fix tree file materialized as symlink to another file
 when building without the bytes. (#20409)

This is the same bug as #19143, except that the fix in 3a48457 missed
the case where the symlink occurs inside an output directory.

Fixes #20408.
---
 .../lib/remote/RemoteExecutionService.java    | 20 ++++++++++---
 .../BuildWithoutTheBytesIntegrationTest.java  | 28 ++++++++++++++++++-
 2 files changed, 43 insertions(+), 5 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
index 506b6da393a9d0..47434f2d1f944f 100644
--- a/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java
+++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java
@@ -1100,7 +1100,7 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re
     ImmutableList.Builder<ListenableFuture<FileMetadata>> downloadsBuilder =
         ImmutableList.builder();
 
-    boolean downloadOutputs = shouldDownloadOutputsFor(result);
+    boolean downloadOutputs = shouldDownloadOutputsFor(result, metadata);
 
     // Download into temporary paths, then move everything at the end.
     // This avoids holding the output lock while downloading, which would prevent the local branch
@@ -1251,13 +1251,13 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re
     return null;
   }
 
-  private boolean shouldDownloadOutputsFor(RemoteActionResult result) {
+  private boolean shouldDownloadOutputsFor(RemoteActionResult result,
+      ActionResultMetadata metadata) {
     if (remoteOptions.remoteOutputsMode.downloadAllOutputs()) {
       return true;
     }
     // An output materialized as a symlink might point to one of the other outputs.
-    if (!result.getOutputSymlinks().isEmpty() || !result.getOutputFileSymlinks().isEmpty()
-        || !result.getOutputDirectorySymlinks().isEmpty()) {
+    if (resultHasSymlinks(result, metadata)) {
       return true;
     }
     // In case the action failed, download all outputs. It might be helpful for debugging and there
@@ -1268,6 +1268,18 @@ private boolean shouldDownloadOutputsFor(RemoteActionResult result) {
     return false;
   }
 
+  private boolean resultHasSymlinks(RemoteActionResult result, ActionResultMetadata metadata) {
+    if (!metadata.symlinks().isEmpty()) {
+      return true;
+    }
+    for (Entry<Path, DirectoryMetadata> entry : metadata.directories()) {
+      if (!entry.getValue().symlinks().isEmpty()) {
+        return true;
+      }
+    }
+    return false;
+  }
+
   private ImmutableList<ListenableFuture<FileMetadata>> buildFilesToDownload(
       RemoteActionExecutionContext context,
       ProgressStatusListener progressStatusListener,
diff --git a/src/test/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTest.java b/src/test/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTest.java
index e0ca5951cf40ce..8e551b1759787d 100644
--- a/src/test/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTest.java
+++ b/src/test/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTest.java
@@ -713,13 +713,39 @@ public void downloadTopLevel_genruleSymlinkToOutput() throws Exception {
 
     // Delete target, re-download it
     getOutputPath("foo").delete();
-
     buildTarget("//:gen");
 
     assertSymlink("foo-link", PathFragment.create("foo"));
     assertValidOutputFile("foo-link", "hello\n");
   }
 
+  @Test
+  public void downloadTopLevel_treeContainingSymlink() throws Exception {
+    // Disable test on Windows.
+    assumeFalse(OS.getCurrent() == OS.WINDOWS);
+
+    setDownloadToplevel();
+    write("tree.bzl", "def _impl(ctx):",
+        "  d = ctx.actions.declare_directory(ctx.label.name)",
+        "  ctx.actions.run_shell(",
+        "    outputs = [d],",
+        "    command = 'cd {} && echo hello > file.txt && ln -s file.txt sym.txt'.format(d.path),",
+        "  )",
+        "  return DefaultInfo(files = depset([d]))",
+        "tree = rule(_impl)");
+    write(
+        "BUILD",
+        "load('tree.bzl', 'tree')",
+        "tree(",
+        "  name = 'tree',",
+        ")");
+
+    buildTarget("//:tree");
+
+    assertSymlink("tree/sym.txt", PathFragment.create("file.txt"));
+    assertValidOutputFile("tree/sym.txt", "hello\n");
+  }
+
   protected void assertSymlink(String binRelativeLinkPath, PathFragment targetPath)
       throws Exception {
     Path output = getOutputPath(binRelativeLinkPath);