Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[6.3.0] Support remote symlink outputs when building without the bytes. #18476

Merged
merged 1 commit into from
May 23, 2023
Merged

[6.3.0] Support remote symlink outputs when building without the bytes. #18476

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
76 changes: 28 additions & 48 deletions src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -844,12 +844,6 @@ private void injectRemoteArtifacts(RemoteAction action, ActionResultMetadata met

for (Map.Entry<Path, DirectoryMetadata> 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(),
Expand Down Expand Up @@ -1082,7 +1076,8 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re

ImmutableList.Builder<ListenableFuture<FileMetadata>> 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
Expand All @@ -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();
Expand Down Expand Up @@ -1139,31 +1130,6 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re

if (downloadOutputs) {
moveOutputsToFinalLocation(downloads, realToTmpPath);

List<SymlinkMetadata> symlinksInDirectories = new ArrayList<>();
for (Entry<Path, DirectoryMetadata> 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<SymlinkMetadata> 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;
Expand Down Expand Up @@ -1197,6 +1163,31 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re
}
}

List<SymlinkMetadata> symlinksInDirectories = new ArrayList<>();
for (Entry<Path, DirectoryMetadata> 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<SymlinkMetadata> 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()) {
Expand Down Expand Up @@ -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;
}
Expand All @@ -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;
}

Expand Down
61 changes: 48 additions & 13 deletions src/test/shell/bazel/remote/build_without_the_bytes_test.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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 <<EOF
def _impl(ctx):
sym = ctx.actions.declare_symlink(ctx.label.name)
ctx.actions.run_shell(
outputs = [sym],
command = "ln -s {} {}".format(ctx.attr.target, sym.path),
)
return DefaultInfo(files = depset([sym]))

symlink = rule(
implementation = _impl,
attrs = {
"target": attr.string(),
},
)
EOF
cat > 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 <<EOF
load(":defs.bzl", "symlink")
symlink(
name = "sym",
target = "target.txt",
)
EOF
}

function test_downloads_toplevel_non_dangling_symlink_output() {
setup_symlink_output
touch pkg/target.txt

bazel build \
--remote_executor=grpc://localhost:${worker_port} \
--remote_download_toplevel \
//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
}

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
Expand Down