From 3bc354fb7be865ce572ce788218bd7fda280f5f4 Mon Sep 17 00:00:00 2001 From: Googler Date: Mon, 22 May 2023 03:08:36 -0700 Subject: [PATCH] Support remote symlink outputs when building without the bytes. Fixes #13355. PiperOrigin-RevId: 534008963 Change-Id: Ia4f22f16960bcdae86c1a8820e3d47a3689876d8 --- .../lib/remote/RemoteExecutionService.java | 89 +++++++------------ .../remote/build_without_the_bytes_test.sh | 67 ++++++++------ 2 files changed, 73 insertions(+), 83 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 20fd3c01873363..b9032d0653dfc9 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 @@ -866,12 +866,6 @@ private void injectRemoteArtifacts( 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(), @@ -1162,7 +1156,8 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re ImmutableList.Builder> downloadsBuilder = ImmutableList.builder(); - boolean downloadOutputs = shouldDownloadOutputsFor(action, result, metadata); + + boolean downloadOutputs = shouldDownloadOutputsFor(action, 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 @@ -1182,10 +1177,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(); @@ -1219,38 +1210,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 a remotely - // produced symlink against the local filesystem is asking for trouble. - // - // Sadly, we started permitting relative symlinks at some point, so we have to allow them - // until the --incompatible_remote_disallow_symlink_in_tree_artifact flag is flipped. - // Absolute symlinks, on the other hand, have never been allowed. - // - // See also https://github.com/bazelbuild/bazel/issues/16361 for potential future work - // to allow *unresolved* symlinks in a tree artifact. - boolean isAbsolute = symlink.target().isAbsolute(); - if (remoteOptions.incompatibleRemoteDisallowSymlinkInTreeArtifact || isAbsolute) { - throw new IOException( - String.format( - "Unsupported symlink '%s' inside tree artifact '%s'", - symlink.path().relativeTo(entry.getKey()), - entry.getKey().relativeTo(execRoot))); - } - 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; @@ -1285,6 +1244,37 @@ 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 a remotely + // produced symlink against the local filesystem is asking for trouble. + // + // Sadly, we started permitting relative symlinks at some point, so we have to allow them + // until the --incompatible_remote_disallow_symlink_in_tree_artifact flag is flipped. + // Absolute symlinks, on the other hand, have never been allowed. + // + // See also https://github.com/bazelbuild/bazel/issues/16361 for potential future work + // to allow *unresolved* symlinks in a tree artifact. + boolean isAbsolute = symlink.target().isAbsolute(); + if (remoteOptions.incompatibleRemoteDisallowSymlinkInTreeArtifact || isAbsolute) { + throw new IOException( + String.format( + "Unsupported symlink '%s' inside tree artifact '%s'", + symlink.path().relativeTo(entry.getKey()), entry.getKey().relativeTo(execRoot))); + } + 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()) { @@ -1325,8 +1315,7 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re return null; } - private boolean shouldDownloadOutputsFor( - RemoteAction action, RemoteActionResult result, ActionResultMetadata metadata) { + private boolean shouldDownloadOutputsFor(RemoteAction action, RemoteActionResult result) { if (remoteOptions.remoteOutputsMode.downloadAllOutputs()) { return true; } @@ -1335,16 +1324,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; - } checkNotNull(remoteOutputChecker, "remoteOutputChecker must not be null"); for (var output : action.getSpawn().getOutputFiles()) { 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 1adb1eceb71641..199b09e99d3181 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 @@ -370,7 +370,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 @@ -419,49 +419,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 - touch a/file1.txt a/file2.txt - cat > a/defs.bzl <<'EOF' -def _impl(ctx): - commands = [] - outputs = [] - for target, name in ctx.attr.symlink_map.items(): - sym = ctx.actions.declare_symlink(name) - file = target.files.to_list()[0] - outputs.append(sym) - commands.append("ln -s {} {}".format(file.path, sym.path)) +function setup_symlink_output() { + mkdir -p pkg + cat > pkg/defs.bzl < a/BUILD <<'EOF' -load(":defs.bzl", "symlinks") -symlinks( + + 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:sym >& $TEST_log || fail "Expected build of //a:sym 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