[6.2.0] Gracefully handle output symlinks with BwoB

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

@@ -98,7 +98,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;
@@ -1083,13 +1082,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
@@ -1109,12 +1102,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();
@@ -1246,6 +1237,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,

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;
@@ -424,6 +426,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(

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"
 }