Transform roots along with paths during output deletion.

benjaminp · copybara-github · commit 082d58de852e · 2020-12-11T04:46:16.000-08:00
4009b17 resolved output paths but not the related roots. Fixes bazelbuild#12678. Closes bazelbuild#12634. PiperOrigin-RevId: 346975821
diff --git a/src/main/java/com/google/devtools/build/lib/actions/AbstractAction.java b/src/main/java/com/google/devtools/build/lib/actions/AbstractAction.java
@@ -362,10 +362,23 @@ protected void deleteOutputs(
     }
 
     for (Artifact output : getOutputs()) {
-      deleteOutput(pathResolver.toPath(output), output.getRoot());
+      deleteOutput(output, pathResolver);
     }
   }
 
+  /**
+   * Remove an output artifact.
+   *
+   * <p>If the path refers to a directory, recursively removes the contents of the directory.
+   *
+   * @param output artifact to remove
+   */
+  protected static void deleteOutput(Artifact output, ArtifactPathResolver pathResolver)
+      throws IOException {
+    deleteOutput(
+        pathResolver.toPath(output), pathResolver.transformRoot(output.getRoot().getRoot()));
+  }
+
   /**
    * Helper method to remove an output file.
    *
@@ -375,23 +388,22 @@ protected void deleteOutputs(
    * @param root the root containing the output. This is used to check that we don't delete
    *     arbitrary files in the file system.
    */
-  public static void deleteOutput(Path path, @Nullable ArtifactRoot root) throws IOException {
+  public static void deleteOutput(Path path, @Nullable Root root) throws IOException {
     try {
       // Optimize for the common case: output artifacts are files.
       path.delete();
     } catch (IOException e) {
       // Handle a couple of scenarios where the output can still be deleted, but make sure we're not
       // deleting random files on the filesystem.
       if (root == null) {
-        throw new IOException(e);
+        throw new IOException("null root", e);
       }
-      Root outputRoot = root.getRoot();
-      if (!outputRoot.contains(path)) {
-        throw new IOException(e);
+      if (!root.contains(path)) {
+        throw new IOException(String.format("%s not under %s", path, root), e);
       }
 
       Path parentDir = path.getParentDirectory();
-      if (!parentDir.isWritable() && outputRoot.contains(parentDir)) {
+      if (!parentDir.isWritable() && root.contains(parentDir)) {
         // Retry deleting after making the parent writable.
         parentDir.setWritable(true);
         deleteOutput(path, root);
diff --git a/src/main/java/com/google/devtools/build/lib/actions/ArtifactPathResolver.java b/src/main/java/com/google/devtools/build/lib/actions/ArtifactPathResolver.java
@@ -37,9 +37,7 @@ public interface ArtifactPathResolver {
    */
   Path convertPath(Path path);
 
-  /**
-   * @return a resolved Rooth corresponding to the given Root.
-   */
+  /** @return a resolved {@link Root} corresponding to the given Root. */
   Root transformRoot(Root root);
 
   ArtifactPathResolver IDENTITY = new IdentityResolver(null);
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/BazelWorkspaceStatusModule.java b/src/main/java/com/google/devtools/build/lib/bazel/BazelWorkspaceStatusModule.java
@@ -161,7 +161,7 @@ public void prepare(
       // The default implementation of this method deletes all output files; override it to keep
       // the old stableStatus around. This way we can reuse the existing file (preserving its mtime)
       // if the contents haven't changed.
-      deleteOutput(pathResolver.toPath(volatileStatus), volatileStatus.getRoot());
+      deleteOutput(volatileStatus, pathResolver);
     }
 
     @Override
diff --git a/src/main/java/com/google/devtools/build/lib/includescanning/SpawnIncludeScanner.java b/src/main/java/com/google/devtools/build/lib/includescanning/SpawnIncludeScanner.java
@@ -295,7 +295,11 @@ public Collection<Inclusion> extractInclusions(
     Path output = getIncludesOutput(file, actionExecutionContext.getPathResolver(), fileType,
         placeNextToFile);
     if (!inMemoryOutput) {
-      AbstractAction.deleteOutput(output, placeNextToFile ? file.getRoot() : null);
+      AbstractAction.deleteOutput(
+          output,
+          placeNextToFile
+              ? actionExecutionContext.getPathResolver().transformRoot(file.getRoot().getRoot())
+              : null);
       if (!placeNextToFile) {
         output.getParentDirectory().createDirectoryAndParents();
       }
@@ -409,7 +413,11 @@ public ListenableFuture<Collection<Inclusion>> extractInclusionsAsync(
         getIncludesOutput(
             file, actionExecutionContext.getPathResolver(), fileType, placeNextToFile);
     if (!inMemoryOutput) {
-      AbstractAction.deleteOutput(output, placeNextToFile ? file.getRoot() : null);
+      AbstractAction.deleteOutput(
+          output,
+          placeNextToFile
+              ? actionExecutionContext.getPathResolver().transformRoot(file.getRoot().getRoot())
+              : null);
       if (!placeNextToFile) {
         output.getParentDirectory().createDirectoryAndParents();
       }
diff --git a/src/test/shell/bazel/remote/remote_execution_test.sh b/src/test/shell/bazel/remote/remote_execution_test.sh
@@ -1667,6 +1667,24 @@ function test_download_toplevel_no_remote_execution() {
       || fail "Failed to run bazel build --remote_download_toplevel"
 }
 
+function test_download_toplevel_can_delete_directory_outputs() {
+  cat > BUILD <<'EOF'
+genrule(
+    name = 'g',
+    outs = ['out'],
+    cmd = "touch $@",
+)
+EOF
+  bazel build
+  mkdir $(bazel info bazel-genfiles)/out
+  touch $(bazel info bazel-genfiles)/out/f
+  bazel build \
+        --remote_download_toplevel \
+        --remote_executor=grpc://localhost:${worker_port} \
+        //:g \
+        || fail "should have worked"
+}
+
 function test_tag_no_remote_cache() {
   mkdir -p a
   cat > a/BUILD <<'EOF'

Original file line number	Diff line number	Diff line change
`@@ -161,7 +161,7 @@ public void prepare(`
`161`	`161`	`// The default implementation of this method deletes all output files; override it to keep`
`162`	`162`	`// the old stableStatus around. This way we can reuse the existing file (preserving its mtime)`
`163`	`163`	`// if the contents haven't changed.`
`164`		`- deleteOutput(pathResolver.toPath(volatileStatus), volatileStatus.getRoot());`
	`164`	`+ deleteOutput(volatileStatus, pathResolver);`
`165`	`165`	`}`
`166`	`166`
`167`	`167`	`@Override`