From a1c53df4544981f80320b53fc90d2051dd4620f0 Mon Sep 17 00:00:00 2001 From: Tiago Quelhas Date: Tue, 23 May 2023 15:09:22 +0200 Subject: [PATCH] [6.3.0] Support remote symlink outputs when building without the bytes. Fixes #13355. PiperOrigin-RevId: 534008963 Change-Id: Ia4f22f16960bcdae86c1a8820e3d47a3689876d8 --- .../lib/remote/RemoteExecutionService.java | 76 +++++++------------ .../remote/build_without_the_bytes_test.sh | 61 +++++++++++---- 2 files changed, 76 insertions(+), 61 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 68e3277d774013..c3cae384b5e7ee 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 @@ -844,12 +844,6 @@ private void injectRemoteArtifacts(RemoteAction action, ActionResultMetadata met for (Map.Entry entry : metadata.directories()) { DirectoryMetadata directory = entry.getValue(); - if (!directory.symlinks().isEmpty()) { - throw new IOException( - "Symlinks in action outputs are not yet supported by " - + "--experimental_remote_download_outputs=minimal"); - } - for (FileMetadata file : directory.files()) { remoteActionFileSystem.injectRemoteFile( file.path().asFragment(), @@ -1082,7 +1076,8 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re ImmutableList.Builder> downloadsBuilder = ImmutableList.builder(); - boolean downloadOutputs = shouldDownloadOutputsFor(result, metadata); + + boolean downloadOutputs = shouldDownloadOutputsFor(result); // Download into temporary paths, then move everything at the end. // This avoids holding the output lock while downloading, which would prevent the local branch @@ -1102,10 +1097,6 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re checkState( result.getExitCode() == 0, "injecting remote metadata is only supported for successful actions (exit code 0)."); - checkState( - metadata.symlinks.isEmpty(), - "Symlinks in action outputs are not yet supported by" - + " --remote_download_outputs=minimal"); } FileOutErr tmpOutErr = outErr.childOutErr(); @@ -1139,31 +1130,6 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re if (downloadOutputs) { moveOutputsToFinalLocation(downloads, realToTmpPath); - - List symlinksInDirectories = new ArrayList<>(); - for (Entry entry : metadata.directories()) { - for (SymlinkMetadata symlink : entry.getValue().symlinks()) { - // Symlinks should not be allowed inside directories because their semantics are unclear: - // tree artifacts are defined as a collection of regular files, and resolving the symlinks - // locally is asking for trouble. Sadly, we did start permitting relative symlinks at some - // point, so we can only ban the absolute ones. - // See https://github.com/bazelbuild/bazel/issues/16361. - if (symlink.target().isAbsolute()) { - throw new IOException( - String.format( - "Unsupported absolute symlink '%s' inside tree artifact '%s'", - symlink.path(), entry.getKey())); - } - symlinksInDirectories.add(symlink); - } - } - - Iterable symlinks = - Iterables.concat(metadata.symlinks(), symlinksInDirectories); - - // Create the symbolic links after all downloads are finished, because dangling symlinks - // might not be supported on all platforms. - createSymlinks(symlinks); } else { ActionInput inMemoryOutput = null; Digest inMemoryOutputDigest = null; @@ -1197,6 +1163,31 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re } } + List symlinksInDirectories = new ArrayList<>(); + for (Entry entry : metadata.directories()) { + for (SymlinkMetadata symlink : entry.getValue().symlinks()) { + // Symlinks should not be allowed inside directories because their semantics are unclear: + // tree artifacts are defined as a collection of regular files, and resolving the symlinks + // locally is asking for trouble. Sadly, we did start permitting relative symlinks at some + // point, so we can only ban the absolute ones. + // See https://github.com/bazelbuild/bazel/issues/16361. + if (symlink.target().isAbsolute()) { + throw new IOException( + String.format( + "Unsupported absolute symlink '%s' inside tree artifact '%s'", + symlink.path(), entry.getKey())); + } + symlinksInDirectories.add(symlink); + } + } + + Iterable symlinks = + Iterables.concat(metadata.symlinks(), symlinksInDirectories); + + // Create the symbolic links after all downloads are finished, because dangling symlinks + // might not be supported on all platforms. + createSymlinks(symlinks); + if (result.success()) { // Check that all mandatory outputs are created. for (ActionInput output : action.getSpawn().getOutputFiles()) { @@ -1237,8 +1228,7 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re return null; } - private boolean shouldDownloadOutputsFor( - RemoteActionResult result, ActionResultMetadata metadata) { + private boolean shouldDownloadOutputsFor(RemoteActionResult result) { if (remoteOptions.remoteOutputsMode.downloadAllOutputs()) { return true; } @@ -1247,16 +1237,6 @@ private boolean shouldDownloadOutputsFor( 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.get(metadata.symlinks(), 0).path()))); - return true; - } return false; } 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 0f884b2a155c54..470a0998920cd0 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 @@ -360,7 +360,7 @@ EOF # Test that --remote_download_toplevel fetches inputs to symlink actions. In # particular, cc_binary links against a symlinked imported .so file, and only # the symlink is in the runfiles. -function test_downloads_toplevel_symlinks() { +function test_downloads_toplevel_symlink_action() { if [[ "$PLATFORM" == "darwin" ]]; then # TODO(b/37355380): This test is disabled due to RemoteWorker not supporting # setting SDKROOT and DEVELOPER_DIR appropriately, as is required of @@ -409,25 +409,60 @@ EOF ./bazel-bin/a/foo${EXE_EXT} || fail "bazel-bin/a/foo${EXE_EXT} failed to run" } -function test_symlink_outputs_warning_with_minimal() { - mkdir -p a - cat > a/input.txt <<'EOF' -Input file +function setup_symlink_output() { + mkdir -p pkg + + cat > pkg/defs.bzl < a/BUILD <<'EOF' -genrule( - name = "foo", - srcs = ["input.txt"], - outs = ["output.txt", "output_symlink", "output_symlink_2"], - cmd = "cp $< $(location :output.txt) && ln -s output.txt $(location output_symlink) && ln -s output.txt $(location output_symlink_2)", + + cat > pkg/BUILD <& $TEST_log || fail "Expected build of //pkg:sym to succeed" + + if [[ "$(readlink bazel-bin/pkg/sym)" != "target.txt" ]]; then + fail "Expected bazel-bin/pkg/sym to be a symlink pointing to target.txt" + fi +} + +function test_downloads_toplevel_dangling_symlink_output() { + setup_symlink_output bazel build \ --remote_executor=grpc://localhost:${worker_port} \ --remote_download_minimal \ - //a:foo >& $TEST_log || fail "Expected build of //a:foo to succeed" - expect_log "Symlinks in action outputs are not yet supported" + //pkg:sym >& $TEST_log || fail "Expected build of //pkg:sym to succeed" + + if [[ "$(readlink bazel-bin/pkg/sym)" != "target.txt" ]]; then + fail "Expected bazel-bin/pkg/sym to be a symlink pointing to target.txt" + fi } # Regression test that --remote_download_toplevel does not crash when the