From 4574a4bcc69a4e0afad160e5cda5ea52255526d3 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). --- .../build/lib/buildtool/ExecutionTool.java | 1 + .../build/lib/remote/RemoteOutputService.java | 28 ++++++++++++++++- .../devtools/build/lib/vfs/OutputService.java | 7 ++++- ...meldOutputServiceBuildIntegrationTest.java | 4 ++- .../OutputsInvalidationIntegrationTest.java | 13 ++++---- .../remote/build_without_the_bytes_test.sh | 31 +++++++++++++++++++ 6 files changed, 75 insertions(+), 9 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/buildtool/ExecutionTool.java b/src/main/java/com/google/devtools/build/lib/buildtool/ExecutionTool.java index 91c74921e94ad2..0b75c4c48c2d12 100644 --- a/src/main/java/com/google/devtools/build/lib/buildtool/ExecutionTool.java +++ b/src/main/java/com/google/devtools/build/lib/buildtool/ExecutionTool.java @@ -500,6 +500,7 @@ private ModifiedFileSet startBuildAndDetermineModifiedOutputFiles( try (SilentCloseable c = Profiler.instance().profile("outputService.startBuild")) { modifiedOutputFiles = outputService.startBuild( + env.getDirectories().getOutputPath(env.getWorkspaceName()), env.getReporter(), buildId, request.getBuildOptions().finalizeActions); informedOutputServiceToStartTheBuild = true; } 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..e5410a01c3823b 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,16 @@ 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.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; @@ -117,7 +122,28 @@ public String getFilesSystemName() { @Override public ModifiedFileSet startBuild( - EventHandler eventHandler, UUID buildId, boolean finalizeActions) throws AbruptExitException { + Path outputPath, + 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. + 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/main/java/com/google/devtools/build/lib/vfs/OutputService.java b/src/main/java/com/google/devtools/build/lib/vfs/OutputService.java index d15a70c12c3aeb..a54feb33800cd3 100644 --- a/src/main/java/com/google/devtools/build/lib/vfs/OutputService.java +++ b/src/main/java/com/google/devtools/build/lib/vfs/OutputService.java @@ -110,6 +110,7 @@ default RemoteArtifactChecker getRemoteArtifactChecker() { /** * Start the build. * + * @param outputPath absolute path pointing to the output directory * @param buildId the UUID build identifier * @param finalizeActions whether this build is finalizing actions so that the output service can * track output tree modifications @@ -117,7 +118,11 @@ default RemoteArtifactChecker getRemoteArtifactChecker() { * @throws BuildFailedException if build preparation failed * @throws InterruptedException */ - ModifiedFileSet startBuild(EventHandler eventHandler, UUID buildId, boolean finalizeActions) + ModifiedFileSet startBuild( + Path outputPath, + EventHandler eventHandler, + UUID buildId, + boolean finalizeActions) throws BuildFailedException, AbruptExitException, InterruptedException; /** Flush and wait for in-progress downloads. */ diff --git a/src/test/java/com/google/devtools/build/lib/buildtool/SkymeldOutputServiceBuildIntegrationTest.java b/src/test/java/com/google/devtools/build/lib/buildtool/SkymeldOutputServiceBuildIntegrationTest.java index 13df861612b8ae..14d1b1b40fc4e5 100644 --- a/src/test/java/com/google/devtools/build/lib/buildtool/SkymeldOutputServiceBuildIntegrationTest.java +++ b/src/test/java/com/google/devtools/build/lib/buildtool/SkymeldOutputServiceBuildIntegrationTest.java @@ -24,6 +24,7 @@ import com.google.devtools.build.lib.vfs.BatchStat; 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 java.util.Map; import java.util.UUID; @@ -52,7 +53,8 @@ public String getFilesSystemName() { @Override public ModifiedFileSet startBuild( - EventHandler eventHandler, UUID buildId, boolean finalizeActions) { + Path outputPath, EventHandler eventHandler, + UUID buildId, boolean finalizeActions) { throw new IllegalStateException(); } diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/OutputsInvalidationIntegrationTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/OutputsInvalidationIntegrationTest.java index bb4441ff70b97a..bf058786f0df38 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/OutputsInvalidationIntegrationTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/OutputsInvalidationIntegrationTest.java @@ -55,7 +55,7 @@ public void prepareOutputServiceMock() throws BuildFailedException, AbruptExitException, InterruptedException, IOException { when(outputService.actionFileSystemType()).thenReturn(ActionFileSystemType.DISABLED); when(outputService.getFilesSystemName()).thenReturn("fileSystemName"); - when(outputService.startBuild(any(), any(), anyBoolean())) + when(outputService.startBuild(any(), any(), any(), anyBoolean())) .thenReturn(ModifiedFileSet.EVERYTHING_MODIFIED); } @@ -86,7 +86,7 @@ public void nothingModified_doesntInvalidateAnyActions(@TestParameter boolean de delete(getOnlyOutput("//foo")); } - when(outputService.startBuild(any(), any(), anyBoolean())) + when(outputService.startBuild(any(), any(), any(), anyBoolean())) .thenReturn(ModifiedFileSet.NOTHING_MODIFIED); events.collector().clear(); buildTarget("//foo"); @@ -124,7 +124,7 @@ public void identicalOutputs_doesntInvalidateAnyActions( buildTarget("//foo"); MoreAsserts.assertContainsEvent(events.collector(), "Executing genrule //foo:foo"); - when(outputService.startBuild(any(), any(), anyBoolean())) + when(outputService.startBuild(any(), any(), any(), anyBoolean())) .thenReturn(modification.modifiedFileSet(getOnlyOutput("//foo"))); events.collector().clear(); buildTarget("//foo"); @@ -140,7 +140,7 @@ public void noCheckOutputFiles_ignoresModifiedFiles( buildTarget("//foo"); MoreAsserts.assertContainsEvent(events.collector(), "Executing genrule //foo:foo"); - when(outputService.startBuild(any(), any(), anyBoolean())) + when(outputService.startBuild(any(), any(), any(), anyBoolean())) .thenReturn(modification.modifiedFileSet(getOnlyOutput("//foo"))); events.collector().clear(); buildTarget("//foo"); @@ -162,7 +162,7 @@ public void everythingModified_invalidatesAllActions( MoreAsserts.assertContainsEvent(events.collector(), "Executing genrule //foo:foo"); delete(getOnlyOutput("//foo")); - when(outputService.startBuild(any(), any(), anyBoolean())) + when(outputService.startBuild(any(), any(), any(), anyBoolean())) .thenReturn( everythingDeleted ? ModifiedFileSet.EVERYTHING_DELETED @@ -185,7 +185,8 @@ public void outputFileModified_invalidatesOnlyAffectedAction() throws Exception Artifact fooOut = getOnlyOutput("//foo"); delete(fooOut); - when(outputService.startBuild(any(), any(), anyBoolean())).thenReturn(modifiedFileSet(fooOut)); + when(outputService.startBuild(any(), any(), any(), anyBoolean())) + .thenReturn(modifiedFileSet(fooOut)); events.collector().clear(); buildTarget("//foo:all"); 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"