Enable IDN on Windows when building with clang#12606
Enable IDN on Windows when building with clang#12606mraunak wants to merge 2 commits intocurl:masterfrom
Conversation
bagder
changed the title
|
So why isn't |
|
It doesn't look right to enable Windows SDK-specific code based on compiler vendor. What needs to be seen is why this build env is missing these Windows declarations What build tool, build config, curl version is this happening with? |
mraunak
changed the title
|
@mraunak: this PR awaits info from you... |
|
Hi Team, Thank you very much for your input. idn.c gets compiled successfully with CLANG 15.0.7 but has the error shown above when compiled with CLANG 17 Build tool: Bazel version 6.4.0 Build config: Curl version: curl 8.4.0 (Windows) libcurl/8.4.0 Schannel WinIDN Clang version: 17.0.6 |
|
Could you show the actual compile error? That's not included in that snippet. |
|
Hi @dfandrich, please find the attached log with the error. Please let me know if any further information is needed from my end |
|
The underlying errors seem to be the same as in your initial screenshot: So, the question from vszakats still stands: why this build env is missing these Windows declarations? |
|
Hi Team, IdnToUnicode and IdnToASCII function is handled by WinNls.h The other solution is to update the TensorFlow configuration file for the Windows platform, for this approach, there is no need for a PR to Curl. |
|
If this is caused by mismatched Edit: As seen in Windows SDK 7.1 and mingw-w64 Windows headers, if undefined, |
|
This could be caused by missing defines/includes/files to compile&link in https://github.com/tensorflow/tensorflow/blob/master/third_party/curl.BUILD |
|
Is this correct though? According to the comment it's trying to fix something in curl The Tensorflow build hack triggers some logic in curl, which Lines 320 to 380 in 24ae4a0 This logic in curl seems dangerous and possibly obsolete, but I haven't tried but this problem might be fixed by deleting either The other logic in curl that touches Lines 2471 to 2488 in 24ae4a0 Added as part of a commit fixing legacy-mingw issues in 37f1c21. |
|
Hi Team, Thank you very much for your input, removing the hack (D_USING_V110_SDK71_) works. |
I will check this with some old Visual Studio versions. I think it was primarily meant as workarounds for old versions (which are still supported via winbuild and pregenerated projects). I don't know why it would be applied to new versions. Regardless that tensor hack is fishy.
|
- If the minimum Windows target OS is pre-Vista (_WIN32_WINNT < 0x600) then error. - Stop comparing against WINVER when making the minimum OS determinations (ie check only _WIN32_WINNT instead of both since that's the one that matters). Prior to this change, if --with-winidn was used in such a case configure would change the minimum target OS to Vista (0x600) so the build would compile. That was done primarily as a workaround for original MinGW which we no longer support. Bug: curl#12606 (comment) Reported-by: Viktor Szakats Closes #xxxxx
|
@mraunak I take it this PR is no longer actually necessary or wanted? |
|
Hi @bagder, yes, we can close it. Thank you! |
|
Thanks! |
Ref: curl#12684 Ref: curl#12606 (comment) Ref: curl@37f1c21
Originall added in 37f1c21 curl#7581 for classic mingw. curl no longer supports building with classic mingw. Ref: curl#12684 Ref: curl#12606 (comment)
1. GHA/windows: enable WinIDN in Linux cross-builds.
(to reveal the issue in CI.)
2. fix compiler warning when building with mingw-w64 supporting
WinIDN, while targeting pre-Vista Windows, with a `WINVER` set to
target Vista or newer. (Such was Ubuntu's mingw-w64 with the
classic-mingw-specific trick in point 3 of this PR.)
```
../../lib/idn.c:154:23: error: redundant redeclaration of ‘IdnToAscii’ [-Werror=redundant-decls]
154 | WINBASEAPI int WINAPI IdnToAscii(DWORD dwFlags,
| ^~~~~~~~~~
In file included from /usr/share/mingw-w64/include/windows.h:73,
from /usr/share/mingw-w64/include/winsock2.h:23,
from ../../lib/setup-win32.h:91,
from ../../lib/curl_setup.h:308,
from ../../lib/idn.c:29:
/usr/share/mingw-w64/include/winnls.h:1075:30: note: previous declaration of ‘IdnToAscii’ was here
1075 | WINNORMALIZEAPI int WINAPI IdnToAscii (DWORD dwFlags, LPCWSTR lpUnicodeCharStr, int cchUnicodeChar, LPWSTR lpASCIICharStr, int cchASCIIChar);
| ^~~~~~~~~~
[...same for IdnToUnicode...]
```
Ref: https://github.com/curl/curl/actions/runs/10542832783/job/29210098553#step:7:89
3. drop `WINVER` override for classic-mingw. curl no longer supports
building with classic-mingw.
Reverts 37f1c21 #7581
4. sync `if IdnToUnicode can be linked` detection snippet with the live
code in `lib/idn.c`. It fixes detection for the scenario in point 2.
5. delete unused `WINIDN_DIR` variable.
Bug: #12606 (comment)
Previous abandoned attempt: #12684
Reviewed-by: Jay Satiro
Closes #14680
7 participants
This PR aims to enable IDN on the Windows platform with CLANG compiler version 17 which is required to successfully
build TensorFlow-CPU on the Windows
Changed https://github.com/curl/curl/blob/master/lib/idn.c to use an inbuilt Windows function IdnToAscii on the Clang Compiler
Below is the screenshot of the error: