Mitigate copying/deletion of unrecognised symlinks #5953
Conversation
|
With this, the issue at least reduces to: |
| @@ -129,19 +129,15 @@ let win32_unlink fn = | |||
| with _ -> raise e) | |||
|
|
|||
| let remove_file_t ?(with_log=true) file = | |||
| if | |||
| try ignore (Unix.lstat file); true with Unix.Unix_error _ -> false | |||
There was a problem hiding this comment.
remove_file_t on a non-existing file used to simply ignore it. This PR changes this behaviour to error out instead.
Was the old behaviour used anywhere that we know?
There was a problem hiding this comment.
I don't think so - its only other use is in the cross-device fallback for mv. The only way it could be called with a file which didn't exist is if the file has been removed between the test in the calling function and the test here, and there was already a race for that anyway (between the test I've deleted here and the internal_error handler below).
There was a problem hiding this comment.
it is also being used in remove_file and remove_dir which are used everywhere in opam's code (e.g. via OpamFilename.remove)
There was a problem hiding this comment.
remove_file doesn't use remove_file_t (LL202-215) and remove_dir already checks whether its argument exists?
There was a problem hiding this comment.
AH! I see. There are two toplevel versions of remove_file in the same file. Only the latter is exported. Sorry for missing that. This is all good then.
There was a problem hiding this comment.
Actually, until you’d said, I’d missed that too! I was only sure that the code was ok because I’d deleted the first remove_file as a result of an unused value warning and already searched for other uses of remove_file_t 🙂
|
maybe this also warrants an addition to the API section of the master changelog actually |
|
The API hasn’t changed though? |
no but its behaviour has (e.g. copy_dir can now display a warning on windows) |
There are two problems we can encounter with symlinks on Windows: - The user cannot create native symlinks (Developer Mode is not enabled) - At the point that the symlink is created, the file it refers to does not exist In this instance, Cygwin's tar falls back to its internal mechanisms, which can't be read by opam and cause issues when copying directories, etc. This commit mitigates the directory copy by displaying a warning and fixes the deletion of the directory by deleting the file anyway.
dra27
force-pushed
the
harden-tar-extraction
branch
from
ba69daf
to
645d2ec
Compare
|
Ah, yes, indeed. I added: |
|
Thanks! |
Partially addresses the issue reported in #5782 (comment)
In these situations, Cygwin is most likely to create a JUNCTION, which can't be read by
Unix.lstat. The fix for now I propose is simply to warn. This has the very big benefit of making ocamlbuild install without needing Developer Mode.It has the downside that we fail to copy the file... I'm suggesting we might choose to live with that for now.
Note that
OpamSystem.remove_file_tcontained an unnecessary second call tolstat. I needed to re-use the actual removal logic for the exception case inremove_dir_t, butlstatis very expensive in C on Unix, even more expensive in OCaml and indescribably more expensive on Windows, so eliminating the double-call seems better!