From 746eb9dd58d609482dc452ed4e0c2e8e6aa5bc94 Mon Sep 17 00:00:00 2001 From: Ed Schouten Date: Thu, 31 Aug 2023 14:17:58 +0200 Subject: [PATCH] Let RemoteOutputService ensure bazel-out/ is not a symlink When doing a regular build without passing in --remote_download_*, ExecutionTool will automatically clean up any traces of an OutputService that replaces bazel-out/ with a symlink pointing to a remote file system. The --remote_download_* option is implemented by installing an OutputService of its own, meaning that the regular logic in ExecutionTool is skipped. The rationale being that an OutputService essentially takes ownership of bazel-out/. This change extends RemoteOutputService to be a more complete implementation of OutputService that ensures that bazel-out/ is set up properly. This improves interoperability with changes such as the gRPC based Remote Output Service protocol (see #12823). Change-Id: I5bedea2895785f3b8836fe8c5f8ae97ca2689233 --- .../build/lib/remote/RemoteModule.java | 6 +++- .../build/lib/remote/RemoteOutputService.java | 30 ++++++++++++++++++ .../remote/build_without_the_bytes_test.sh | 31 +++++++++++++++++++ 3 files changed, 66 insertions(+), 1 deletion(-) diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java index 4e36f75def20eb..e099e2ea269530 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java @@ -141,6 +141,7 @@ public final class RemoteModule extends BlazeModule { @Nullable private RemoteActionContextProvider actionContextProvider; @Nullable private RemoteActionInputFetcher actionInputFetcher; @Nullable private RemoteOptions remoteOptions; + @Nullable private CommandEnvironment env; @Nullable private RemoteOutputService remoteOutputService; @Nullable private TempPathGenerator tempPathGenerator; @Nullable private BlockWaitingModule blockWaitingModule; @@ -303,6 +304,7 @@ public void beforeCommand(CommandEnvironment env) throws AbruptExitException { Preconditions.checkState(actionContextProvider == null, "actionContextProvider must be null"); Preconditions.checkState(actionInputFetcher == null, "actionInputFetcher must be null"); Preconditions.checkState(remoteOptions == null, "remoteOptions must be null"); + Preconditions.checkState(env == null, "env must be null"); Preconditions.checkState(tempPathGenerator == null, "tempPathGenerator must be null"); Preconditions.checkState(remoteOutputChecker == null, "remoteOutputChecker must be null"); @@ -313,6 +315,7 @@ public void beforeCommand(CommandEnvironment env) throws AbruptExitException { } this.remoteOptions = remoteOptions; + this.env = env; AuthAndTLSOptions authAndTlsOptions = env.getOptions().getOptions(AuthAndTLSOptions.class); DigestHashFunction hashFn = env.getRuntime().getFileSystem().getDigestFunction(); @@ -914,6 +917,7 @@ public void afterCommand() { actionContextProvider = null; actionInputFetcher = null; remoteOptions = null; + env = null; remoteOutputService = null; tempPathGenerator = null; rpcLogFile = null; @@ -1067,7 +1071,7 @@ public void executorInit(CommandEnvironment env, BuildRequest request, ExecutorB public OutputService getOutputService() { Preconditions.checkState(remoteOutputService == null, "remoteOutputService must be null"); if (actionContextProvider.getRemoteCache() != null) { - remoteOutputService = new RemoteOutputService(); + remoteOutputService = new RemoteOutputService(env); } return remoteOutputService; } diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteOutputService.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteOutputService.java index a259bb071e86e2..a362489b3772ef 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteOutputService.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteOutputService.java @@ -34,11 +34,17 @@ import com.google.devtools.build.lib.actions.cache.OutputMetadataStore; import com.google.devtools.build.lib.buildtool.buildevent.ExecutionPhaseCompleteEvent; import com.google.devtools.build.lib.events.EventHandler; +import com.google.devtools.build.lib.runtime.CommandEnvironment; +import com.google.devtools.build.lib.server.FailureDetails.Execution; +import com.google.devtools.build.lib.server.FailureDetails.Execution.Code; +import com.google.devtools.build.lib.server.FailureDetails.FailureDetail; import com.google.devtools.build.lib.util.AbruptExitException; +import com.google.devtools.build.lib.util.DetailedExitCode; import com.google.devtools.build.lib.vfs.BatchStat; import com.google.devtools.build.lib.vfs.FileSystem; import com.google.devtools.build.lib.vfs.ModifiedFileSet; import com.google.devtools.build.lib.vfs.OutputService; +import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; import com.google.devtools.build.lib.vfs.Root; import com.google.devtools.build.skyframe.SkyFunction.Environment; @@ -51,11 +57,17 @@ /** Output service implementation for the remote module */ public class RemoteOutputService implements OutputService { + private final CommandEnvironment env; + @Nullable private RemoteOutputChecker remoteOutputChecker; @Nullable private RemoteActionInputFetcher actionInputFetcher; @Nullable private LeaseService leaseService; @Nullable private Supplier fileCacheSupplier; + public RemoteOutputService(CommandEnvironment env) { + this.env = checkNotNull(env); + } + void setRemoteOutputChecker(RemoteOutputChecker remoteOutputChecker) { this.remoteOutputChecker = remoteOutputChecker; } @@ -118,6 +130,24 @@ public String getFilesSystemName() { @Override public ModifiedFileSet startBuild( EventHandler eventHandler, UUID buildId, boolean finalizeActions) throws AbruptExitException { + // One of the responsibilities of OutputService.startBuild() is that + // it ensures the output path is valid. If the previous + // OutputService redirected the output path to a remote location, we + // must undo this. + Path outputPath = env.getDirectories().getOutputPath(env.getWorkspaceName()); + if (outputPath.isSymbolicLink()) { + try { + outputPath.delete(); + } catch (IOException e) { + throw new AbruptExitException( + DetailedExitCode.of( + FailureDetail.newBuilder() + .setMessage(String.format("Couldn't remove output path symlink: %s", e.getMessage())) + .setExecution(Execution.newBuilder().setCode(Code.LOCAL_OUTPUT_DIRECTORY_SYMLINK_FAILURE)) + .build()), + e); + } + } return ModifiedFileSet.EVERYTHING_MODIFIED; } 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 ef50cfd8e96cc4..8682df93f8ea10 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 @@ -2049,4 +2049,35 @@ EOF expect_log "Hello World" } +function test_output_path_is_symlink() { + cat > BUILD << 'EOF' +genrule( + name = "foo", + outs = ["bar"], + cmd = "touch $@", +) +EOF + bazel build \ + --remote_executor=grpc://localhost:${worker_port} \ + --remote_download_minimal \ + //:foo >& $TEST_log || fail "Failed to build //:foo" + + # --remote_download_minimal and --remote_download_toplevel install an + # OutputService. One of the responsibilities of an OutputService is + # that it ensures a valid output path is present. + # + # Simulate the case where another OutputService replaced the output + # path with a symbolic link. If Bazel is rerun with + # --remote_download_minimal, it should remove the symbolic link, so + # that builds can take place on the local file system. + output_path=$(bazel info output_path) + rm -rf $output_path + ln -s /nonexistent $output_path + + bazel build \ + --remote_executor=grpc://localhost:${worker_port} \ + --remote_download_minimal \ + //:foo >& $TEST_log || fail "Failed to build //:foo" +} + run_suite "Build without the Bytes tests"