Skip to content

Commit

Permalink
Transform roots along with paths during output deletion.
Browse files Browse the repository at this point in the history
4009b17 resolved output paths but not the related roots.

Fixes #12678.

Closes #12634.

PiperOrigin-RevId: 346975821
  • Loading branch information
benjaminp authored and copybara-github committed Dec 11, 2020
1 parent ddfc932 commit 082d58d
Show file tree
Hide file tree
Showing 5 changed files with 49 additions and 13 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*
Expand All @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
Expand Down Expand Up @@ -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();
}
Expand Down
18 changes: 18 additions & 0 deletions src/test/shell/bazel/remote/remote_execution_test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand Down

0 comments on commit 082d58d

Please sign in to comment.