Implement force deletion for RemoveAll#3236
Implement force deletion for RemoveAll#3236peterebden merged 6 commits intothought-machine:masterfrom
Conversation
peterebden
force-pushed
the
force-delete
branch
from
5ef6217 to
e360f97
Compare
|
Thank you! |
| 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. | ||
| // Hence why we're using rm rather than fork() + fs.RemoveAll. |
There was a problem hiding this comment.
I've noticed in a few of my worktrees (using noremote) that some of these non-writable files get left in plz-out (e.g. /home/slittley/core3/pukufmt/src/plz-out/tmp/third_party/go/_github.com_go-webauthn_x#dl._build/go_mod_download_folder/pkg/mod/golang.org/x/telemetry/config@v0.29.0/doc.go), which I think means that plz clean won't work correctly either.
We might want to change the non-async mode below to use fs.RemoveAll?
There was a problem hiding this comment.
Yup fair, it is a bit cleaner this way I think.
| // 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.PathError).Err, os.ErrPermission) { |
There was a problem hiding this comment.
We've lost the os.IsNotExist check here (which should be errors.Is(err, fs.ErrNotExist)); is that ok?
There was a problem hiding this comment.
Yep, from the docs:
If the path does not exist, RemoveAll returns nil (no error)
I'm not sure why we were checking that before.
| // 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.PathError).Err, os.ErrPermission) { |
There was a problem hiding this comment.
err.(*os.PathError).Err looks like a potential panic to me. According to the godoc os.RemoveAll does always return a *PathError, but this conversion should be entirely unnecessary - *PathError implements Unwrap returning .Err, so errors.Is(err, os.ErrPermission) should work fine
There was a problem hiding this comment.
I tried that, it doesn't work.
There was a problem hiding this comment.
Ok no fair enough, it does seem to be now. Not sure why I thought it wasn't originally - but agreed it is a heap nicer without the cast.
| return fmt.Errorf("failed to remove %s: %w\nOutput: %s", path, err, string(out)) | ||
| } | ||
| return nil | ||
| return os.RemoveAll(path) |
There was a problem hiding this comment.
This should also have a errors.Is(err, fs.ErrNotExist) check
There was a problem hiding this comment.
as above, RemoveAll doesn't error if the path doesn't exist
3 participants
This replaces
fs.ForceRemovewithfs.RemoveAlland changes every call toos.RemoveAllto use it.It attempts a standard delete, then if that fails attempts to chmod any non-writable directories it can find, then delete again.
This is IMO a bit cleaner than the previous implementation which was shelling out to
rm, requiring a process executor (It's also less Unix-specific, which could be relevant in a far future if we ever tried to get Windows working). I wonder if that was one reason that only a small handful of cases calledForceRemove; anyway, it's not needed now.Will fix / work around #3235 although I still fundamentally think Go should be doing something sensible there. I've bootstrapped a copy of Please on this machine (at least up to the Python stuff that I haven't finished fixing yet) with 1.23.0 and it seems fine.