Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Merged
laanwj merged 4 commits into from
Feb 5, 2019
Merged

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

laanwj merged 4 commits into from
Feb 5, 2019

Conversation

@ken2812221
Copy link
Contributor

@ken2812221 ken2812221 commented Dec 11, 2018 •
edited
Loading

@ken2812221 ken2812221 mentioned this pull request Dec 11, 2018
Closed
@laanwj
Copy link
Member

laanwj commented Dec 11, 2018 •
edited
Loading

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

@ken2812221
Copy link
Contributor Author

ken2812221 commented Dec 11, 2018

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

@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
Copy link
Member

laanwj commented Dec 11, 2018 •
edited
Loading

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
Copy link
Contributor Author

ken2812221 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
Copy link
Contributor

kristapsk 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
Copy link
Member

laanwj 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
Copy link
Contributor

kristapsk commented Dec 11, 2018

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

@kristapsk
Copy link
Contributor

kristapsk commented Dec 12, 2018

But should be mentioned in release notes.

@luke-jr
Copy link
Member

luke-jr 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 Aekanan5906 Dec 12, 2018
@laanwj
Copy link
Member

laanwj commented Dec 12, 2018 •
edited
Loading

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
Copy link
Contributor

DrahtBot commented Dec 12, 2018

Gitian builds for commit 5f23460 (master):

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

@fanquake
Copy link
Member

fanquake 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.

@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
Copy link
Member

fanquake 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
Copy link
Contributor

DrahtBot commented Dec 12, 2018 •
edited
Loading

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

Conflicts

No conflicts as of last run.

@maflcko
Copy link
Member

maflcko 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
Copy link
Contributor Author

ken2812221 commented Dec 12, 2018 •
edited
Loading

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

@fanquake
Copy link
Member

fanquake 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
@laanwj
Copy link
Member

laanwj commented Dec 13, 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.

Not sure, that depends:

  • does any of the dependencies make use of higher _WIN32_WINNT APIs when explicitly defined?

  • does _WIN32_WINNT matter at all if code is not using any of the APIs exposed by the newer windows version? E.g. does it cause use of more efficient APIs for existing things, without code changes?

If either is yes, it might make sense to equalize them. If not, there's no point in doing this for dependencies.

@ken2812221
Copy link
Contributor Author

ken2812221 commented Dec 14, 2018

I found it be defined in many places. So I think it would be better to use -D_WIN32_WINNT=0x0601.

@laanwj
Copy link
Member

laanwj commented Dec 14, 2018

I found it be defined in many places. So I think it would be better to use -D_WIN32_WINNT=0x0601.

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, ...)

@ken2812221
Copy link
Contributor Author

ken2812221 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 patch-1 branch January 2, 2019 01:50
@fanquake
Copy link
Member

fanquake commented Jan 2, 2019

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

@ken2812221 ken2812221 restored the patch-1 branch January 23, 2019 08:22
@ken2812221 ken2812221 reopened this Jan 23, 2019
@ken2812221
windows: Set _WIN32_WINNT to 0x0601 (Windows 7)
1bd9ffd 
Also remove all defines in many places and define it in configure stage to keep consistency.
@ken2812221 ken2812221 mentioned this pull request Jan 23, 2019
Closed
@Empact
Copy link
Contributor

Empact commented Jan 23, 2019

Do we need to worry about WINVER as well?
https://docs.microsoft.com/en-us/cpp/porting/modifying-winver-and-win32-winnt?view=vs-2019

@Empact
Copy link
Contributor

Empact 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
Copy link
Contributor

Empact 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
@laanwj
Copy link
Member

laanwj commented Jan 24, 2019

@theuni can you weigh in here please

@Empact
Drop defunct Windows compat fixes
d0522ec 
"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
Copy link
Contributor

Empact 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
Copy link
Contributor

Empact commented Jan 25, 2019

utACK d8a2992

@Empact
Copy link
Contributor

Empact commented Jan 25, 2019

re-utACK d0522ec

@theuni
Copy link
Member

theuni commented Jan 25, 2019 •
edited
Loading

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
Copy link
Contributor Author

ken2812221 commented Jan 26, 2019 •
edited
Loading

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
Copy link
Member

theuni 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
laanwj added a commit that referenced this pull request Feb 5, 2019
@laanwj
Merge #14922: windows: Set _WIN32_WINNT to 0x0601 (Windows 7)
3a573fd 
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 patch-1 branch February 5, 2019 17:26
jasonbcox pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Oct 2, 2020
@ken2812221 @Fabcien
windows: Set _WIN32_WINNT to 0x0601 (Windows 7)
b969a53 
Summary:
Was set to 0x0501 (XP) which is no longer supported.

Backport of core [[bitcoin/bitcoin#14922 | PR14922]].

Test Plan:
```
cmake -GNinja ..\
  -DCMAKE_TOOLCHAIN_FILE=../cmake/platforms/Win64.cmake \
  -DBUILD_BITCOIN_SEEDER=OFF
ninja
```

Reviewers: #bitcoin_abc, deadalnix

Reviewed By: #bitcoin_abc, deadalnix

Differential Revision: https://reviews.bitcoinabc.org/D7741
@str4d str4d mentioned this pull request Jun 5, 2021
Merged
@jamescowens jamescowens mentioned this pull request Jun 27, 2021
Merged
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

Windows

Projects

None yet

Milestone

0.18.0

Development

Successfully merging this pull request may close these issues.

9 participants

@ken2812221 @laanwj @kristapsk @luke-jr @DrahtBot @fanquake @maflcko @Empact @theuni