Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
evetion wants to merge 2 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
66caf72
Fix Shlwapi capitalization for Windows cross compile.
evetion Sep 4, 2022
401b502
Lowercased all MinGW headers.
evetion Sep 11, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion compiler/cpp/tests/catch/catch.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6338,7 +6338,7 @@ namespace Catch {
#endif

#ifdef __AFXDLL
#include <AfxWin.h>
#include <afxwin.h>
#else
#include <windows.h>
#endif
Expand Down
2 changes: 1 addition & 1 deletion lib/cpp/src/thrift/transport/THttpServer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@
#include <thrift/transport/THttpServer.h>
#include <thrift/transport/TSocket.h>
#if defined(_MSC_VER) || defined(__MINGW32__)
#include <Shlwapi.h>
#include <shlwapi.h>
#endif

using std::string;
Expand Down
4 changes: 2 additions & 2 deletions lib/cpp/src/thrift/transport/TPipeServer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,8 @@
#ifdef _WIN32
#include <thrift/windows/OverlappedSubmissionThread.h>
#include <thrift/windows/Sync.h>
#include <AccCtrl.h>
#include <Aclapi.h>
#include <accctrl.h>
#include <aclapi.h>
#include <sddl.h>
#endif //_WIN32

Expand Down
2 changes: 1 addition & 1 deletion lib/cpp/src/thrift/transport/TServerSocket.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@
// adds problematic macros like min() and max(). Try to work around:
#define NOMINMAX
#define WIN32_LEAN_AND_MEAN
#include <Windows.h>
#include <windows.h>
Copy link
Member

@emmenlau emmenlau Sep 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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"?

Copy link
Contributor Author

@evetion evetion Sep 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@emmenlau emmenlau Sep 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

@emmenlau emmenlau Sep 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@evetion evetion Sep 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@emmenlau emmenlau Sep 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

#undef NOMINMAX
#undef WIN32_LEAN_AND_MEAN
#endif
Expand Down
2 changes: 1 addition & 1 deletion lib/cpp/src/thrift/transport/TWebSocketServer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@
#include <thrift/transport/TSocket.h>
#include <thrift/transport/THttpServer.h>
#if defined(_MSC_VER) || defined(__MINGW32__)
#include <Shlwapi.h>
#include <shlwapi.h>
#define THRIFT_strncasecmp(str1, str2, len) _strnicmp(str1, str2, len)
#define THRIFT_strcasestr(haystack, needle) StrStrIA(haystack, needle)
#else
Expand Down
2 changes: 1 addition & 1 deletion lib/cpp/src/thrift/windows/SocketPair.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@
#include <string.h>

// Win32
#include <WS2tcpip.h>
#include <ws2tcpip.h>

int thrift_socketpair(int d, int type, int protocol, THRIFT_SOCKET sv[2]) {
THRIFT_UNUSED_VARIABLE(protocol);
Expand Down
2 changes: 1 addition & 1 deletion lib/cpp/src/thrift/windows/Sync.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@
#define WIN32_LEAN_AND_MEAN
#define _THRIFT_UNDEF_WIN32_LEAN_AND_MEAN
#endif
#include <Windows.h>
#include <windows.h>
#ifdef _THRIFT_UNDEF_NOMINMAX
#undef NOMINMAX
#undef _THRIFT_UNDEF_NOMINMAX
Expand Down