Ensure grep-includes can write to its output file by deleting it upfront.
The include scanner deletes existing output files before running
grep-includes, but only did so for outputs that were generated when
scanning the includes of files from the source tree -- not for source
files generated during the build. Make this happen now in all cases.
The cleanup is necessary in all cases because, when grep-includes runs
remotely, we download its output into a file and set its permissions to
555. If we happen to run this same action locally at a later time, the
local run may not be able to overwrite the local file and we fail the
build.
This should fix random build failures during include scanning observed
when using the dynamic scheduler without local sandboxing in Google-internal
builds.
Addresses https://github.com/bazelbuild/bazel/issues/7818.
RELNOTES: None.
PiperOrigin-RevId: 244718195
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 c1b1eaa..71250f6 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
@@ -381,26 +381,30 @@
*/
protected void deleteOutputs(Path execRoot) throws IOException {
for (Artifact output : getOutputs()) {
- deleteOutput(output);
+ deleteOutput(output.getPath(), output.getRoot());
}
}
/**
- * Helper method to remove an Artifact. If the Artifact refers to a directory recursively removes
- * the contents of the directory.
+ * Helper method to remove an output file.
+ *
+ * <p>If the path refers to a directory, recursively removes the contents of the directory.
+ *
+ * @param path the output to remove
+ * @param root the root containing the output. This is used to sanity-check that we don't delete
+ * arbitrary files in the file system.
*/
- protected void deleteOutput(Artifact output) throws IOException {
- Path path = output.getPath();
+ public static void deleteOutput(Path path, @Nullable ArtifactRoot 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 (output.getRoot() == null) {
+ if (root == null) {
throw e;
}
- Root outputRoot = output.getRoot().getRoot();
+ Root outputRoot = root.getRoot();
if (!outputRoot.contains(path)) {
throw e;
}
@@ -409,7 +413,7 @@
if (!parentDir.isWritable() && outputRoot.contains(parentDir)) {
// Retry deleting after making the parent writable.
parentDir.setWritable(true);
- deleteOutput(output);
+ deleteOutput(path, root);
} else if (path.isDirectory(Symlinks.NOFOLLOW)) {
path.deleteTree();
} else {
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 f2e12af..a11655d 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
@@ -155,7 +155,7 @@
// 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(volatileStatus);
+ deleteOutput(volatileStatus.getPath(), volatileStatus.getRoot());
}
@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 e885735..8111a66 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
@@ -19,6 +19,7 @@
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Iterables;
+import com.google.devtools.build.lib.actions.AbstractAction;
import com.google.devtools.build.lib.actions.ActionAnalysisMetadata;
import com.google.devtools.build.lib.actions.ActionExecutionContext;
import com.google.devtools.build.lib.actions.ActionExecutionMetadata;
@@ -274,20 +275,10 @@
throws IOException, ExecException, InterruptedException {
Path output = getIncludesOutput(file, actionExecutionContext.getPathResolver(), fileType,
placeNextToFile);
- if (!inMemoryOutput && !placeNextToFile) {
- try {
- Path dir = output.getParentDirectory();
- if (dir.isDirectory()) {
- // If the output directory already exists, delete the old include file.
- output.delete();
- }
- dir.createDirectoryAndParents();
- } catch (IOException e) {
- throw new IOException(
- "Error creating output directory "
- + output.getParentDirectory()
- + ": "
- + e.getMessage());
+ if (!inMemoryOutput) {
+ AbstractAction.deleteOutput(output, placeNextToFile ? file.getRoot() : null);
+ if (!placeNextToFile) {
+ output.getParentDirectory().createDirectoryAndParents();
}
}
