Skip to content
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.

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

windows: Set _WIN32_WINNT to 0x0601 (Windows 7) #14922

Merged
merged 4 commits into from Feb 5, 2019

Conversation

Projects
None yet
9 participants
@ken2812221
Copy link
Member

commented Dec 11, 2018

@laanwj

This comment has been minimized.

Copy link
Member

commented Dec 11, 2018

Is that so? I know we dropped Windows XP support, but did we drop support for Vista?

@ken2812221 ken2812221 force-pushed the ken2812221:patch-1 branch Dec 11, 2018

@ken2812221

This comment has been minimized.

Copy link
Member Author

commented Dec 11, 2018

@laanwj Oh, I saw #12546 and thought that we didn't support Vista anymore. Changed to 0x0600 (Vista)

@ken2812221 ken2812221 force-pushed the ken2812221:patch-1 branch Dec 11, 2018

@ken2812221 ken2812221 changed the title windows: Set_WIN32_WINNT to 0x0601 (Windows 7) windows: Set _WIN32_WINNT to 0x0600 (Windows Vista) Dec 11, 2018

@laanwj

This comment has been minimized.

Copy link
Member

commented Dec 11, 2018

I mean, we could still decide to drop Vista support for 0.18.0 if there's a good reason for it, but it'll require some discussion.
(unless support for Vista was already dropped, but I don't know and cannot find any discussion of that?)

What does this change do?

@ken2812221

This comment has been minimized.

Copy link
Member Author

commented Dec 11, 2018

#14881 is using inet_pton and it's only for Vista or later. So I create this PR just for that.

@kristapsk

This comment has been minimized.

Copy link
Contributor

commented Dec 11, 2018

If inet_pton is the only reason, that could be easily re-implemented. Of course, if there is a need to keep Vista support. I personnaly don't care. :)

@laanwj

This comment has been minimized.

Copy link
Member

commented Dec 11, 2018

@kristapsk Vista supports that, which is the minimum that is supported, so now after changing the minimum (initially it was changing it to W7) to Vista this PR is non-controversial.

@kristapsk

This comment has been minimized.

Copy link
Contributor

commented Dec 11, 2018

Ok, then it's strong utACK from me, nobody should run Bitcoin Core on XP anymore.

@kristapsk

This comment has been minimized.

Copy link
Contributor

commented Dec 12, 2018

But should be mentioned in release notes.

@luke-jr

This comment has been minimized.

Copy link
Member

commented Dec 12, 2018

Prefer to see this kind of change merged as part of a PR that needs/uses it.

Release notes are already clear that XP isn't supported, for several versions now.

@bitcoin bitcoin deleted a comment from aekanan Dec 12, 2018

@laanwj

This comment has been minimized.

Copy link
Member

commented Dec 12, 2018

But should be mentioned in release notes.

XP has already not been supported since 0.13.0 in 2016, which was explicitly mentioned in the release notes then (and many a release after that): https://github.com/bitcoin/bitcoin/blob/master/doc/release-notes/release-notes-0.13.0.md#compatibility
There's no need for any argument or discussion about this here.

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Dec 12, 2018

Gitian builds for commit 5f23460 (master):

Gitian builds for commit 5d233926dec4b3df52849cd15e4a86429adfd8bc (master and this pull):

@fanquake

This comment has been minimized.

Copy link
Member

commented Dec 12, 2018

FWIW I spun up a Windows Vista (SP2) VM and downloaded the v0.17.0.1 binary. It "seems" to run ok, although I didn't test extensively:
0 17 0 1 on windows vista

However, Qt last listed Vista as a supported platform (deployment only) in 5.6 (LTS - Mar 2019), so I'd be happy to suggest tagging this for 0.18.0, revert to bumping to 0x0601 (Windows 7), and updating any release-notes/supported versions text in this PR.

I'd be pretty surprised if anyone is testing on Vista (at all), and given Qt has dropped support for it, I assume it's just a matter of time before it will stop working (possibly randomly), especially if we move to a newer Qt for release builds in 0.18.0.

@ken2812221 ken2812221 force-pushed the ken2812221:patch-1 branch Dec 12, 2018

@fanquake fanquake added this to the 0.18.0 milestone Dec 12, 2018

@fanquake fanquake changed the title windows: Set _WIN32_WINNT to 0x0600 (Windows Vista) windows: Set _WIN32_WINNT to 0x0601 (Windows 7) Dec 12, 2018

@fanquake

This comment has been minimized.

Copy link
Member

commented Dec 12, 2018

@ken2812221 Could you update this PR to include docs changes.
Is the src/zmq/zmqabstractnotifier.cpp change from your last force-push intended?

Related IRC discussion here, (lines 284 - ~335).

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Dec 12, 2018

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Dec 12, 2018

Gitian fails with:

  CXX      zmq/libbitcoin_zmq_a-zmqabstractnotifier.o
In file included from ./compat.h:31:0,
                 from ./util/system.h:18,
                 from zmq/zmqabstractnotifier.cpp:6:
/usr/share/mingw-w64/include/mswsock.h:201:5: error: ‘WSAPOLLFD’ does not name a type
     WSAPOLLFD fdArray[0];
     ^~~~~~~~~
/usr/share/mingw-w64/include/mswsock.h:204:39: error: typedef ‘LPFN_WSAPOLL’ is initialized (use decltype instead)
   typedef INT (WSAAPI *LPFN_WSAPOLL) (LPWSAPOLLFD fdarray, ULONG nfds, INT timeout);
                                       ^~~~~~~~~~~
/usr/share/mingw-w64/include/mswsock.h:204:39: error: ‘LPWSAPOLLFD’ was not declared in this scope
/usr/share/mingw-w64/include/mswsock.h:204:39: note: suggested alternative: ‘LPWSAPOLLDATA’
   typedef INT (WSAAPI *LPFN_WSAPOLL) (LPWSAPOLLFD fdarray, ULONG nfds, INT timeout);
                                       ^~~~~~~~~~~
                                       LPWSAPOLLDATA
/usr/share/mingw-w64/include/mswsock.h:204:66: error: expected primary-expression before ‘nfds’
   typedef INT (WSAAPI *LPFN_WSAPOLL) (LPWSAPOLLFD fdarray, ULONG nfds, INT timeout);
                                                                  ^~~~
/usr/share/mingw-w64/include/mswsock.h:204:76: error: expected primary-expression before ‘timeout’
   typedef INT (WSAAPI *LPFN_WSAPOLL) (LPWSAPOLLFD fdarray, ULONG nfds, INT timeout);
                                                                            ^~~~~~~
Makefile:7050: recipe for target 'zmq/libbitcoin_zmq_a-zmqabstractnotifier.o' failed
make[2]: *** [zmq/libbitcoin_zmq_a-zmqabstractnotifier.o] Error 1
make[2]: Leaving directory '/home/ubuntu/build/bitcoin/distsrc-i686-w64-mingw32/src'
Makefile:10392: recipe for target 'all-recursive' failed
make[1]: *** [all-recursive] Error 1
make[1]: Leaving directory '/home/ubuntu/build/bitcoin/distsrc-i686-w64-mingw32/src'
Makefile:775: recipe for target 'all-recursive' failed
make: *** [all-recursive] Error 1
@ken2812221

This comment has been minimized.

Copy link
Member Author

commented Dec 12, 2018

@MarcoFalke Gitian build would still be failed now. It's still under investigation.

@fanquake

This comment has been minimized.

Copy link
Member

commented Dec 12, 2018

One other thought, if we're changing this, do we want to explicitly pass the same version to anything in depends? i.e at the moment miniupnpc ends up being built with -D_WIN32_WINNT=0X501.

@ken2812221 ken2812221 changed the title windows: Set _WIN32_WINNT to 0x0601 (Windows 7) [WIP] windows: Set _WIN32_WINNT to 0x0601 (Windows 7) Dec 13, 2018

@ken2812221

This comment has been minimized.

Copy link
Member Author

commented Dec 20, 2018

TBH as it's part of the code itself (the "contract" with Windows), I personally prefer it to be defined in the code, not in the build system.
(this helps analysis tools, other IDEs, MSVC, ...)

Mingw has defined _WIN32_WINNT=0x0502 by default. This would cause compile issue if we include windows.h first and define _WIN32_WINNT later. So I think it would be better to define it in build system. So it could be defined at first. We don't have to consider the include order.

@ken2812221 ken2812221 force-pushed the ken2812221:patch-1 branch Dec 20, 2018

@laanwj

This comment has been minimized.

Copy link
Member

commented Dec 20, 2018

@ken2812221 Sure, but what I don't understand is how this differs from the previous setting. What would go wrong with a patch that changes the value 0x0501 to 0x0601 in the existing files and nothing else?

@ken2812221

This comment has been minimized.

Copy link
Member Author

commented Jan 2, 2019

Since #14881 was closed, this PR shall not be necessary anymore. This change could cause compilation issue on Mingw or we would have to define _WIN32_WINNT in every file that we include windows related headers. I think it does not worth to do that.

@ken2812221 ken2812221 closed this Jan 2, 2019

@ken2812221 ken2812221 deleted the ken2812221:patch-1 branch Jan 2, 2019

@fanquake

This comment has been minimized.

Copy link
Member

commented Jan 2, 2019

@ken2812221 #14881 has just been moved to #15052.

@ken2812221 ken2812221 restored the ken2812221:patch-1 branch Jan 23, 2019

@ken2812221 ken2812221 reopened this Jan 23, 2019

windows: Set _WIN32_WINNT to 0x0601 (Windows 7)
Also remove all defines in many places and define it in configure stage to keep consistency.

@ken2812221 ken2812221 force-pushed the ken2812221:patch-1 branch to 1bd9ffd Jan 23, 2019

@Empact

This comment has been minimized.

Copy link
Member

commented Jan 23, 2019

@Empact

This comment has been minimized.

Copy link
Member

commented Jan 23, 2019

Looks like you could drop this code:

bitcoin/src/init.cpp

Lines 896 to 900 in 82cf681

#ifndef PROCESS_DEP_ENABLE
// We define this here, because GCCs winbase.h limits this to _WIN32_WINNT >= 0x0601 (Windows 7),
// which is not correct. Can be removed, when GCCs winbase.h is fixed!
#define PROCESS_DEP_ENABLE 0x00000001
#endif

@Empact

This comment has been minimized.

Copy link
Member

commented Jan 23, 2019

Concept ACK

@ken2812221 ken2812221 changed the title [WIP] windows: Set _WIN32_WINNT to 0x0601 (Windows 7) windows: Set _WIN32_WINNT to 0x0601 (Windows 7) Jan 23, 2019

@ken2812221 ken2812221 force-pushed the ken2812221:patch-1 branch to d8a2992 Jan 23, 2019

@laanwj

This comment has been minimized.

Copy link
Member

commented Jan 24, 2019

@theuni can you weigh in here please

Drop defunct Windows compat fixes
"The AI_ADDRCONFIG flag is defined on the Windows SDK for Windows Vista
and later. The AI_ADDRCONFIG flag is supported on Windows Vista and
later."
https://docs.microsoft.com/en-us/windows/desktop/api/ws2tcpip/nf-ws2tcpip-getaddrinfo

However, the version of MinGW we use on Travis is not current and does
not carry the relevant definition, as such I defined it in compat.
https://github.com/wine-mirror/wine/blob/master/include/ws2tcpip.h

Testing confirms that the PROTECTION_LEVEL_UNRESTRICTED,
IPV6_PROTECTION_LEVEL, PROCESS_DEP_ENABLE, AI_ADDRCONFIG, are now
supported by the version of Windows that we test against, so can be
removed.
https://travis-ci.org/bitcoin/bitcoin/jobs/483255439
https://travis-ci.org/Empact/bitcoin/jobs/484123087
@Empact

This comment has been minimized.

Copy link
Member

commented Jan 25, 2019

This commit shows a few more settings that can be removed:
Empact@d0522ec

I used the FAIL_ON_EXTRANEOUS_COMPAT method from #15231 to identify them:
https://travis-ci.org/bitcoin/bitcoin/jobs/483255439
https://travis-ci.org/Empact/bitcoin/jobs/484123087

@Empact

This comment has been minimized.

Copy link
Member

commented Jan 25, 2019

utACK d8a2992

@Empact

This comment has been minimized.

Copy link
Member

commented Jan 25, 2019

re-utACK d0522ec

@theuni

This comment has been minimized.

Copy link
Member

commented Jan 25, 2019

Concept ACK.

Agreed with @laanwj about the depends. I would think that qt would be the most likely to be problematic, but it appears to be handled sanely there:

# Override MinGW's definition in _mingw.h
mingw: DEFINES += WINVER=0x600 _WIN32_WINNT=0x0600

Which made me curious about WINVER. This msdn Article doesn't specifically mention that they need to be the same value, but imo it's implied.

So, I think we should be setting both. Then there's a new snag. Leveldb sets WINVER but not _WIN32_WINNT (I thought this behavior was copied from their buildsystem, but see below):

src/Makefile.leveldb.include:LEVELDB_CPPFLAGS_INT += -DLEVELDB_PLATFORM_WINDOWS -DWINVER=0x0500 -D__USE_MINGW_ANSI_STDIO=1

I tracked that down, and we actually added it ourselves in b102466:

    * Define WINVER=0x0500 so MinGW32 can use the 64-bit-filesystem Windows
      api calls

So it should be safe to bump that to whatever.

I'd suggest:

  • Define WINVER and _WIN32_WINNT together
  • Remove the WINVER define in Makefile.leveldb.include

Edit: Whoops, I see now that @Empact already mentioned WINVER. IMO we should indeed "worry about it", if for no other reason than to save ourselves from hard-to-track-down compile failures in the future.

@ken2812221

This comment has been minimized.

Copy link
Member Author

commented Jan 26, 2019

WINVER has already been defined as _WIN32_WINNT at here since mingw v1.0 released. Do we really have to re-define it explicitly?

@theuni

This comment has been minimized.

Copy link
Member

commented Jan 26, 2019

@ken2812221 Ok, yes, I see that:

/* Choose WINVER Value */
#ifndef WINVER
#ifdef _WIN32_WINNT
#define WINVER      _WIN32_WINNT
#else
#define WINVER      0x0502
#endif
#endif

I guess the msvc headers work similarly? utACK as long as that's the case.

@laanwj laanwj merged commit 0164b0f into bitcoin:master Feb 5, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Feb 5, 2019

Merge #14922: windows: Set _WIN32_WINNT to 0x0601 (Windows 7)
0164b0f build: Remove WINVER pre define in Makefile.leveldb.inlcude (Chun Kuan Lee)
d0522ec Drop defunct Windows compat fixes (Ben Woosley)
d8a2992 windows: Call SetProcessDEPPolicy directly (Chun Kuan Lee)
1bd9ffd windows: Set _WIN32_WINNT to 0x0601 (Windows 7) (Chun Kuan Lee)

Pull request description:

  The current minimum support Windows version is Vista. So set it to 0x0600
  https://github.com/mirror/mingw-w64/blob/5a88def8ad862ef8f4e5f2e69661bfb2d07f1ce2/mingw-w64-headers/include/sdkddkver.h#L19

Tree-SHA512: 38e2afc79426ae547131c8ad3db2e0a7f54a95512f341cfa0c06e4b2fe79521ae67d2795ef96b0192e683e4f1ba6183c010d7b4b8d6b3e68b9bf48c374c59e7d

@ken2812221 ken2812221 deleted the ken2812221:patch-1 branch Feb 5, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.