You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@thrift.apache.org by GitBox <gi...@apache.org> on 2022/09/04 15:19:45 UTC
[GitHub] [thrift] evetion opened a new pull request, #2649: Fix capitalization for case sensitive cross compile.
evetion opened a new pull request, #2649:
URL: https://github.com/apache/thrift/pull/2649
Like #2518, in cross compiling thrift in https://github.com/JuliaPackaging/Yggdrasil/pull/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
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] hdclark commented on pull request #2649: [WIP] Fix , capitalization for case sensitive cross compile.
Posted by GitBox <gi...@apache.org>.
hdclark commented on PR #2649:
URL: https://github.com/apache/thrift/pull/2649#issuecomment-1335648315
+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:30 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' '{}' \+
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] emmenlau commented on a diff in pull request #2649: [WIP] Fix , capitalization for case sensitive cross compile.
Posted by GitBox <gi...@apache.org>.
emmenlau commented on code in PR #2649:
URL: https://github.com/apache/thrift/pull/2649#discussion_r967849635
##########
lib/cpp/src/thrift/transport/TServerSocket.cpp:
##########
@@ -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>
Review Comment:
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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] evetion commented on a diff in pull request #2649: [WIP] Fix , capitalization for case sensitive cross compile.
Posted by GitBox <gi...@apache.org>.
evetion commented on code in PR #2649:
URL: https://github.com/apache/thrift/pull/2649#discussion_r967793917
##########
lib/cpp/src/thrift/transport/TServerSocket.cpp:
##########
@@ -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>
Review Comment:
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](https://stackoverflow.com/a/15466951/7200132), 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] evetion commented on a diff in pull request #2649: [WIP] Fix , capitalization for case sensitive cross compile.
Posted by GitBox <gi...@apache.org>.
evetion commented on code in PR #2649:
URL: https://github.com/apache/thrift/pull/2649#discussion_r967848189
##########
lib/cpp/src/thrift/transport/TServerSocket.cpp:
##########
@@ -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>
Review Comment:
> 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](https://github.com/apache/thrift/pull/2518) to chime in @jeremiahpslewis @Jens-G.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] emmenlau commented on pull request #2649: [WIP] Fix , capitalization for case sensitive cross compile.
Posted by GitBox <gi...@apache.org>.
emmenlau commented on PR #2649:
URL: https://github.com/apache/thrift/pull/2649#issuecomment-1241685909
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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] emmenlau commented on a diff in pull request #2649: [WIP] Fix , capitalization for case sensitive cross compile.
Posted by GitBox <gi...@apache.org>.
emmenlau commented on code in PR #2649:
URL: https://github.com/apache/thrift/pull/2649#discussion_r967835015
##########
lib/cpp/src/thrift/transport/TServerSocket.cpp:
##########
@@ -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>
Review Comment:
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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] emmenlau commented on a diff in pull request #2649: [WIP] Fix , capitalization for case sensitive cross compile.
Posted by GitBox <gi...@apache.org>.
emmenlau commented on code in PR #2649:
URL: https://github.com/apache/thrift/pull/2649#discussion_r967835164
##########
lib/cpp/src/thrift/transport/TServerSocket.cpp:
##########
@@ -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>
Review Comment:
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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] emmenlau commented on a diff in pull request #2649: [WIP] Fix , capitalization for case sensitive cross compile.
Posted by GitBox <gi...@apache.org>.
emmenlau commented on code in PR #2649:
URL: https://github.com/apache/thrift/pull/2649#discussion_r967640902
##########
lib/cpp/src/thrift/transport/TServerSocket.cpp:
##########
@@ -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>
Review Comment:
@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"?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org