Makefile.mk: restore _mingw.h for default _WIN32_WINNT
#12217
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
In 8.4.0 we deleted `_mingw.h` as part of purging old-mingw support. Turns out `_mingw.h` had the side-effect of setting a default `_WIN32_WINNT` value expected by `lib/config-win32.h` to enable `getaddinfo` support in `Makefile.mk` mingw-w64 builds. This caused disabling support for this unless specifying the value manually. Restore this header and update its comment to tell why we continue to need it. Regression from 3802910 curl#11625 Reported-by: zhengqwe on github Helped-by: Nico Rieck Fixes curl#12134 Fixes curl#12136 Closes #xxxxx
This was referenced Oct 27, 2023
vszakats
added a commit
to curl/curl-for-win
that referenced
this pull request
Oct 27, 2023
|
Awesome work on this @vszakats ! |
|
Thanks @bagder. After more investigation this seems to be the correct fix. An even more correct fix would be to include |
vszakats
changed the title
_mingw.h to set _WIN32_WINNT_mingw.h for default _WIN32_WINNT
vszakats
added a commit
to curl/curl-for-win
that referenced
this pull request
Oct 27, 2023
We did not set an explicit Windows target version previously. curl (as of v8.4.0) doesn't utilize Windows features beyond Vista, neither do any of the default dependencies. Meaning: binaries are bit-by-bit identical when setting or not setting this. However, it is a good idea to set it explititly to Vista, to ensure we live up to our compatibility goal even if curl or a dependency decides to implement features specific to newer Windows versions. Before this patch targets were defined by toolchain defaults: - x64, x86: 0x0a00 (Win10) - arm64: 0x0601 (Win7) (requires Win10 anyway for the CPU) This also works around regression found in curl v8.4.0: curl/curl#12217
vszakats
added a commit
to curl/curl-for-win
that referenced
this pull request
Oct 27, 2023
We did not set an explicit Windows target version previously. curl (as of v8.4.0) doesn't utilize Windows features beyond Vista, neither do any of the default dependencies. Meaning: binaries are bit-by-bit identical when setting or not setting this. However, it is a good idea to set it explititly to Vista, to ensure we live up to our compatibility goal even if curl or a dependency decides to implement features specific to newer Windows versions. Before this patch targets were defined by toolchain defaults: - x64, x86: 0x0a00 (Win10) - arm64: 0x0601 (Win7) (requires Win10 anyway for the CPU) This also works around the `getaddrinfo` regression found in curl v8.4.0: curl/curl#12217
vszakats
added a commit
to curl/curl-for-win
that referenced
this pull request
Oct 27, 2023
- fix making `.def` timestamp reproducible. - dump imports for Windows curl.exe. - explicitly set target Windows version (to Vista). also works around 8.4.0_1 failing to link to `getaddrinfo`. Ref: curl/curl#12217 8.5.0 fixes this by above upstream PR, and curl-for-win also switches to CMake, which is not affected by the issue.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In 8.4.0 we deleted
_mingw.has part of purging old-mingw support.Turns out
_mingw.hhad the side-effect of setting a default_WIN32_WINNTvalue expected bylib/config-win32.hto enablegetaddrinfosupport inMakefile.mkmingw-w64 builds. This causeddisabling support for this unless specifying the value manually.
Restore this header and update its comment to tell why we continue
to need it.
This triggered a regression in official Windows curl builds starting
with 8.4.0_1. Fixed in 8.4.0_6. (8.5.0 will be using CMake.)
Regression from 3802910 #11625
Reported-by: zhengqwe on github
Helped-by: Nico Rieck
Fixes #12134
Fixes #12136
Closes #12217