Closed SGrondin closed 11 months ago
@talex5 thanks for the comments! I'll add rm_r
to Path
and make the changes.
@talex5 I realize now why I didn't think to put rm_r
/rmtree
into Path
: because it needs to be able to empty directories recursively, and it can't do that very well if symbolic links are present because the Path
module functions always follow the symbolic links. I can't use Path.unlink
on a symbolic link, I have to use the blocking Unix.unlink
in a thread.
But I fully agree that such a function needs to be part of Path
, I've already had to implement it 3 times in 3 different codebases. I have a few questions first in order to finish this PR:
Unix.unlink
in a new systhread (for now until #607 is fixed) when removing symbolic links? Or should just forego adding anything to Path
for now until #607 is fixed?
Eio_posix
rm_r
because rmtree
doesn't follow OCaml's snake case convention. rm_tree
sounds like an alias of the inexistent Tree.rm
. To my eyes, rm_r
looks more OCaml-y and there's no confusion about its meaning were I not familiar with Eio. Ruby uses rm_r
as well. That said, I think rmtree
is perfectly acceptable if you insist!I realize now why I didn't think to put rm_r/rmtree into Path: because it needs to be able to empty directories recursively, and it can't do that very well if symbolic links are present because the Path module functions always follow the symbolic links.
Right, this is why we need #599 first. That can be merged once https://github.com/ocaml/opam-repository/pull/24292 is accepted into opam-repository.
I can't use Path.unlink on a symbolic link, I have to use the blocking Unix.unlink in a thread.
Are you sure? Path.unlink
works for me.
Regarding the name, Lwt calls this delete_recursively
https://github.com/ocsigen/lwt/blob/2eee2a1b9e386cc0eacf455a5a9356150f920edb/src/unix/lwt_io.mli#L550
Another option would be Path.unlink ~recursive:true
.
I like delete_recursively
a lot 👍 Dangerous/powerful functions deserve longer, unambiguous names.
If I recall correctly, Path.unlink
is unable to remove a symbolic link if that link points to something outside of cwd
. It's why I had to resort to Unix.unlink
in this PR. It looked as if Path.unlink
was designed to seamlessly see through symbolic links, but now I understand it might just be a bug that occurs when the link points to something outside cwd
.
I just tested symlinks Path
operations every way I could think of, and I can confirm that it all works as expected. My earlier confusion came from the fact that I was statting with Path.with_open_in path Eio.File.stat
and that's absolutely going to follow the symlink!
I've just made several improvements based on the previous review. I'll complete development of this PR as soon as #599 is merged.
Stat support is now merged (as #620), so we can unpause this PR now!
Fantastic! I'll probably work on it this Saturday.
@talex5 Updated! See my comments on Element about kind
and rmtree
. If you do decide to make changes to those 2 functions I'll want to update this PR. If not, then consider this PR ready.
Once this is merged we need to start requesting changes on PRs adding tests without proper usage of the ~clear
argument. It's pretty easy to tell just by reading the test.
Thanks!
At the moment, developing with
dune runtest -w
is not the most pleasant experience. The first test run will succeed, but the subsequent runs of many tests (fs.md
,process.md
,README.md
) will fail with variousAlready_exists
andNot_found
errors.This PR cleans up those leftover artifacts to ensure accurate test outcomes.
I first tried to do something more automatic, where it would detect the previous artifacts, so that we won't have to remember to invoke the cleanup function with the right paths, but the result was more brittle than this. I wanted to make sure it would work even if the test gets interrupted, so the cleanup has to happen before the test run, not after it. Please let me know if you can think of a better approach.