From ca30372e210a638cfce8334b6dc3396c83424baa Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Fri, 14 Apr 2023 05:18:39 -0700 Subject: [PATCH] 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. Closes #18075. PiperOrigin-RevId: 524264497 Change-Id: Id0dc77baf1f2c7c54553cf4d7f2d52ce889d1b8b --- .../lib/remote/RemoteExecutionService.java | 42 ++++++++++++------- .../BuildWithoutTheBytesIntegrationTest.java | 30 +++++++++++++ .../remote/build_without_the_bytes_test.sh | 4 +- 3 files changed, 60 insertions(+), 16 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 d073b337e25030..e00ffb04490ea3 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 @@ -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> 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> 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 1922c99c9a674d..240dbee398f59e 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 @@ -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 index 3f58db80c1d7d7..813c3b1de769b0 100755 --- 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" }