MinGW/MSVC Sys.rename cornercase regression

[jmid]

Last year, #12184 and #12320 fixed two MinGW Sys.rename cornercases.
I found a third one which sadly flew under the radar:

# Sys.mkdir "sandbox\\eee" 0o755;;
- : unit = ()
# Sys.rename "sandbox" "sandbox\\eee";;
Exception: Sys_error "Invalid argument".
# Sys.file_exists "sandbox\\eee";;
- : bool = false

This happens only when the target is an empty child directory of the source, in which case

• it fails as expected
• but has the side effect of removing the target.

This is a consequence of the try-then-retry strategy of #12320.
Using PathIsPrefix to avoid retrying on a less-common error path does not seem too invasive IMHO.

[MisterDA]

You should squash the last two commits (for bisecting it's better to have changes made to configure.ac reflected in configure in the same commit). You need to regenerate configure with Autoconf 2.71. If you have a later version installed, you may use this Docker invocation (courtesy of David):

docker run --rm -it -v $PWD:/home/opam/ocaml ocaml/opam:ubuntu-22.04-opam sh -c 'sudo apt install -y autoconf ; make -C ocaml configure'

[jmid]

I've rerun tools/autogen with the proper autoconf version and squashed the commits as requested.
I also fixed an 80-column overflow and added a Changes entry, which required another rebasing.

[MisterDA]

1. Do you know if PathIsPrefix does something smarter than a strcmp? or does it actually check the filesystem? The documentation doesn't tell much.
2. The problem is not specific to mingw (by opposition to msvc), but rather to Windows, right?

[jmid]

1. Do you know if PathIsPrefix does something smarter than a strcmp? or does it actually check the filesystem? The documentation doesn't tell much.

No, sorry, I don't know more than what's available in the above link.
I went with a Windows API call to keep things as simple as possible.

2. The problem is not specific to mingw (by opposition to msvc), but rather to Windows, right?

Yep, this should also affect the restored MSVC port coming in 5.3.
Technically it is a regression against 5.1 and 5.2 with MinGW support.

[gasche]

I'm prepared to approve on behalf of @MisterDA if/when that day comes.

[MisterDA]

I also know that @shindere dislikes when we add failing tests in a commit, and then the fix for the commit: when we bisect and run the testsuite, the new failing test can be annoying to ignore for the person who's looking for another error. I either suggest you squash the patches, or the core-dev merging this PR squashes them.

I'm going to try this PR and convince myself, and report back.

[shindere]

@MisterDA speaks wisely. :) For sake of completeness, the recommended way of re-generating `configure` is through `tools/autogen` which can take, as argument, the ` autoconf` executable to invoke. So if the package manager allows to install, say, `autoconf`, `autoconf-2.71` etc. binaries side by side in the PATH, then one can invoke `tools/autogen autoconf-2.71`. In case this is not yet documented at least in `HACKING.adoc`, would someboy have the time and energy to open a PR? Would be happy to review.

[shindere]

True again, many thanks!

[gasche]

when we bisect and run the testsuite, the new failing test can be annoying to ignore for the person who's looking for another error

My solution to this problem is to commit the erroneous reference output in the first version that exhibits the bug, with a comment in the test that explains that we observe/expect/record an incorrect output. Then the testsuite passes both before the fix and after the fix, and/but it is clear what the status is, and we can see the change in testsuite behavior in the commit that introduces the fix.

[shindere]

Antonin Décimo (2024/05/15 00:33 -0700):

