Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Transform roots along with paths during output deletion. #12634

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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