From 1f7aa364746a17f7e5ef02d2f3ee01d15a3bce74 Mon Sep 17 00:00:00 2001 From: Benjamin Peterson Date: Fri, 4 Dec 2020 18:50:49 -0600 Subject: [PATCH] Transform roots along with paths during output deletion. 4009b176cb480eb2f16b86f8d05a2e2beb62f61d resolved output paths but not the related roots. Fixes https://github.com/bazelbuild/bazel/issues/12678. --- .../build/lib/actions/AbstractAction.java | 26 ++++++++++++++----- .../lib/actions/ArtifactPathResolver.java | 4 +-- .../lib/bazel/BazelWorkspaceStatusModule.java | 2 +- .../includescanning/SpawnIncludeScanner.java | 12 +++++++-- .../bazel/remote/remote_execution_test.sh | 18 +++++++++++++ 5 files changed, 49 insertions(+), 13 deletions(-) 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 index 11c368d44d5973..63d4566e8ed550 100644 --- 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. + * + *

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,7 +388,7 @@ 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(); @@ -383,15 +396,14 @@ public static void deleteOutput(Path path, @Nullable ArtifactRoot root) throws I // 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 index d9f5e3d43dd8b7..982e1807eeaaa0 100644 --- 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 index 946eedeef1c7b1..4d1fca447d07d4 100644 --- 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 index 3918d701b244b1..8e03f89b2434a4 100644 --- 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 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> 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 index 440a23939da82c..6ed2b6231d3b0f 100755 --- 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'