Implement force deletion for RemoveAll

docs/BUILD

@@ -59,7 +59,7 @@ genrule(
 plugins = {
     "python": "v1.7.2",
     "java": "v0.4.1",
-    "go": "v1.21.1",
+    "go": "v1.21.2",
     "cc": "v0.4.0",
     "shell": "v0.2.0",
     "go-proto": "v0.3.0",

src/build/build_step.go

@@ -135,7 +135,7 @@ func prepareOnly(state *core.BuildState, target *core.BuildTarget) error {
 		return errStop
 	}
 
-	if err := prepareDirectories(state.ProcessExecutor, target); err != nil {
+	if err := prepareDirectories(target); err != nil {
 		return err
 	}
 	if err := prepareSources(state, state.Graph, target); err != nil {
@@ -277,7 +277,7 @@ func buildTarget(state *core.BuildState, target *core.BuildTarget, runRemotely b
 			return nil
 		}
 
-		if err := prepareDirectories(state.ProcessExecutor, target); err != nil {
+		if err := prepareDirectories(target); err != nil {
 			return fmt.Errorf("Error preparing directories for %s: %s", target.Label, err)
 		}
 
@@ -406,7 +406,7 @@ func buildTarget(state *core.BuildState, target *core.BuildTarget, runRemotely b
 	}
 	// Clean up the temporary directory once it's done.
 	if state.CleanWorkdirs {
-		if err := fs.ForceRemove(state.ProcessExecutor, target.TmpDir()); err != nil {
+		if err := fs.RemoveAll(target.TmpDir()); err != nil {
 			log.Warning("Failed to remove temporary directory for %s: %s", target.Label, err)
 		}
 	}
@@ -571,19 +571,19 @@ func prepareParentDirs(target *core.BuildTarget, out string) error {
 }
 
 // Prepares the temp and out directories for a target
-func prepareDirectories(executor *process.Executor, target *core.BuildTarget) error {
-	if err := prepareDirectory(executor, target.TmpDir(), true); err != nil {
+func prepareDirectories(target *core.BuildTarget) error {
+	if err := prepareDirectory(target.TmpDir(), true); err != nil {
 		return err
 	}
 	if err := prepareOutputDirectories(target); err != nil {
 		return err
 	}
-	return prepareDirectory(executor, target.OutDir(), false)
+	return prepareDirectory(target.OutDir(), false)
 }
 
-func prepareDirectory(executor *process.Executor, directory string, remove bool) error {
-	if remove && core.PathExists(directory) {
-		if err := fs.ForceRemove(executor, directory); err != nil {
+func prepareDirectory(directory string, remove bool) error {
+	if remove {
+		if err := fs.RemoveAll(directory); err != nil {
 			return err
 		}
 	}
@@ -743,7 +743,7 @@ func moveOutput(state *core.BuildState, target *core.BuildTarget, tmpOutput, rea
 			log.Debug("Checking %s vs. %s, hashes match", tmpOutput, realOutput)
 			return false, nil
 		}
-		if err := os.RemoveAll(realOutput); err != nil {
+		if err := fs.RemoveAll(realOutput); err != nil {
 			return true, err
 		}
 	}
@@ -773,7 +773,7 @@ func moveOutput(state *core.BuildState, target *core.BuildTarget, tmpOutput, rea
 func RemoveOutputs(target *core.BuildTarget) error {
 	for _, output := range target.Outputs() {
 		out := filepath.Join(target.OutDir(), output)
-		if err := os.RemoveAll(out); err != nil {
+		if err := fs.RemoveAll(out); err != nil {
 			return err
 		} else if err := fs.EnsureDir(out); err != nil {
 			return err
@@ -1048,9 +1048,9 @@ func fetchRemoteFile(state *core.BuildState, target *core.BuildTarget) error {
 		httpClientLimiter = make(chan struct{}, state.Config.Build.ParallelDownloads)
 	})
 
-	if err := prepareDirectory(state.ProcessExecutor, target.OutDir(), false); err != nil {
+	if err := prepareDirectory(target.OutDir(), false); err != nil {
 		return err
-	} else if err := prepareDirectory(state.ProcessExecutor, target.TmpDir(), false); err != nil {
+	} else if err := prepareDirectory(target.TmpDir(), false); err != nil {
 		return err
 	}
 	var err error

src/build/filegroup.go

@@ -83,7 +83,7 @@ func (builder *filegroupBuilder) Build(state *core.BuildState, target *core.Buil
 		return false, nil
 	}
 	// Must actually build the file.
-	if err := os.RemoveAll(to); err != nil {
+	if err := fs.RemoveAll(to); err != nil {
 		return true, err
 	} else if err := fs.EnsureDir(to); err != nil {
 		return true, err
@@ -103,7 +103,7 @@ func (builder *filegroupBuilder) Build(state *core.BuildState, target *core.Buil
 // We don't force this to be done in bash to avoid errors with maximum command lengths,
 // and it's actually quite fiddly to get just so there.
 func buildFilegroup(state *core.BuildState, target *core.BuildTarget) (bool, error) {
-	if err := prepareDirectory(state.ProcessExecutor, target.OutDir(), false); err != nil {
+	if err := prepareDirectory(target.OutDir(), false); err != nil {
 		return true, err
 	}
 	changed := false

src/build/incrementality.go

@@ -388,7 +388,7 @@ func loadTargetMetadata(target *core.BuildTarget) (*core.BuildMetadata, error) {
 // StoreTargetMetadata stores the target metadata into a file in the output directory of the target.
 func StoreTargetMetadata(target *core.BuildTarget, md *core.BuildMetadata) error {
 	filename := targetBuildMetadataFileName(target)
-	if err := os.RemoveAll(filename); err != nil {
+	if err := fs.RemoveAll(filename); err != nil {
 		return fmt.Errorf("failed to remove existing %s build metadata file: %w", target.Label, err)
 	} else if err := os.MkdirAll(filepath.Dir(filename), core.DirPermissions); err != nil {
 		return fmt.Errorf("Failed to create directory for build metadata file for %s: %w", target, err)

src/cache/BUILD

@@ -1,13 +1,6 @@
 go_library(
     name = "cache",
-    srcs = [
-        "async_cache.go",
-        "cache.go",
-        "cmd_cache.go",
-        "dir_cache.go",
-        "http_cache.go",
-        "noop.go",
-    ],
+    srcs = glob(["*.go"], exclude=["*_test.go"]),
     pgo_file = "//:pgo",
     visibility = ["PUBLIC"],
     deps = [
@@ -19,25 +12,13 @@ go_library(
         "//src/cli/logging",
         "//src/core",
         "//src/fs",
-        "//src/process",
     ],
 )
 
-filegroup(
-    name = "test_data",
-    srcs = ["test_data"],
-    test_only = True,
-)
-
 go_test(
     name = "cache_test",
-    srcs = [
-        "async_cache_test.go",
-        "cmd_cache_test.go",
-        "dir_cache_test.go",
-        "http_cache_test.go",
-    ],
-    data = [":test_data"],
+    srcs = glob(["*_test.go"]),
+    data = ["test_data"],
     deps = [
         ":cache",
         "///third_party/go/github.com_stretchr_testify//assert",

src/cache/dir_cache.go

@@ -36,7 +36,7 @@ func (cache *dirCache) Store(target *core.BuildTarget, key []byte, files []strin
 	cacheDir := cache.getPath(target, key, "")
 	tmpDir := cache.getFullPath(target, key, "", "=")
 	cache.markDir(cacheDir, 0)
-	if err := os.RemoveAll(cacheDir); err != nil {
+	if err := fs.RemoveAll(cacheDir); err != nil {
 		log.Warning("Failed to remove existing cache directory %s: %s", cacheDir, err)
 		return
 	}
@@ -64,7 +64,7 @@ func (cache *dirCache) storeCompressed(target *core.BuildTarget, filename string
 	log.Debug("Storing %s: %s in dir cache...", target.Label, filename)
 	if err := cache.storeCompressed2(target, filename, files); err != nil {
 		log.Warning("Failed to store files in cache: %s", err)
-		os.RemoveAll(filename) // Just a best-effort removal at this point
+		fs.RemoveAll(filename) // Just a best-effort removal at this point
 		return 0
 	}
 	// It's too hard to tell from a tar.Writer how big the resulting tarball is. Easier to just re-stat it here.
@@ -153,7 +153,7 @@ func (cache *dirCache) ensureStoreReady(filename string) error {
 	dir := filepath.Dir(filename)
 	if err := os.MkdirAll(dir, core.DirPermissions); err != nil {
 		return err
-	} else if err := os.RemoveAll(filename); err != nil {
+	} else if err := fs.RemoveAll(filename); err != nil {
 		return err
 	}
 	return nil
@@ -285,15 +285,15 @@ func (cache *dirCache) ensureRetrieveReady(target *core.BuildTarget, out string)
 	}
 	// It seems to be quite important that we unlink the existing file first to avoid ETXTBSY errors
 	// in cases where we're running an existing binary (as Please does during bootstrap, for example).
-	if err := os.RemoveAll(fullOut); err != nil {
+	if err := fs.RemoveAll(fullOut); err != nil {
 		return "", err
 	}
 	return fullOut, nil
 }
 
 func (cache *dirCache) Clean(target *core.BuildTarget) {
 	// Remove for all possible keys, so can't get getPath here
-	if err := os.RemoveAll(filepath.Join(cache.Dir, target.Label.PackageName, target.Label.Name)); err != nil {
+	if err := fs.RemoveAll(filepath.Join(cache.Dir, target.Label.PackageName, target.Label.Name)); err != nil {
 		log.Warning("Failed to remove artifacts for %s from dir cache: %s", target.Label, err)
 	}
 }
@@ -448,7 +448,7 @@ func (cache *dirCache) clean(highWaterMark, lowWaterMark uint64) uint64 {
 			log.Errorf("Couldn't rename %s: %s", entry.Path, err)
 			continue
 		}
-		if err := os.RemoveAll(newPath); err != nil {
+		if err := fs.RemoveAll(newPath); err != nil {
 			log.Errorf("Couldn't remove %s: %s", newPath, err)
 			continue
 		}

src/cache/http_cache.go

@@ -18,7 +18,6 @@ import (
 	"github.com/thought-machine/please/src/cli"
 	"github.com/thought-machine/please/src/core"
 	"github.com/thought-machine/please/src/fs"
-	"github.com/thought-machine/please/src/process"
 )
 
 type httpCache struct {
@@ -204,7 +203,7 @@ func openFile(header *tar.Header) (*os.File, error) {
 	if err != nil {
 		if os.IsPermission(err) {
 			// The file might already exist and be ro. If so, remove it.
-			if err := fs.ForceRemove(process.New(), header.Name); err != nil {
+			if err := fs.RemoveAll(header.Name); err != nil {
 				log.Debug("failed to remove existing file when restoring from the cache: %w", err)
 			}
 			return os.OpenFile(header.Name, os.O_WRONLY|os.O_TRUNC|os.O_CREATE, os.FileMode(header.Mode))

src/clean/clean.go

@@ -68,7 +68,7 @@ func cleanTarget(state *core.BuildState, target *core.BuildTarget) {
 func clean(path string) {
 	if core.PathExists(path) {
 		log.Info("Cleaning path %s", path)
-		if err := deleteDir(path, false); err != nil {
+		if err := fs.RemoveAll(path); err != nil {
 			log.Fatalf("Failed to clean path %s: %s", path, err)
 		}
 	}
@@ -81,10 +81,6 @@ func clean(path string) {
 // Conversely there is obviously no guarantee about at what point it will actually cease to
 // be on disk any more.
 func AsyncDeleteDir(dir string) error {
-	return deleteDir(dir, true)
-}
-
-func deleteDir(dir string, async bool) error {
 	rm, err := exec.LookPath("rm")
 	if err != nil {
 		return err
@@ -95,16 +91,9 @@ func deleteDir(dir string, async bool) error {
 	if err != nil {
 		return err
 	}
-	if async {
-		// Note that we can't fork() directly and continue running Go code, but ForkExec() works okay.
-		// Hence why we're using rm rather than fork() + os.RemoveAll.
-		_, err = syscall.ForkExec(rm, []string{rm, "-rf", newDir}, nil)
-		return err
-	}
-	out, err := exec.Command(rm, "-rf", newDir).CombinedOutput()
-	if err != nil {
-		log.Error("Failed to remove directory: %s", string(out))
-	}
+	// Note that we can't fork() directly and continue running Go code, but ForkExec() works okay.
+	// Hence why we're using rm rather than fork() + fs.RemoveAll.
+	_, err = syscall.ForkExec(rm, []string{rm, "-rf", newDir}, nil)
 	return err
 }
 

src/core/utils.go

@@ -374,7 +374,7 @@ func PrepareSourcePair(pair SourcePair) error {
 
 // PrepareRuntimeDir prepares a directory with a target's runtime data for a command to be run on.
 func PrepareRuntimeDir(state *BuildState, target *BuildTarget, dir string) error {
-	if err := fs.ForceRemove(state.ProcessExecutor, dir); err != nil {
+	if err := fs.RemoveAll(dir); err != nil {
 		return err
 	}
 

src/fs/BUILD

@@ -1,40 +1,19 @@
 go_library(
     name = "fs",
-    srcs = [
-        "attr.go",
-        "copy.go",
-        "executable.go",
-        "fs.go",
-        "glob.go",
-        "hash.go",
-        "home.go",
-        "iofs.go",
-        "sort.go",
-        "walk.go",
-    ],
+    srcs = glob(["*.go"], exclude=["*_test.go"]),
     pgo_file = "//:pgo",
     visibility = ["PUBLIC"],
     deps = [
         "///third_party/go/github.com_karrick_godirwalk//:godirwalk",
         "///third_party/go/github.com_peterebden_go-deferred-regex//:go-deferred-regex",
         "///third_party/go/github.com_pkg_xattr//:xattr",
         "//src/cli/logging",
-        "//src/process",
     ],
 )
 
 go_test(
     name = "fs_test",
-    srcs = [
-        "copy_test.go",
-        "fs_test.go",
-        "glob_integration_test.go",
-        "glob_test.go",
-        "hash_benchmark_test.go",
-        "hash_test.go",
-        "home_test.go",
-        "sort_test.go",
-    ],
+    srcs = glob(["*_test.go"], exclude=["glob_integration_test.go", "*_benchmark_test.go"]),
     data = ["test_data"],
     deps = [
         ":fs",

src/fs/fs.go

@@ -2,13 +2,14 @@
 package fs
 
 import (
+	"errors"
 	"fmt"
 	"io"
+	"io/fs"
 	"os"
 	"path/filepath"
 
 	"github.com/thought-machine/please/src/cli/logging"
-	"github.com/thought-machine/please/src/process"
 )
 
 var log = logging.Log
@@ -145,7 +146,7 @@ func renameFile(from, to string) (err error) {
 	if err != nil {
 		return err
 	}
-	err = os.RemoveAll(from)
+	err = RemoveAll(from)
 	if err != nil {
 		return err
 	}
@@ -186,18 +187,25 @@ func copyFile(from, to string) (err error) {
 	return nil
 }
 
-// ForceRemove will try and remove the path with `os.RemoveAll`, and if that fails, it will use `rm -rf` running the
-// command in any namespace that please is configured to.
-func ForceRemove(exec *process.Executor, path string) error {
-	// Try and remove it normally first
-	if err := os.RemoveAll(path); err == nil || os.IsNotExist(err) {
+// RemoveAll will try and remove the path with `os.RemoveAll`; if that fails with a permission error,
+// it will attempt to adjust permissions to make things writable, then remove them.
+func RemoveAll(path string) error {
+	if err := os.RemoveAll(path); err == nil || !errors.Is(err, os.ErrPermission) {
+		return err
+	} else if err := filepath.WalkDir(path, func(path string, d fs.DirEntry, err error) error {
+		const writable = 0o220
+		if err != nil {
+			return err
+		} else if d.IsDir() && d.Type()&writable != writable {
+			if info, err := d.Info(); err != nil {
+				return fmt.Errorf("could not read info for %s: %w", path, err)
+			} else if err := os.Chmod(path, info.Mode()|writable); err != nil {
+				return fmt.Errorf("could not make %s writable: %w", path, err)
+			}
+		}
 		return nil
+	}); err != nil {
+		return fmt.Errorf("failed to remove directory %s: %w", path, err)
 	}
-
-	cmd := exec.ExecCommand(process.NoSandbox, false, "rm", "-rf", path)
-
-	if out, err := cmd.CombinedOutput(); err != nil {
-		return fmt.Errorf("failed to remove %s: %w\nOutput: %s", path, err, string(out))
-	}
-	return nil
+	return os.RemoveAll(path)
 }