Fix the asynchronous cleaning of readonly directories.
Previously, clean --async would silently fail to remove files in read-only
directories. Such directories arise from tree artifacts or unsandboxed actions
writing junk into the output tree. This change makes sure all directories can be
edited before executing the "rm -rf".
The limits of shelling out come into relief in this CL. A more solid approach
would be daemonizing a native utility that executes the code backing the VFS's
Path.deleteTreesBelow method. That would ensure synchronous and asynchronous
cleans have the same erasure capabilities and pave the way for cross-platform
--async support. This CL takes a small step in that direction by replacing
arcane shell daemonization with Bazel's daemonize tool.
Fixes https://github.com/bazelbuild/bazel/issues/12125.
Closes #12452.
PiperOrigin-RevId: 342869437
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/commands/CleanCommand.java b/src/main/java/com/google/devtools/build/lib/runtime/commands/CleanCommand.java
index 5c68499..78f8e77 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/commands/CleanCommand.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/commands/CleanCommand.java
@@ -35,12 +35,12 @@
import com.google.devtools.build.lib.server.FailureDetails.CleanCommand.Code;
import com.google.devtools.build.lib.server.FailureDetails.FailureDetail;
import com.google.devtools.build.lib.shell.CommandException;
+import com.google.devtools.build.lib.shell.CommandResult;
import com.google.devtools.build.lib.util.CommandBuilder;
import com.google.devtools.build.lib.util.InterruptedFailureDetails;
import com.google.devtools.build.lib.util.OS;
import com.google.devtools.build.lib.util.Pair;
import com.google.devtools.build.lib.util.ProcessUtils;
-import com.google.devtools.build.lib.util.ShellEscaper;
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.common.options.Option;
import com.google.devtools.common.options.OptionDocumentationCategory;
@@ -203,22 +203,27 @@
env.getReporter()
.handle(Event.info(null, pathItemName + " moved to " + tempPath + " for deletion"));
- // Daemonize the shell and use the double-fork idiom to ensure that the shell
- // exits even while the "rm -rf" command continues.
String command =
String.format(
- "exec >&- 2>&- <&- && (/usr/bin/setsid /bin/rm -rf %s &)&",
- ShellEscaper.escapeString(tempPath.getPathString()));
+ "/usr/bin/find %s -type d -not -perm -u=rwx -exec /bin/chmod -f u=rwx {} +; /bin/rm"
+ + " -rf %s",
+ tempBaseName, tempBaseName);
+ logger.atInfo().log("Executing daemonic shell command %s", command);
- logger.atInfo().log("Executing shell command %s", ShellEscaper.escapeString(command));
-
- // Doesn't throw iff command exited and was successful.
- new CommandBuilder()
- .addArg(command)
- .useShell(true)
- .setWorkingDir(tempPath.getParentDirectory())
- .build()
- .execute();
+ // Daemonize the shell to ensure that the shell exits even while the "rm
+ // -rf" command continues.
+ CommandResult result =
+ new CommandBuilder()
+ .addArg(
+ env.getBlazeWorkspace().getBinTools().getEmbeddedPath("daemonize").getPathString())
+ .addArgs("-l", "/dev/null")
+ .addArgs("-p", "/dev/null")
+ .addArg("--")
+ .addArgs("/bin/sh", "/bin/sh", "-c", command)
+ .setWorkingDir(tempPath.getParentDirectory())
+ .build()
+ .execute();
+ logger.atInfo().log("Shell command status: %s", result.getTerminationStatus());
}
private BlazeCommandResult actuallyClean(
diff --git a/src/test/py/bazel/bazel_clean_test.py b/src/test/py/bazel/bazel_clean_test.py
index f8a775c..dcfdc85 100644
--- a/src/test/py/bazel/bazel_clean_test.py
+++ b/src/test/py/bazel/bazel_clean_test.py
@@ -14,6 +14,7 @@
import os
import re
+import time
import unittest
from src.test.py.bazel import test_base
@@ -70,7 +71,7 @@
self.AssertExitCode(exit_code, 0, stderr)
matcher = self._findMatch(' moved to (.*) for deletion', stderr)
self.assertTrue(matcher, stderr)
- first_temp = matcher.group(0)
+ first_temp = matcher.group(1)
self.assertTrue(first_temp, stderr)
# Now do it again (we need to build to recreate exec root).
self.RunBazel(['build'])
@@ -78,11 +79,35 @@
self.AssertExitCode(exit_code, 0, stderr)
matcher = self._findMatch(' moved to (.*) for deletion', stderr)
self.assertTrue(matcher, stderr)
- second_temp = matcher.group(0)
+ second_temp = matcher.group(1)
self.assertTrue(second_temp, stderr)
# Two directories should be different.
self.assertNotEqual(second_temp, first_temp, stderr)
+ @unittest.skipIf(not test_base.TestBase.IsLinux(),
+ 'Async clean only supported on Linux')
+ def testBazelAsyncCleanWithReadonlyDirectories(self):
+ self.ScratchFile('WORKSPACE')
+ exit_code, _, stderr = self.RunBazel(['build'])
+ self.AssertExitCode(exit_code, 0, stderr)
+ exit_code, stdout, stderr = self.RunBazel(['info', 'execution_root'])
+ self.AssertExitCode(exit_code, 0, stderr)
+ execroot = stdout[0]
+ readonly_dir = os.path.join(execroot, 'readonly')
+ os.mkdir(readonly_dir)
+ open(os.path.join(readonly_dir, 'somefile'), 'wb').close()
+ os.chmod(readonly_dir, 0o555)
+ exit_code, _, stderr = self.RunBazel(['clean', '--async'])
+ matcher = self._findMatch(' moved to (.*) for deletion', stderr)
+ self.assertTrue(matcher, stderr)
+ temp = matcher.group(1)
+ for _ in range(50):
+ if not os.path.isdir(temp):
+ break
+ time.sleep(.1)
+ else:
+ self.fail('temporary directory not removed: {!r}'.format(stderr))
+
def _findMatch(self, pattern, items):
r = re.compile(pattern)
for line in items:
