Sys.readdir on MingW Windows disagrees with Linux behavior

[Lucccyo]

While I wrote multicore tests of Sys module that run on Linux, macOS, and Windows CI,
I discovered that Sys.readdir doesn't behave the same on Linux or macOS as on MingW Windows.

Running the command on a Windows OCaml top-level using OCaml 5.0.0:

# Sys.readdir "non_existant_path";;
- : string array = [||]

We get back a string array empty instead of raising a Sys.error complaining of an unfound directory.

[dra27]

Thanks for the report! Just to add that this appears to be native-Windows-specific, rather than OCaml 5 only. It's the same for both the 4.11 mingw-w64 and msvc64 compilers I had lying around on my machine.

[shindere]

Many thanks for having reported this issue, @Lucccyo.

In my understanding, the behaviour you obseve comes from runtime/win32.C:

    return errno == ENOENT ? 0 : -1;

Basically, if the preceeding call to _wfindfirst returns -1 then, if errno == ENOENT we return 0 and otherwise -1. I don't understand what's the
reason why we return 0 rather than -1 when errno == ENOENT?

However, this has always been so since readdir got implemented by
@xavierleroy in 2003, see 859efb8 so I am surprised that, if
there really is a problem, nobody noticed so far.

[xavierleroy]

I don't understand what's the reason why we return 0 rather than -1 when errno == ENOENT?

_wfindfirst fails with ENOENT if the directory is empty. (Indeed, there is no first entry in this case.) That's why 0 is returned. Now, it could be that it also fails with ENOENT if the directory doesn't exist...

I'm sure this can be fixed in no more than 50 lines of Win32 incantations, but I'll let others give it a try.

[shindere]

Xavier Leroy (2022/12/21 03:21 -0800):

> I don't understand what's the reason why we return 0 rather than -1 when errno == ENOENT? `_wfindfirst` fails with ENOENT if the directory is empty. (Indeed, there is no first entry in this case.) That's why 0 is returned. Now, it could be that it also fails with ENOENT if the directory doesn't exist...

Ah. For those interested, the documentation is at `https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/findfirst-functions?view=msvc-170`.

I'm sure this can be fixed in no more than 50 lines of Win32 incantations, but I'll let others give it a try.

I think this is beyond my skills. Would one of you @dra27 or @nojb be able to work on that one? If not, one thing we could also do would be to add a comment in the code so that the next uninformed reader has a chance tu understand what's going on.

[Lucccyo]

Can we, in the case of ENOENT, just redo _wfindfirst() with the original path, without "*.*" appended at the end? If this second call fails with ENOENT, too, we know that the directory does not exist. Otherwise, the directory exists but is empty.

[shindere]

Given that you have tested, do you feel brave enough to submit a PR? I am wondering whether adding `*.*` even in the first call is (still) necessary?

[xavierleroy]

I am wondering whether adding *.* even in the first call is (still) necessary?

The replies to all your questions are in MSDN. You'll see that bad APIs never die.

[Lucccyo]

Given that you have tested, do you feel brave enough to submit a PR?

I can give it a try :)

[shindere]

Charlène_Gros (2022/12/22 01:31 -0800):

> Given that you have tested, do you feel brave enough to submit a PR? I can give it a try :)

Cool! Go ahead and I'll make sure to be participating in the review!

[Lucccyo]

We did a couple of tests with @tertium, and we're unsure what is the correct way of handling this, any comments are welcome.

The idea was to replace the line return errno == ENOENT ? 0 : -1; in caml_read_directory() (commit 4f23169, file runtime/win32.c, line 434) by the following code:

if (errno != ENOENT) return -1;
h = _wfindfirst(dirname, &fileinfo);
if (h == -1) return -1;
_findclose(h);
return 0;

That is, if the first call to _wfindfirst() on dirname/*.* fails with ENOENT, we call it again on dirname, and if this time it succeeds, we conclude that dirname exists and is empty. (Notice that if dirname is not a directory, the first _wfindfirst() fails with EINVAL, and so we return -1 right away.)

It turns out, however, that this test is useless. Indeed, there is no such thing as "empty directory", because . and .. are always there, and so the first _wfindfirst() won't fail. It is still possible to have an empty volume, because there are no . and .. at the root of a volume. However, in this case _wfindfirst() fails with ENOENT on the second call, too! In other words, if there are no files in, say, volume D:, then Windows itself considers that D: (or D:\) is not a valid directory name. Indeed, running dir D: produces an error message File not found.

At this stage, I'm inclined to suggest a much simpler fix: whenever _wfindfirst() fails, caml_read_directory() should always return -1, no matter the value of errno. Empty directories are not actually empty (as they contain . and ..), and empty volumes seem to be too much of a special case, and there seems to be little reason for caml_read_directory() to try to be more intelligent than dir itself.

[xavierleroy]

Indeed, there is no such thing as "empty directory", because . and .. are always there, and so the first _wfindfirst() won't fail.

Oh! Excellent point! If that's indeed the case, your simpler fix is perfect, let's implement this ASAP.

[dra27]

I'm afraid there are empty directories - root directories are capable of being empty. It's irritatingly inconsitent: despite the fact that . and .. are not returned for C:\, C:\. and C:\.. are valid directories with the expected meaning. It's much less common than it used to be, but a problem if, say, something like Unison checks the destination path is empty before copying files and that happens to be a USB stick 🤷 The fix is still nice and simple, though... I'll comment in the PR.