1. Do you know if [`PathIsPrefix`](https://learn.microsoft.com/en-us/windows/win32/api/shlwapi/nf-shlwapi-pathisprefixa) does something smarter than a `strcmp`? Some conversions, maybe? The documentation doesn't tell much.

My guess is that `PathIsPrefix` is more than a string prefix, that at least some string normalisation is inovlved to remove/resolve, say, `./`, `../`, normalise the quotations...

2. The problem is not specific to mingw (by opposition to msvc), but rather to Windows, right?

Given that the response is yes, I wanted to edit the title of the PR to reflect this accurately but that does not seem to work at the moment, I don't know why but if somebody with more power than me could do it, it would be awesome. (Thanks in advance.) Re:the history of the changes, I do understand the logic that leads to add the test first, to illustrate the failure, and to then fix it, but I indeed think it's better to not split that in two consecutive commits on trunk. May I also nitpick on `testsuite/tests/lib-sys/rename.reference` and ask for it to terminate with a `\n` so that `git show` does not print its `\ No newline at end of file` warning? I am also curious about the `configure.ac` change, is it because we now use `PathIsPrefix` that we need to add all these libraries we didn't need before!? @dra27 may be interested in this! I'd suggest to squash all the commits into just one and likely no need to mention the callto `tools/autogen` in the commit message.

[jmid]

I've updated the PR title.

I'm fine with squashing the commits at the end.

My editor initially added a final newline to testsuite/tests/lib-sys/rename.reference which caused the reference diff test to fail,
hence I removed it again to make the test pass.

Finally, the PR is only adding one library: shlwapi to configure.ac.
I however needed to add a newline to fit within the 80 column limit, so it may appear as a bigger change.

[shindere]

Jan Midtgaard (2024/05/15 12:51 -0700):

I've updated the PR title.

Thanks!

I'm fine with squashing the commits at the end.

Can you please do it so that you can also make the commit message nice?

My editor initially added a final newline to `testsuite/tests/lib-sys/rename.reference` which caused the reference diff test to fail, hence I removed it again to make the test pass.

Maybe better to make the test program add a \n rather than fix things the other way around, so that, if you need to add other tests in the future, you will only be adding lines, rather than having tomodify existing ones.

Finally, the PR is only adding one library: `shlwapi` to `configure.ac`. I however needed to add a newline to fit within the 80 column limit, so it may appear as a bigger change.

I may have read too quickly, sorry about that!

[MisterDA]

I looked at this and could reproduce the issue and the fix.
I'm however pretty sure that we never fixed any of the reported problems in Unix.rename! It's also never mentioned in the previous PRs. Unfortunately Unix.rename wasn't calling internaly rename_os but MoveFileEx still (unlike Unix.mkdir, for instance, which delegates its work to mkdir_os). I suggest we apply the following patch:

diff --git a/otherlibs/unix/rename_win32.c b/otherlibs/unix/rename_win32.c
index 453ecc1151..ff6421421f 100644
--- a/otherlibs/unix/rename_win32.c
+++ b/otherlibs/unix/rename_win32.c
@@ -30,9 +30,7 @@ CAMLprim value caml_unix_rename(value path1, value path2)
   caml_unix_check_path(path2, "rename");
   wpath1 = caml_stat_strdup_to_utf16(String_val(path1));
   wpath2 = caml_stat_strdup_to_utf16(String_val(path2));
-  ok = MoveFileEx(wpath1, wpath2,
-                  MOVEFILE_REPLACE_EXISTING | MOVEFILE_WRITE_THROUGH |
-                  MOVEFILE_COPY_ALLOWED);
+  ok = rename_os(wpath1, wpath2);
   caml_stat_free(wpath1);
   caml_stat_free(wpath2);
   if (! ok) {   

If we're considering backporting the previous patches, I think this one could join the basket. I'll audit win32 code from libunix to see whether more functions can delegate their implementation to C code from the runtime.

[MisterDA]

It's also interesting to fix libunix, considering that libraries such as Lwt often wrap libunix functions.

[jmid]

I completely agree about fixing the Unix library too to ensure better portability!
However, I think this is drifting from the PR's original purpose and better handled in a separate one.

(In fact I have a draft model test of some of the corresponding Unix functions that I intend to get back to, which did reveal similar rename differences 🙂 )

[jmid]

I've squashed the commits as requested.

I can see the CI failing on MSVC due to #13174 - this failure is however unrelated (the separate commits were passing yesterday before the squash).

[jmid]

I added @dra27 as reviewer as he provided off-line feedback.
This was also an opportunity to kick off another CI run now that #13174 has been merged.
I expect the MSVC workflow to pass again.

[shindere]

Thanks for the work! I have one final wish and one final question: Wish: Squash the two commits into one. Question: in the test, is there a reason to first do a print_string "Rename parent directory to empty child directory: "; then some work, and then the `print_newline`, rather that just using `priint_endline`?

[jmid]

Wish: Squash the two commits into one.

Sure

Question: in the test, is there a reason to first do a print_string "Rename parent directory to empty child directory: "; then some work, and then the print_newline, rather that just using priint_endline?

That won't work, as the expected output is

Rename parent directory to empty child directory: fails as expected

Note the fails as expected part. We need the newline afterwards.
The PR is just doing what the previous testsuite/tests/lib-sys/rename.ml tests are already doing

[shindere]

Jan Midtgaard (2024/05/17 07:37 -0700):

> Wish: Squash the two commits into one. Sure

thanks!

> Question: in the test, is there a reason to first do a print_string "Rename parent directory to empty child directory: "; then some work, and then the `print_newline`, rather that just using `priint_endline`? That won't work, as the expected output is ``` Rename parent directory to empty child directory: fails as expected ``` Note the `fails as expected` part. We need the newline afterwards. The PR is just doing what the previous testsuite/tests/lib-sys/rename.ml tests are already doing

It makes sense! Thanks for having explained!