Gracefully handle output symlinks with BwoB

fmeum · copybara-github · commit ca30372e210a · 2023-04-14T05:20:16.000-07:00
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. Closes #18075. PiperOrigin-RevId: 524264497 Change-Id: Id0dc77baf1f2c7c54553cf4d7f2d52ce889d1b8b
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
@@ -99,7 +99,6 @@
 import com.google.devtools.build.lib.remote.common.RemotePathResolver;
 import com.google.devtools.build.lib.remote.merkletree.MerkleTree;
 import com.google.devtools.build.lib.remote.options.RemoteOptions;
-import com.google.devtools.build.lib.remote.options.RemoteOutputsMode;
 import com.google.devtools.build.lib.remote.util.DigestUtil;
 import com.google.devtools.build.lib.remote.util.TempPathGenerator;
 import com.google.devtools.build.lib.remote.util.TracingMetadataUtils;
@@ -1136,13 +1135,7 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re
 
     ImmutableList.Builder<ListenableFuture<FileMetadata>> downloadsBuilder =
         ImmutableList.builder();
-    RemoteOutputsMode remoteOutputsMode = remoteOptions.remoteOutputsMode;
-    boolean downloadOutputs =
-        remoteOutputsMode.downloadAllOutputs()
-            ||
-            // 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;
+    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
@@ -1162,12 +1155,10 @@ 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"
+              + " --remote_download_outputs=minimal");
     }
 
     FileOutErr tmpOutErr = outErr.childOutErr();
@@ -1300,6 +1291,29 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re
     return null;
   }
 
+  private boolean shouldDownloadOutputsFor(
+      RemoteActionResult result, ActionResultMetadata metadata) {
+    if (remoteOptions.remoteOutputsMode.downloadAllOutputs()) {
+      return true;
+    }
+    // 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.
+    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.getOnlyElement(metadata.symlinks()).path())));
+      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
@@ -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,34 @@ 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(
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
@@ -409,7 +409,7 @@ EOF
   ./bazel-bin/a/foo${EXE_EXT} || fail "bazel-bin/a/foo${EXE_EXT} failed to run"
 }
 
-function test_symlink_outputs_not_allowed_with_minimial() {
+function test_symlink_outputs_warning_with_minimal() {
   mkdir -p a
   cat > a/input.txt <<'EOF'
 Input file
@@ -426,7 +426,7 @@ EOF
   bazel build \
     --remote_executor=grpc://localhost:${worker_port} \
     --remote_download_minimal \
-    //a:foo >& $TEST_log && fail "Expected failure to build //a:foo"
+    //a:foo >& $TEST_log || fail "Expected build of //a:foo to succeed"
   expect_log "Symlinks in action outputs are not yet supported"
 }
 
