Fix <Shlwapi.h>, <Windows.h> capitalization for case sensitive cross compile.

[evetion]

Like #2518, in cross compiling thrift in JuliaPackaging/Yggdrasil#5427, I encountered:

[12:29:29] /workspace/srcdir/thrift/lib/cpp/src/thrift/transport/THttpServer.cpp:28:23: fatal error: Shlwapi.h: No such file or directory

There might be more cases of these includes, so Work in Progress for now.

[emmenlau]

Thanks, your changes seem reasonable! But do you know if various versions of Visual Studio or the Microsoft toolchain set these names consistently? I think I've seen Windows.h in many projects and places, so is this also a common casing?

[emmenlau]

@evetion I'm sorry but I'm not sure that this is actually portable. I've googled a bit and found https://stackoverflow.com/a/15466951/7200132 where the stance is:

newer VC++ installations and Windows SDKs seem to use Windows.h

I've checked on my development machine with Visual Studio 2022 17.3.3, and found the header to be:

/c/Program Files (x86)/Windows Kits/10/Include/10.0.20348.0/um/Windows.h

So is it possible that your specific development environment is "special"?

[evetion]

It is portable, as Windows is case-insensitive (windows.h, Windows.h and even WiNdOwS.h are the same on Windows). However, when cross-compiling from a case-sensitive environment (most Linux), there's a difference and most tools use windows.h.

In the same answer on Stackoverflow that you linked, it says:

Since windows.h will always work on both Windows and a Linux cross compile, I'd use #include <windows.h> if I ever thought about it.

[emmenlau]

I'm not sure I fully understand this conclusion, please help.

Here is my thinking:

• Based on Stackoverflow, we assume that newer VC++ installations and Windows SDKs seem to use Windows.h (with uppercase first letter), correct?
• If I where to copy or extract such an installation onto a case-sensitive file system, then I would need to use Windows.h as the correct casing, correct?

Why would windows.h (with lowercase spelling) be a better, or even a good, alternative, if the default casing is Windows.h? That seems to jump out of context, or I'm missing something?

[emmenlau]

And also, if Windows.h is the default casing, why does your case-sensitive installation use windows.h instead? Isn't this the "odd" case, and Windows.h is the common case?

[evetion]

Based on Stackoverflow, we assume that newer VC++ installations and Windows SDKs seem to use Windows.h (with uppercase first letter), correct?

Yes.

If I where to copy or extract such an installation onto a case-sensitive file system, then I would need to use Windows.h as the correct casing, correct?

If you could copy/install such an installation yes, but one can't. So that's why in cross-compilation you'd use MinGW, which comes with their own lower-cased headers, such as <windows.h>.

And also, if Windows.h is the default casing, why does your case-sensitive installation use windows.h instead? Isn't this the "odd" case, and Windows.h is the common case?

I agree that my case is the "odd" case here, especially if you're not used to cross-compiling. That said, I think supporting cross-compilation is great for open-source software and the requested change is small and doesn't impact anything (the CI errors should be unrelated). edit: The codebase does contain defined(__MINGW32__) statements, so it seems to support cross-compiling by design.

As I'm not an expert in C++ or even cross-compilation, it might be best for the authors of the previous related PR to chime in @jeremiahpslewis @Jens-G.

[emmenlau]

Dear @evetion , I think I understand now better. The point that cross-compilation would use headers that are not from Microsoft was beyond me. With this context, your request makes more sense.

But still a question:

If I where to copy or extract such an installation onto a case-sensitive file system, then I would need to use Windows.h as the correct casing, correct?

If you could copy/install such an installation yes, but one can't. So that's why in cross-compilation you'd use MinGW, which comes with their own lower-cased headers, such as <windows.h>.

Are you saying its generally impossible to use Visual Studio headers from a case-sensitive file system? If that is true then I consider this PR very valid. But I'm not sure if this can be so easily justified. I remember that I used NFS drives on Windows in the past, and I seem to recall that the Windows NFS-driver was treating them case-sensitive. However that was long ago and my memory may betray me. Therefore I've quickly googled, and it seems there are other cases in which Windows may use case-sensitive file systems, like https://www.howtogeek.com/354220/how-to-enable-case-sensitive-folders-on-windows-10/

I did not search much further, maybe you know more about this?

[hdclark]

+1

I'm encountering the same case sensitivity issue using MXE to cross-compile on Linux for Windows. Compiling only the libraries using the current master branch, the following includes needed to be fixed:

