Try removing the RO bit when unlinking files on Windows #849
Conversation
On Windows, some programs may set the read-only bit on files. For instance, git marks its index files as read-only. Calling `Unix.unlink` on such a file will result in an `Unix.Unix_error(Unix.EACCES, "unlink", _)` exception. `Lwt_io.with_temp_dir` creates a temporary directory in which it is possible that a read-only file is created. In such case, the `delete_recursively` function will not be able to clean the temporary directory and will also raise an exception. This patch allows by setting the file writable (removing the RO bit) to delete the file. This code was copied from Dune: https://github.com/ocaml/dune/blob/ed361ebc4f37a81d3e6ffc905b0d45f61bc17e9c/otherlibs/stdune-unstable/fpath.ml#L74-L88
MisterDA
force-pushed
the
windows-chmod-readonly-delete-recursively
branch
from
817dd12
to
5fb5215
Compare
src/unix/lwt_io.ml
Outdated
| Lwt_unix.chmod fn 0o666 >>= fun () -> | ||
| Lwt_unix.unlink fn | ||
| with | ||
| | _ -> raise e) |
There was a problem hiding this comment.
Maybe it complicates things a bit but is it worth resetting the mod to the original setting in case the second unlink fails?
It does complicate the code to do so (read stats before chmod, catch exceptions of unlink specifically,restore perms before reraising), but at least it'd be more likely to leave the file in its original state.
In addition, I think you should be using Lwt.catch to handle exceptions here. (Although I might be missing something so please don't hesitate to let me know if there's a reason that try-with is appropriate here specifically.)
There was a problem hiding this comment.
I applied the two suggestions. The original permissions are fetched with lstat, if I'm not mistaken that's the correct syscall in our case.
I hadn't thought about using Lwt.catch, I converted the code to it.
There was a problem hiding this comment.
I noticed that you made a subtle change and I'm not sure if it's on purpose: You now reraise the exception from the second unlink whereas you previously reraised the original exception.
I don't think one is necessary better than the other (in the specific case of a system programming library for windows), but I'd rather the semantic was not chosen accidentally. Is the change intended or not?
|
Nice catch!
I was something I overlooked. I honestly don't thing it changes much, but
I'll reinstate the previous behaviour.
|
|
@raphael-proust Thanks for merging, but I didn't have the time to apply the change to re-raise the first exception! |
* Try removing the RO bit when unlinking files on Windows On Windows, some programs may set the read-only bit on files. For instance, git marks its index files as read-only. Calling `Unix.unlink` on such a file will result in an `Unix.Unix_error(Unix.EACCES, "unlink", _)` exception. `Lwt_io.with_temp_dir` creates a temporary directory in which it is possible that a read-only file is created. In such case, the `delete_recursively` function will not be able to clean the temporary directory and will also raise an exception. This patch allows by setting the file writable (removing the RO bit) to delete the file. This code was copied from Dune: https://github.com/ocaml/dune/blob/ed361ebc4f37a81d3e6ffc905b0d45f61bc17e9c/otherlibs/stdune-unstable/fpath.ml#L74-L88 * Use Lwt.catch instead of try-with and restore perms if unlink fails
On Windows, some programs may set the read-only bit on files. For
instance, git marks its index files as read-only. Calling
Unix.unlinkon such a file will result in anUnix.Unix_error(Unix.EACCES, "unlink", _)exception.Lwt_io.with_temp_dircreates a temporary directory in which it ispossible that a read-only file is created. In such case, the
delete_recursivelyfunction will not be able to clean the temporarydirectory and will also raise an exception.
This patch allows by setting the file writable (removing the RO bit)
to delete the file.
This code was copied from Dune:
https://github.com/ocaml/dune/blob/ed361ebc4f37a81d3e6ffc905b0d45f61bc17e9c/otherlibs/stdune-unstable/fpath.ml#L74-L88