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

pull ken2812221 wants to merge 4 commits into bitcoin:master from ken2812221:patch-1 changing 10 files +4 −44
  1. ken2812221 commented at 12:20 PM on December 11, 2018: contributor

    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

  2. fanquake added the label Windows on Dec 11, 2018
  3. laanwj commented at 12:27 PM on December 11, 2018: member

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

  4. ken2812221 force-pushed on Dec 11, 2018
  5. ken2812221 commented at 12:36 PM on December 11, 2018: contributor

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

  6. ken2812221 force-pushed on Dec 11, 2018
  7. ken2812221 renamed this:
    windows: Set_WIN32_WINNT to 0x0601 (Windows 7)
    windows: Set _WIN32_WINNT to 0x0600 (Windows Vista)
    on Dec 11, 2018
  8. laanwj commented at 12:42 PM on December 11, 2018: member

    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?

  9. ken2812221 commented at 12:58 PM on December 11, 2018: contributor

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

  10. MarcoFalke added the label Needs gitian build on Dec 11, 2018
  11. kristapsk commented at 4:26 PM on December 11, 2018: contributor

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

  12. laanwj commented at 5:23 PM on December 11, 2018: member

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

  13. kristapsk commented at 5:39 PM on December 11, 2018: contributor

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

  14. kristapsk commented at 1:27 AM on December 12, 2018: contributor

    But should be mentioned in release notes.

  15. luke-jr commented at 2:40 AM on December 12, 2018: member

    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.

  16. fanquake deleted a comment on Dec 12, 2018
  17. laanwj commented at 8:06 AM on December 12, 2018: member

    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.

  18. DrahtBot commented at 12:16 PM on December 12, 2018: member

    <!--a722867cd34abeea1fadc8d60700f111-->

    Gitian builds for commit 5f23460c7e316fe7c944680f3feff07ebb867f70 (master):

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

  19. DrahtBot removed the label Needs gitian build on Dec 12, 2018
  20. fanquake commented at 1:20 PM on December 12, 2018: member

    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: <img width="809" alt="0 17 0 1 on windows vista" src="https://user-images.githubusercontent.com/863730/49872229-6846a100-fe53-11e8-968c-c9500f80742e.png">

    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.

  21. ken2812221 force-pushed on Dec 12, 2018
  22. fanquake added this to the milestone 0.18.0 on Dec 12, 2018
  23. fanquake renamed this:
    windows: Set _WIN32_WINNT to 0x0600 (Windows Vista)
    windows: Set _WIN32_WINNT to 0x0601 (Windows 7)
    on Dec 12, 2018
  24. fanquake commented at 3:35 PM on December 12, 2018: member

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

  25. DrahtBot commented at 4:08 PM on December 12, 2018: member

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    No conflicts as of last run.

  26. MarcoFalke commented at 4:45 PM on December 12, 2018: member

    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
  27. MarcoFalke added the label Needs gitian build on Dec 12, 2018
  28. ken2812221 commented at 4:48 PM on December 12, 2018: contributor

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

  29. fanquake commented at 5:09 PM on December 12, 2018: member

    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.

  30. ken2812221 renamed this:
    windows: Set _WIN32_WINNT to 0x0601 (Windows 7)
    [WIP] windows: Set _WIN32_WINNT to 0x0601 (Windows 7)
    on Dec 13, 2018
  31. laanwj commented at 1:30 PM on December 13, 2018: member

    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.

  32. ken2812221 force-pushed on Dec 14, 2018
  33. ken2812221 commented at 8:07 AM on December 14, 2018: contributor

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

  34. laanwj commented at 9:17 AM on December 14, 2018: member

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

  35. DrahtBot commented at 12:03 AM on December 15, 2018: member

    <!--a722867cd34abeea1fadc8d60700f111-->

    Gitian builds for commit 9a4334443085970a42792db3528695585fe7053b (master):

    Gitian builds for commit fd1a996c6795374be1eb7ca68da1761ca3a782c2 (master and this pull):

  36. DrahtBot removed the label Needs gitian build on Dec 15, 2018
  37. ken2812221 force-pushed on Dec 17, 2018
  38. ken2812221 commented at 9:37 AM on December 20, 2018: contributor

    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.

  39. ken2812221 force-pushed on Dec 20, 2018
  40. laanwj commented at 2:14 PM on December 20, 2018: member

    @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?

  41. ken2812221 commented at 1:50 AM on January 2, 2019: contributor

    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.

  42. ken2812221 closed this on Jan 2, 2019
  43. ken2812221 deleted the branch on Jan 2, 2019
  44. fanquake commented at 1:59 AM on January 2, 2019: member

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

  45. ken2812221 restored the branch on Jan 23, 2019
  46. ken2812221 reopened this on Jan 23, 2019
  47. windows: Set _WIN32_WINNT to 0x0601 (Windows 7)
    Also remove all defines in many places and define it in configure stage to keep consistency.
    1bd9ffdd44
  48. ken2812221 force-pushed on Jan 23, 2019
  49. Empact commented at 8:50 AM on January 23, 2019: member

    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

  50. Empact commented at 8:53 AM on January 23, 2019: member

    Looks like you could drop this code: https://github.com/bitcoin/bitcoin/blob/82cf6813a4ef1b4a5439eb6cddb1ab426f3c31a2/src/init.cpp#L896-L900

  51. Empact commented at 8:54 AM on January 23, 2019: member

    Concept ACK

  52. windows: Call SetProcessDEPPolicy directly d8a2992067
  53. ken2812221 renamed this:
    [WIP] windows: Set _WIN32_WINNT to 0x0601 (Windows 7)
    windows: Set _WIN32_WINNT to 0x0601 (Windows 7)
    on Jan 23, 2019
  54. ken2812221 force-pushed on Jan 23, 2019
  55. laanwj commented at 2:32 PM on January 24, 2019: member

    @theuni can you weigh in here please

  56. 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
    d0522ec94e
  57. Empact commented at 12:03 AM on January 25, 2019: member

    This commit shows a few more settings that can be removed: https://github.com/Empact/bitcoin/commit/d0522ec94ebbaa564f5f6b31236d4df032664411

    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

  60. theuni commented at 11:41 PM on January 25, 2019: member

    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 b1024662eafddd5560fbfbac29333e5e967ca0f8:

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

  61. build: Remove WINVER pre define in Makefile.leveldb.inlcude 0164b0f5cf
  62. ken2812221 commented at 1:30 AM on January 26, 2019: contributor

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

  63. theuni commented at 5:07 AM on January 26, 2019: member

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

  64. laanwj merged this on Feb 5, 2019
  65. laanwj closed this on Feb 5, 2019
  66. laanwj referenced this in commit 3a573fd46c on Feb 5, 2019
  67. ken2812221 deleted the branch on Feb 5, 2019
  68. jasonbcox referenced this in commit b969a5330c on Oct 2, 2020
  69. zkbot referenced this in commit 372f695d4d on Jun 5, 2021
  70. DrahtBot locked this on Dec 16, 2021
Contributors
ken2812221laanwjkristapskluke-jrDrahtBotfanquakeMarcoFalkeEmpacttheuni
Labels
Windows

Milestone
0.18.0

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-17 03:14 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me