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 bazelbuild#12823).

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;
       }

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;
   }
 

src/main/java/com/google/devtools/build/lib/vfs/OutputService.java

@@ -110,14 +110,19 @@ 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
    * @return a ModifiedFileSet of changed output files.
    * @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. */

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();
                   }
 

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");
 

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"