Skip to content

Commit

Permalink
Support remote symlink outputs when building without the bytes.
Browse files Browse the repository at this point in the history 
Fixes #13355.

PiperOrigin-RevId: 534008963
Change-Id: Ia4f22f16960bcdae86c1a8820e3d47a3689876d8
  • Loading branch information
@tjgq @copybara-github
tjgq authored and copybara-github committed May 22, 2023
1 parent 997be1e commit 0ff6b88
Show file tree
Hide file tree
Showing 2 changed files with 73 additions and 83 deletions.
89 changes: 34 additions & 55 deletions src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -866,12 +866,6 @@ private void injectRemoteArtifacts(

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 @@ -1162,7 +1156,8 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re

ImmutableList.Builder<ListenableFuture<FileMetadata>> 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
Expand All @@ -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();
Expand Down Expand Up @@ -1219,38 +1210,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 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<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 @@ -1285,6 +1244,37 @@ 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 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<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 @@ -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;
}
Expand All @@ -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()) {
Expand Down
67 changes: 39 additions & 28 deletions src/test/shell/bazel/remote/build_without_the_bytes_test.sh
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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 <<EOF
def _impl(ctx):
sym = ctx.actions.declare_symlink(ctx.label.name)
ctx.actions.run_shell(
outputs = outputs,
command = " && ".join(commands),
outputs = [sym],
command = "ln -s {} {}".format(ctx.attr.target, sym.path),
)
return DefaultInfo(files = depset([sym]))
return DefaultInfo(files = depset(outputs))
symlinks = rule(
symlink = rule(
implementation = _impl,
attrs = {
"symlink_map": attr.label_keyed_string_dict(allow_files = True),
"target": attr.string(),
},
)
EOF
cat > a/BUILD <<'EOF'
load(":defs.bzl", "symlinks")
symlinks(

cat > pkg/BUILD <<EOF
load(":defs.bzl", "symlink")
symlink(
name = "sym",
symlink_map = {
"file1.txt": "sym1",
"file2.txt": "sym2",
},
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: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
Expand Down

2 comments on commit 0ff6b88

@brentleyjones
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tjgq Is this cherry-pickable into 6.3?

@tjgq
Copy link
Contributor Author

@tjgq tjgq commented on 0ff6b88 May 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tjgq Is this cherry-pickable into 6.3?

Cherry-picks are getting trickier since 6.3 and 7.0 have diverged quite a bit, but this one was simple enough to fix the merge conflicts. See #18476.

Please sign in to comment.