[1/2] configure: Default to _WIN32_WINNT=0x0501 as minimum, for legacy mingw

Message ID 20170531102143.2261-1-martin@martin.st
State Committed
Headers show

Commit Message

Martin Storsjö May 31, 2017, 10:21 a.m.
This makes the getaddrinfo functions visible, which aren't normally
by default on legacy mingw.

We already force __MSVCRT_VERSION__ to an XP version.
---
 configure | 2 ++
 1 file changed, 2 insertions(+)

Comments

Diego Biurrun May 31, 2017, 10:30 a.m. | #1
On Wed, May 31, 2017 at 01:21:42PM +0300, Martin Storsjö wrote:
> This makes the getaddrinfo functions visible, which aren't normally
> by default on legacy mingw.
> 
> We already force __MSVCRT_VERSION__ to an XP version.
> ---
>  configure | 2 ++
>  1 file changed, 2 insertions(+)

probably OK

You could mention in the log message what 0x0501 stands for (XP?).

Diego
wm4 May 31, 2017, 10:34 a.m. | #2
On Wed, 31 May 2017 13:21:42 +0300
Martin Storsjö <martin@martin.st> wrote:

> This makes the getaddrinfo functions visible, which aren't normally
> by default on legacy mingw.
> 
> We already force __MSVCRT_VERSION__ to an XP version.
> ---
>  configure | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/configure b/configure
> index fb3920a261..77926bb85b 100755
> --- a/configure
> +++ b/configure
> @@ -4133,6 +4133,8 @@ probe_libc(){
>          add_${pfx}cppflags -U__STRICT_ANSI__ -D__USE_MINGW_ANSI_STDIO=1
>          check_${pfx}cpp_condition _mingw.h "__MSVCRT_VERSION__ < 0x0700" &&
>              add_${pfx}cppflags -D__MSVCRT_VERSION__=0x0700
> +        check_${pfx}cpp_condition windows.h "_WIN32_WINNT < 0x0501" &&
> +            add_${pfx}cppflags -D_WIN32_WINNT=0x0501
>          eval test \$${pfx_no_}cc_type = "gcc" &&
>              add_${pfx}cppflags -D__printf__=__gnu_printf__
>      elif check_${pfx}cpp_condition crtversion.h "defined _VC_CRT_MAJOR_VERSION"; then

That's all nice and good, but why not drop support for legacy mingw?
Martin Storsjö May 31, 2017, 10:38 a.m. | #3
On Wed, 31 May 2017, wm4 wrote:

> On Wed, 31 May 2017 13:21:42 +0300
> Martin Storsjö <martin@martin.st> wrote:
>
>> This makes the getaddrinfo functions visible, which aren't normally
>> by default on legacy mingw.
>> 
>> We already force __MSVCRT_VERSION__ to an XP version.
>> ---
>>  configure | 2 ++
>>  1 file changed, 2 insertions(+)
>> 
>> diff --git a/configure b/configure
>> index fb3920a261..77926bb85b 100755
>> --- a/configure
>> +++ b/configure
>> @@ -4133,6 +4133,8 @@ probe_libc(){
>>          add_${pfx}cppflags -U__STRICT_ANSI__ -D__USE_MINGW_ANSI_STDIO=1
>>          check_${pfx}cpp_condition _mingw.h "__MSVCRT_VERSION__ < 0x0700" &&
>>              add_${pfx}cppflags -D__MSVCRT_VERSION__=0x0700
>> +        check_${pfx}cpp_condition windows.h "_WIN32_WINNT < 0x0501" &&
>> +            add_${pfx}cppflags -D_WIN32_WINNT=0x0501
>>          eval test \$${pfx_no_}cc_type = "gcc" &&
>>              add_${pfx}cppflags -D__printf__=__gnu_printf__
>>      elif check_${pfx}cpp_condition crtversion.h "defined _VC_CRT_MAJOR_VERSION"; then
>
> That's all nice and good, but why not drop support for legacy mingw?

Well, given that we had 46e3936fb04 in for half a year without any 
complaints about the missing alignment, we probably don't have many 
serious users of it, so I guess it could be done. I have an ancient 
cross-environment of it set up that I test occasionally though.

In this case, we don't really gain much else than these few lines in 
configure, or is there something else we're missing? In that case I don't 
mind keeping them for another few years unless they end up being a bigger 
problem.

// Martin
Hendrik Leppkes May 31, 2017, 10:40 a.m. | #4
On Wed, May 31, 2017 at 12:21 PM, Martin Storsjö <martin@martin.st> wrote:
> This makes the getaddrinfo functions visible, which aren't normally
> by default on legacy mingw.
>
> We already force __MSVCRT_VERSION__ to an XP version.
> ---
>  configure | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/configure b/configure
> index fb3920a261..77926bb85b 100755
> --- a/configure
> +++ b/configure
> @@ -4133,6 +4133,8 @@ probe_libc(){
>          add_${pfx}cppflags -U__STRICT_ANSI__ -D__USE_MINGW_ANSI_STDIO=1
>          check_${pfx}cpp_condition _mingw.h "__MSVCRT_VERSION__ < 0x0700" &&
>              add_${pfx}cppflags -D__MSVCRT_VERSION__=0x0700
> +        check_${pfx}cpp_condition windows.h "_WIN32_WINNT < 0x0501" &&
> +            add_${pfx}cppflags -D_WIN32_WINNT=0x0501
>          eval test \$${pfx_no_}cc_type = "gcc" &&
>              add_${pfx}cppflags -D__printf__=__gnu_printf__
>      elif check_${pfx}cpp_condition crtversion.h "defined _VC_CRT_MAJOR_VERSION"; then
> --
> 2.11.0 (Apple Git-81)
>

Perhaps use 0x0502 to match the value we use for MSVC?

- Hendrik
Martin Storsjö May 31, 2017, 10:43 a.m. | #5
On Wed, 31 May 2017, Hendrik Leppkes wrote:

> On Wed, May 31, 2017 at 12:21 PM, Martin Storsjö <martin@martin.st> wrote:
>> This makes the getaddrinfo functions visible, which aren't normally
>> by default on legacy mingw.
>>
>> We already force __MSVCRT_VERSION__ to an XP version.
>> ---
>>  configure | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/configure b/configure
>> index fb3920a261..77926bb85b 100755
>> --- a/configure
>> +++ b/configure
>> @@ -4133,6 +4133,8 @@ probe_libc(){
>>          add_${pfx}cppflags -U__STRICT_ANSI__ -D__USE_MINGW_ANSI_STDIO=1
>>          check_${pfx}cpp_condition _mingw.h "__MSVCRT_VERSION__ < 0x0700" &&
>>              add_${pfx}cppflags -D__MSVCRT_VERSION__=0x0700
>> +        check_${pfx}cpp_condition windows.h "_WIN32_WINNT < 0x0501" &&
>> +            add_${pfx}cppflags -D_WIN32_WINNT=0x0501
>>          eval test \$${pfx_no_}cc_type = "gcc" &&
>>              add_${pfx}cppflags -D__printf__=__gnu_printf__
>>      elif check_${pfx}cpp_condition crtversion.h "defined _VC_CRT_MAJOR_VERSION"; then
>> --
>> 2.11.0 (Apple Git-81)
>>
>
> Perhaps use 0x0502 to match the value we use for MSVC?

Sure, I can do that.

// Martin
James Almer May 31, 2017, 12:25 p.m. | #6
On 5/31/2017 7:21 AM, Martin Storsjö wrote:
> This makes the getaddrinfo functions visible, which aren't normally
> by default on legacy mingw.
> 
> We already force __MSVCRT_VERSION__ to an XP version.
> ---
>  configure | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/configure b/configure
> index fb3920a261..77926bb85b 100755
> --- a/configure
> +++ b/configure
> @@ -4133,6 +4133,8 @@ probe_libc(){
>          add_${pfx}cppflags -U__STRICT_ANSI__ -D__USE_MINGW_ANSI_STDIO=1
>          check_${pfx}cpp_condition _mingw.h "__MSVCRT_VERSION__ < 0x0700" &&
>              add_${pfx}cppflags -D__MSVCRT_VERSION__=0x0700
> +        check_${pfx}cpp_condition windows.h "_WIN32_WINNT < 0x0501" &&

defined(_WIN32_WINNT) && _WIN32_WINNT < 0x0501

Or 0x0502 as Hendrik suggested.
Martin Storsjö May 31, 2017, 12:31 p.m. | #7
On Wed, 31 May 2017, James Almer wrote:

> On 5/31/2017 7:21 AM, Martin Storsjö wrote:
>> This makes the getaddrinfo functions visible, which aren't normally
>> by default on legacy mingw.
>> 
>> We already force __MSVCRT_VERSION__ to an XP version.
>> ---
>>  configure | 2 ++
>>  1 file changed, 2 insertions(+)
>> 
>> diff --git a/configure b/configure
>> index fb3920a261..77926bb85b 100755
>> --- a/configure
>> +++ b/configure
>> @@ -4133,6 +4133,8 @@ probe_libc(){
>>          add_${pfx}cppflags -U__STRICT_ANSI__ -D__USE_MINGW_ANSI_STDIO=1
>>          check_${pfx}cpp_condition _mingw.h "__MSVCRT_VERSION__ < 0x0700" &&
>>              add_${pfx}cppflags -D__MSVCRT_VERSION__=0x0700
>> +        check_${pfx}cpp_condition windows.h "_WIN32_WINNT < 0x0501" &&
>
> defined(_WIN32_WINNT) && _WIN32_WINNT < 0x0501

Well, after including windows.h, I would expect that it is defined 
already, but sure, I can add the extra defined().

I intentionally picked windows.h because e.g. legacy mingw32 defines 
the default in windef.h + winver.h, while mingw-w64 defines it in 
_mingw.h. This codeblock should only be used for legacy mingw32, but 
despite that I wanted to have a test that should work as intended even 
outside of that.

Hendrik mentioned that you might have something similiar in ffmpeg 
already, and I found 69f7aad5710 authored by you. There I noticed that you 
only check _mingw.h, but at least in my old mingw32 version, _mingw.h 
doesn't actually define _WIN32_WINNT at all, so the test there probably 
doesn't really check what you intended.

> Or 0x0502 as Hendrik suggested.

Sure, I've already amended it that way locally.

// Martin
James Almer May 31, 2017, 12:59 p.m. | #8
On 5/31/2017 9:31 AM, Martin Storsjö wrote:
> On Wed, 31 May 2017, James Almer wrote:
> 
>> On 5/31/2017 7:21 AM, Martin Storsjö wrote:
>>> This makes the getaddrinfo functions visible, which aren't normally
>>> by default on legacy mingw.
>>>
>>> We already force __MSVCRT_VERSION__ to an XP version.
>>> ---
>>>  configure | 2 ++
>>>  1 file changed, 2 insertions(+)
>>>
>>> diff --git a/configure b/configure
>>> index fb3920a261..77926bb85b 100755
>>> --- a/configure
>>> +++ b/configure
>>> @@ -4133,6 +4133,8 @@ probe_libc(){
>>>          add_${pfx}cppflags -U__STRICT_ANSI__ -D__USE_MINGW_ANSI_STDIO=1
>>>          check_${pfx}cpp_condition _mingw.h "__MSVCRT_VERSION__ <
>>> 0x0700" &&
>>>              add_${pfx}cppflags -D__MSVCRT_VERSION__=0x0700
>>> +        check_${pfx}cpp_condition windows.h "_WIN32_WINNT < 0x0501" &&
>>
>> defined(_WIN32_WINNT) && _WIN32_WINNT < 0x0501
> 
> Well, after including windows.h, I would expect that it is defined
> already, but sure, I can add the extra defined().
> 
> I intentionally picked windows.h because e.g. legacy mingw32 defines the
> default in windef.h + winver.h, while mingw-w64 defines it in _mingw.h.
> This codeblock should only be used for legacy mingw32, but despite that
> I wanted to have a test that should work as intended even outside of that.
> 
> Hendrik mentioned that you might have something similiar in ffmpeg
> already, and I found 69f7aad5710 authored by you. There I noticed that
> you only check _mingw.h, but at least in my old mingw32 version,
> _mingw.h doesn't actually define _WIN32_WINNT at all, so the test there
> probably doesn't really check what you intended.

The check

"defined(_WIN32_WINNT) && _WIN32_WINNT >= 0x0502" || add_${pfx}cppflags
-D_WIN32_WINNT=0x0502

will succeed if _WIN32_WINNT is not defined, or if it's defined by the
header or the user with for example --extra-cflags with a value lower
than 0x0502. That way said value is forced as a minimum.
If what you say is true then it would only be a problem if a mingw32
toolchain at some point starts defaulting to for example 0x0601, in
which case it would wrongly supersede it, but i don't see that ever
happening. Including windows.h may be a good idea anyway to prevent that.

I didn't test that commit but others did before i pushed it, and it was
reportedly working as intended.
Martin Storsjö May 31, 2017, 1:05 p.m. | #9
On Wed, 31 May 2017, James Almer wrote:

> On 5/31/2017 9:31 AM, Martin Storsjö wrote:
>> On Wed, 31 May 2017, James Almer wrote:
>> 
>>> On 5/31/2017 7:21 AM, Martin Storsjö wrote:
>>>> This makes the getaddrinfo functions visible, which aren't normally
>>>> by default on legacy mingw.
>>>>
>>>> We already force __MSVCRT_VERSION__ to an XP version.
>>>> ---
>>>>  configure | 2 ++
>>>>  1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/configure b/configure
>>>> index fb3920a261..77926bb85b 100755
>>>> --- a/configure
>>>> +++ b/configure
>>>> @@ -4133,6 +4133,8 @@ probe_libc(){
>>>>          add_${pfx}cppflags -U__STRICT_ANSI__ -D__USE_MINGW_ANSI_STDIO=1
>>>>          check_${pfx}cpp_condition _mingw.h "__MSVCRT_VERSION__ <
>>>> 0x0700" &&
>>>>              add_${pfx}cppflags -D__MSVCRT_VERSION__=0x0700
>>>> +        check_${pfx}cpp_condition windows.h "_WIN32_WINNT < 0x0501" &&
>>>
>>> defined(_WIN32_WINNT) && _WIN32_WINNT < 0x0501
>> 
>> Well, after including windows.h, I would expect that it is defined
>> already, but sure, I can add the extra defined().
>> 
>> I intentionally picked windows.h because e.g. legacy mingw32 defines the
>> default in windef.h + winver.h, while mingw-w64 defines it in _mingw.h.
>> This codeblock should only be used for legacy mingw32, but despite that
>> I wanted to have a test that should work as intended even outside of that.
>> 
>> Hendrik mentioned that you might have something similiar in ffmpeg
>> already, and I found 69f7aad5710 authored by you. There I noticed that
>> you only check _mingw.h, but at least in my old mingw32 version,
>> _mingw.h doesn't actually define _WIN32_WINNT at all, so the test there
>> probably doesn't really check what you intended.
>
> The check
>
> "defined(_WIN32_WINNT) && _WIN32_WINNT >= 0x0502" || add_${pfx}cppflags
> -D_WIN32_WINNT=0x0502
>
> will succeed if _WIN32_WINNT is not defined, or if it's defined by the
> header or the user with for example --extra-cflags with a value lower
> than 0x0502. That way said value is forced as a minimum.
> If what you say is true then it would only be a problem if a mingw32
> toolchain at some point starts defaulting to for example 0x0601, in
> which case it would wrongly supersede it, but i don't see that ever
> happening. Including windows.h may be a good idea anyway to prevent that.
>
> I didn't test that commit but others did before i pushed it, and it was
> reportedly working as intended.

Yes, the only case where it wouldn't is the purely hypothetical (but IMO 
implied) case where the headers would default to a newer version.

It's not something that is happening, but by currently including _mingw.h 
and checking these, it kinda implies that this header would set the mingw 
specific default value of _WIN32_WINNT - and it doesn't.

// Martin
Rémi Denis-Courmont May 31, 2017, 4:29 p.m. | #10
Le keskiviikkona 31. toukokuuta 2017, 12.30.34 EEST Diego Biurrun a écrit :
> On Wed, May 31, 2017 at 01:21:42PM +0300, Martin Storsjö wrote:
> > This makes the getaddrinfo functions visible, which aren't normally
> > by default on legacy mingw.
> > 
> > We already force __MSVCRT_VERSION__ to an XP version.
> > ---
> > 
> >  configure | 2 ++
> >  1 file changed, 2 insertions(+)
> 
> probably OK
> 
> You could mention in the log message what 0x0501 stands for (XP?).

There are macros defining the useful values. For this specific case, 
_WIN32_WINNT_WINXP exists.
Hendrik Leppkes May 31, 2017, 4:50 p.m. | #11
On Wed, May 31, 2017 at 6:29 PM, Rémi Denis-Courmont <remi@remlab.net> wrote:
> Le keskiviikkona 31. toukokuuta 2017, 12.30.34 EEST Diego Biurrun a écrit :
>> On Wed, May 31, 2017 at 01:21:42PM +0300, Martin Storsjö wrote:
>> > This makes the getaddrinfo functions visible, which aren't normally
>> > by default on legacy mingw.
>> >
>> > We already force __MSVCRT_VERSION__ to an XP version.
>> > ---
>> >
>> >  configure | 2 ++
>> >  1 file changed, 2 insertions(+)
>>
>> probably OK
>>
>> You could mention in the log message what 0x0501 stands for (XP?).
>
> There are macros defining the useful values. For this specific case,
> _WIN32_WINNT_WINXP exists.
>

If you want to use a macro, then use _WIN32_WINNT_WS03, which is the
one that corresponds to 0x0502 (XP SP2 or Server 2003 SP1)

- Hendrik
Martin Storsjö June 1, 2017, 10:30 a.m. | #12
On Wed, 31 May 2017, Hendrik Leppkes wrote:

> On Wed, May 31, 2017 at 6:29 PM, Rémi Denis-Courmont <remi@remlab.net> wrote:
>> Le keskiviikkona 31. toukokuuta 2017, 12.30.34 EEST Diego Biurrun a écrit :
>>> On Wed, May 31, 2017 at 01:21:42PM +0300, Martin Storsjö wrote:
>>> > This makes the getaddrinfo functions visible, which aren't normally
>>> > by default on legacy mingw.
>>> >
>>> > We already force __MSVCRT_VERSION__ to an XP version.
>>> > ---
>>> >
>>> >  configure | 2 ++
>>> >  1 file changed, 2 insertions(+)
>>>
>>> probably OK
>>>
>>> You could mention in the log message what 0x0501 stands for (XP?).
>>
>> There are macros defining the useful values. For this specific case,
>> _WIN32_WINNT_WINXP exists.
>>
>
> If you want to use a macro, then use _WIN32_WINNT_WS03, which is the
> one that corresponds to 0x0502 (XP SP2 or Server 2003 SP1)

Kept the numeric constant for consistency with the other cases of the same 
in configure, pushed with the other comments applied.

// Martin

Patch

diff --git a/configure b/configure
index fb3920a261..77926bb85b 100755
--- a/configure
+++ b/configure
@@ -4133,6 +4133,8 @@  probe_libc(){
         add_${pfx}cppflags -U__STRICT_ANSI__ -D__USE_MINGW_ANSI_STDIO=1
         check_${pfx}cpp_condition _mingw.h "__MSVCRT_VERSION__ < 0x0700" &&
             add_${pfx}cppflags -D__MSVCRT_VERSION__=0x0700
+        check_${pfx}cpp_condition windows.h "_WIN32_WINNT < 0x0501" &&
+            add_${pfx}cppflags -D_WIN32_WINNT=0x0501
         eval test \$${pfx_no_}cc_type = "gcc" &&
             add_${pfx}cppflags -D__printf__=__gnu_printf__
     elif check_${pfx}cpp_condition crtversion.h "defined _VC_CRT_MAJOR_VERSION"; then