lib/cpp/src/thrift/transport/THttpServer.cpp:29  Shlwapi.h -> shlwapi.h
lib/cpp/src/thrift/transport/TServerSocket.cpp:73   Windows.h -> windows.h
lib/cpp/src/thrift/windows/Sync.h:40  Windows.h -> windows.h
lib/cpp/src/thrift/transport/TPipeServer.cpp:30 AccCtrl.h -> accctrl.h
lib/cpp/src/thrift/transport/TPipeServer.cpp:31 Aclapi.h -> aclapi.h
lib/cpp/src/thrift/windows/SocketPair.cpp:37  WS2tcpip.h -> ws2tcpip.h

In absence of a patch, the following also worked for me on both v0.17.0 release and current master branch:

find repo_root/ \
 -type f \( -iname '*.h' -o -iname '*.cpp' \) \
 -exec sed -i -e 's/Windows[.]h/windows.h/g' \
    -e 's/Shlwapi[.]h/shlwapi.h/g' \
    -e 's/AccCtrl[.]h/accctrl.h/g' \
    -e 's/Aclapi[.]h/aclapi.h/g' \
    -e 's/WS2tcpip[.]h/ws2tcpip.h/g' '{}' \+

[emmenlau]

Sorry that this is such a troublesome issue for such a simple problem. But I am under the impression that there are two contradictory choices:

• use lowercase spelling, which helps for cross-compilation, but breaks case-sensitive Windows installations, and
• use the current spelling, which breaks cross-compilation, but helps for case-sensitive Windows installations.

I'm unsure how to proceed. Comments, suggestions?

[hdclark]

Could we symlink or duplicate the relevant files?

To expand on this, I mean to place a symlink or file 'shlwapi.h' in the local includes directory that links to/duplicates the system 'Shlwapi.h'. It's quite a cumbersome kludge...

[emmenlau]

Could we symlink or duplicate the relevant files?

I'm sorry but I think this is way too much of a hack.

The only "solution" I can think of is to use some build system checks, i.e. in cmake, to see what casing is used for each of these headers, and then use some magic (like defines, or an auto-generated include header) that adapts the includes accordingly. It is just a lot of work for such a "small" annoyance.

Better ideas? And would anyone be willing to propose a solution in code, as a PR?

[evetion]

Sorry that this is such a troublesome issue for such a simple problem. But I am under the impression that there are two contradictory choices:

* use lowercase spelling, which helps for cross-compilation, but breaks case-sensitive Windows installations, and

* use the current spelling, which breaks cross-compilation, but helps for case-sensitive Windows installations.

Agreed that these are contradictory, but pragmatically I would not give them equal footing. There are several people cross-compiling, and the current codebase has some setup for cross-compiling already. The case-sensitive Windows installations seem hypothetical to me, and even if it breaks in some corner case, the question is how often such a setup is used. If I search the apache org for the include: https://github.com/search?q=org%3Aapache+%23include+%3Cwindows.h%3E&type=code, I roughly see 80-90% of the hits use the lowercase variant, so there seem no issues on those repositories.

So I would just go forward with using lowercase everywhere, but then again, I'm biased here ;)

[ctubbsii]

Sorry that this is such a troublesome issue for such a simple problem. But I am under the impression that there are two contradictory choices:

* use lowercase spelling, which helps for cross-compilation, but breaks case-sensitive Windows installations, and

* use the current spelling, which breaks cross-compilation, but helps for case-sensitive Windows installations.

Agreed that these are contradictory, but pragmatically I would not give them equal footing. There are several people cross-compiling, and the current codebase has some setup for cross-compiling already. The case-sensitive Windows installations seem hypothetical to me, and even if it breaks in some corner case, the question is how often such a setup is used. If I search the apache org for the include: https://github.com/search?q=org%3Aapache+%23include+%3Cwindows.h%3E&type=code, I roughly see 80-90% of the hits use the lowercase variant, so there seem no issues on those repositories.

So I would just go forward with using lowercase everywhere, but then again, I'm biased here ;)

I agree that the case-sensitive Windows case is very hypothetical. I'd fix it for the 90% use case using lower case exclusively, and leave it to the outliers to patch their build if they are doing something with a case-sensitive filesystem on Windows and have differently cased files. @emmenlau 's suggestion to have build checks for the correct defines is fine, if somebody wanted to pursue it for those outlier situations, but I don't think it should block this simple fix from being merged.

[evetion]

In related news, my similar PR to apache/arrow#44755 was merged.

[ctubbsii]

@emmenlau Are you still opposed to this patch? I am in favor. I think it addresses the 90% case... where it works on case-insensitive Windows (which nearly 100% of Windows installations use), and works for cross-compilation everywhere else.