Fix compilation on MSYS2

[tleedjarv]

(all defines moved up because some caml/*.h includes pull in windows.h)
Fixes #1164

@jhjourdan it would be great if you could test in opam-repository before this gets merged.

[gdt]

What does 0603 mean? It's a magic number.

Is there a bug in msys2, or is i that msys2 uses the windows sdk, and that is documented to need to be told the oldest api, and if you don't, it provides a really old api, and this definition isn't part of that? I'm still having trouble understanding if this is unison doing what it should, or if it's a workaround.

Setting this is presumably declaring the min version, and thus the resulting binary may not (probably does not) run on older. I'm ok with that. But this says API, and there's ABI.

It could be that I'm being too difficult here, but I don't even understand today, and surely will not in a year or so. I want wisdom to be in checked-in comments, not in discusssions, so future self will see it.

It might be that @jhjourdan should steal the PR and write the comments I want, or the ones I would want if I understood, since I think @tleedjarv does not really want to hack on Windows :-)

[tleedjarv]

The short answer is: I don't know why MSYS2 requires it (it shouldn't) but defining it has no known negatives.

The long answer is: This is going to be a long-winded reply - I'm writing it down for the record here - but I believe none of this needs to go into comments. I don't have any specific Windows knowledge in this case, so I might be wrong, but as you correctly mentioned I don't want to invest any more time in this (as I believe there are no negative side-effects to worry about here).

Why magic number?
It is my understanding that this way is (somewhat) common (also present in OCaml sources, for that matter).
You need to use magic numbers for two reasons. One: sufficiently old headers won't have defined names for the newer constants. That said, it is probably irrelevant here because compilation would then break with or without those names... Two: A separate header file needs be included to get those defined names into scope, as WINVER needs to be defined before including any of the Windows API headers.

Why this number?
In this instance the magic number is selected by precisely what it says in the comment: to get the definition of DUPLICATE_EXTENTS_DATA into scope; there is no other intention behind it and no known side-effects either.

Why it is only MSYS2 that requires this approach, I don't know and I don't plan on investigating. This shouldn't be required (as is also demonstrated by MSVC and by MinGW under Cygwin).

Will it break ABI?
My understanding is that this is strictly only about API as Windows doesn't intentionally break ABI. The built binaries will be able to run on older Windows versions, unless you then link in some new API functions without proper guards. I've written Windows-specific code such that it will run fine under older Windows versions, it just needs to be compiled with newer headers present. I have also verified this by testing built binaries in Windows 7 (as the magic number here specifies Win 8.1).

In the end, I think none of this discussion belongs in the comments or even in the commit message because, in my (limited) understanding, this is a pretty standard thing in the Windows world and the mechanics of WINVER are irrelevant here. The real question here is why MSYS2 refuses to play nice and, to be honest, it doesn't matter really.

[jhjourdan]

I've written Windows-specific code such that it will run fine under older Windows versions, it just needs to be compiled with newer headers present. I have also verified this by testing built binaries in Windows 7 (as the magic number here specifies Win 8.1).

Interesting: how is it possible that FSCTL_DUPLICATE_EXTENTS_TO_FILE is recognized by DeviceIoControl on Windows 7? In my understanding, this API appeared on Windows 8.1.

[tleedjarv]

The trick here is that the functions exist in earlier API (although not existing wouldn't be a problem either - solved by dynamic lookup), so calling those with an unknown flag will just result in a benign error, which causes fallback code to be run. There is a guard check (via GetVolumeInformationByHandle) to never invoke unsupported operations, though.

[tleedjarv]

I don't know why MSYS2 requires it

Or maybe now I do. Just by a hunch I decided to check and there you have it: https://www.msys2.org/docs/windows_support/

Mingw Toolchains: MINGW32/MINGW64 environments allow targeting Windows 7+ still.

If this is true then it could mean that MSYS2 deliberately sets the default WINVER to a value suitable for Windows 7 (which does not yet have the definition of DUPLICATE_EXTENTS_DATA).

Edit: Looks like confirmed (from the same URL):

2022-12-26: Default _WIN32_WINNT bumped to Windows 8.1 for UCRT environments. MINGW32/MINGW64 environments still default to Windows 7.

So there you have it, mystery solved.

[gdt]

And yes, testing would be great.

[jhjourdan]

See ocaml/opam-repository#28851: all lights are greens, the tests passed.

[gdt]

It's great to see progress figuring this out. My goal are simply that 1) the comments that are checked in (PR discussion doesn't count for posterity) explain everything to the future and 2) if this is a bug in msys, we at least note that, and perhaps file. But it appears to be a choice, and maybe the bug is just that they don't document how to change it.

[tleedjarv]

The thing is, this WINVER / _WIN32_WINNT is a Microsoft thing and normally defaults to whatever SDK you're building with (I think if you have recent toolchain that means the Windows version you're building on). You actively set the value to target an older version. This is nothing to write home about.

I don't know how MinGW sets the default value, but it appears that MinGW in Cygwin and MinGW in MSYS2 use a different default. The issue is that MSYS2 limits the API for you and you then set the value to target a newer version. Which to me appears backwards and not what Microsoft intended. That's just what I think, I may be totally wrong, I've no experience with any of this.

I have now recorded all of this in the comment.

[gdt]

Thanks -i think future self will be happy with this!