Gracefully handle output symlinks with BwoB

If an action creates output symlinks, `--remote_download_minimal` now falls back to downloading all its outputs rather than failing with an exception, which most of the time led to a broken build.
bazelbuild · Apr 13, 2023 · 0c7c67a · 0c7c67a
1 parent 0e8e611
commit 0c7c67a
Show file tree

Hide file tree

Showing 2 changed files with 40 additions and 7 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
@@ -1142,7 +1142,10 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re
             ||
             // In case the action failed, download all outputs. It might be helpful for debugging
             // and there is no point in injecting output metadata of a failed action.
-            result.getExitCode() != 0;
+            result.getExitCode() != 0
+            ||
+            // Symlinks in actions output are not yet supported with BwoB.
+            !metadata.symlinks().isEmpty();
 
     // Download into temporary paths, then move everything at the end.
     // This avoids holding the output lock while downloading, which would prevent the local branch
@@ -1162,12 +1165,8 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re
       checkState(
           result.getExitCode() == 0,
           "injecting remote metadata is only supported for successful actions (exit code 0).");
-
-      if (!metadata.symlinks().isEmpty()) {
-        throw new IOException(
-            "Symlinks in action outputs are not yet supported by "
-                + "--experimental_remote_download_outputs=minimal");
-      }
+      checkState(metadata.symlinks.isEmpty(), "Symlinks in action outputs are not yet supported by"
+          + " --experimental_remote_download_outputs=minimal");
     }
 
     FileOutErr tmpOutErr = outErr.childOutErr();

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
@@ -17,6 +17,7 @@
 import static com.google.common.truth.Truth.assertThat;
 import static com.google.devtools.build.lib.remote.util.IntegrationTestUtils.startWorker;
 import static org.junit.Assert.assertThrows;
+import static org.junit.Assume.assumeFalse;
 
 import com.google.common.collect.ImmutableList;
 import com.google.devtools.build.lib.actions.Artifact;
@@ -32,6 +33,7 @@
 import com.google.devtools.build.lib.util.OS;
 import com.google.devtools.build.lib.vfs.FileSystemUtils;
 import com.google.devtools.build.lib.vfs.Path;
+import com.google.devtools.build.lib.vfs.Symlinks;
 import java.io.IOException;
 import org.junit.After;
 import org.junit.Test;
@@ -433,6 +435,38 @@ public void symlinkToNestedDirectory() throws Exception {
     buildTarget("//a:one_local", "//a:two_local", "//a:one_remote", "//a:two_remote");
   }
 
+  @Test
+  public void outputSymlinkHandledGracefully() throws Exception {
+    // Symlinks may not be supported on Windows
+    assumeFalse(OS.getCurrent() == OS.WINDOWS);
+    write(
+        "a/defs.bzl",
+        "def _impl(ctx):",
+        "  out = ctx.actions.declare_symlink(ctx.label.name)",
+        "  ctx.actions.run_shell(",
+        "    inputs = [],",
+        "    outputs = [out],",
+        "    command = 'ln -s hello $1',",
+        "    arguments = [out.path],",
+        "  )",
+        "  return DefaultInfo(files = depset([out]))",
+        "",
+        "my_rule = rule(",
+        "  implementation = _impl,",
+        ")");
+
+    write(
+        "a/BUILD",
+        "load(':defs.bzl', 'my_rule')",
+        "",
+        "my_rule(name = 'hello')");
+
+    buildTarget("//a:hello");
+
+    Path outputPath = getOutputPath("a/hello");
+    assertThat(outputPath.stat(Symlinks.NOFOLLOW).isSymbolicLink()).isTrue();
+  }
+
   @Test
   public void replaceOutputDirectoryWithFile() throws Exception {
     write(