Skip to content

Commit

Permalink
Gracefully handle output symlinks with BwoB
Browse files Browse the repository at this point in the history
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
  • Loading branch information
fmeum authored and copybara-github committed Apr 14, 2023
1 parent 9f36740 commit ca30372
Show file tree
Hide file tree
Showing 3 changed files with 60 additions and 16 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -1136,13 +1135,7 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re

ImmutableList.Builder<ListenableFuture<FileMetadata>> 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
Expand All @@ -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();
Expand Down Expand Up @@ -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<ListenableFuture<FileMetadata>> buildFilesToDownload(
RemoteActionExecutionContext context,
ProgressStatusListener progressStatusListener,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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(
Expand Down
4 changes: 2 additions & 2 deletions src/test/shell/bazel/remote/build_without_the_bytes_test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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"
}

Expand Down

0 comments on commit ca30372

Please sign in to comment.