Not quite sure why, when I build from Visual Studio, I'm not getting any of the errors that the buildbots are reporting. I clearly need to investigate the MicroPython build system some more, and figure this out, before this can be merged.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.39%. Comparing base (35b2edf) to head (bcea6c6).

@@           Coverage Diff           @@
##           master   #14162   +/-   ##
=======================================
  Coverage   98.39%   98.39%           
=======================================
  Files         161      161           
  Lines       21200    21200           
=======================================
  Hits        20860    20860           
  Misses        340      340           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

OK, it seems to compile now, but there are test failures.

The Linux cross compile build seems to be using a very old version of the MS SDK. Some of the identifiers that were introduced in Windows Vista were not being defined by default by the headers there. I fixed that by defining the target Windows version macros before including windows.h. I've included that as a separate commit, since it's a separable thing.

I've also broken out the initialisation of WinSock2 into a separate commit. The initialisation is hopefully uncontroversial, and hopefully right.

Code size report:

   bare-arm:    +0 +0.000% 
minimal x86:    +0 +0.000% 
   unix x64:    +0 +0.000% standard
      stm32:    +0 +0.000% PYBV10
     mimxrt:    +0 +0.000% TEENSY40
        rp2:    +0 +0.000% RPI_PICO
       samd:    +0 +0.000% ADAFRUIT_ITSYBITSY_M4_EXPRESS

Did you see #12810 ?

Test failures are partly due to tests assuming unix/posix socket error codes.

@stinos : No, I hadn't seen #12810, thanks for pointing that out!

It looks like you've gotten far further than me in getting sockets to work, so I'll stop working on this PR.

I would suggest you pull in my first two commits. Consistent definition of target Windows version., and Initialise WinSock2 library at startup..

Good luck getting sockets support landed!

Consistent definition of target Windows version.

I don't seem to be needing WIN32_LEAN_AND_MEAN for winsock2 though? On the other hand I only defined _WIN32_WINNT for mingw builds so maybe there's some interaction between the macros there. If you think this is a worthy addition you could create a PR for it, but then the explanation in that windows_version.h should probably clarify exactly what is needed and why, seeing that everything except sockets for mingw builds seems to work fine without it.

Initialise WinSock2 library at startup..

Yeah I had the initialization in modcocket.c itself but indeed it's probably better to have it it in init.c so I changed that in my PR (plus guarded by MICROPY_PY_SOCKET and added the WSACleanup counterpart as well)

Thanks for taking the init changes.

Regarding the other change... OK, here's the long and complicated explanation of the problems, followed by the simple solution. I may or may not end up making a PR for this - I'm waiting until the sockets PR and my open ntptime PRs get accepted, before I make any more MicroPython PRs. But hopefully this helps explain why you need it.

WIN32_LEAN_AND_MEAN

Regarding WIN32_LEAN_AND_MEAN, see https://learn.microsoft.com/en-us/windows/win32/winsock/creating-a-basic-winsock-application . From that page:

If an #include line is needed for the Windows.h header file, this should be preceded with the #define WIN32_LEAN_AND_MEAN macro.

In other words, this file doesn't compile:

#include <windows.h>
#include <winsock2.h>

But this does compile:

#define WIN32_LEAN_AND_MEAN
#include <windows.h>
#include <winsock2.h>

And this does compile, too:

#include <winsock2.h>

Since programmers routinely add new headers to code they're working on, this problem can pop up with a small change that leads to adding either <windows.h> or <winsock2.h> to a file that already contained the other one - either directly or via a chain of includes.

The thing is, this is all very weird and subtle and complicated. And it fails with weird compile errors, making it awkward to debug unless you know about this problem already. It creates a barrier to entry for people wanting to contribute. So we should do what we can to simplify it.

The only sane approach is to make sure WIN32_LEAN_AND_MEAN is defined, in every place that windows.h is included (whether including it directly or indirectly).

_WIN32_WINNT

The MicroPython Windows port has a minimum version of Windows that it supports. For example, it could be that MicroPython supports Windows 8 and later, or Windows 10 and later.

Note that even if MicroPython doesn't make a formal decision, there will still be a minimum version of Windows that it supports, it's just that you might not know what that version is, and it might not be documented anywhere, and it might change when someone adds a new MicroPython feature using newer Windows features. So it's better to make a formal decision. Even if you're not sure, I suggest you pick something. You can always change that decision later.

I recommend supporting Windows 8 as the minimum, for now. Windows 10 would also be a reasonable choice, but I don't think there are any Windows-10-only APIs that you want to use right now. Older versions of Windows are ancient, no longer supported, and not in common use among the group of developers who might want to run MicroPython on Windows.

Once you've made a decision, the Windows headers are designed to help you enforce it. By defining _WIN32_WINNT to the minimum version of Windows that you support, the Windows headers will only declare functions and constants that will work on that version of Windows. Newer functions and constants will just not be declared, so will cause a compile error.

If you don't define _WIN32_WINNT, you will get some default version of Windows. This Windows version varies depending on what version of the Windows headers you're compiling against. For a practical example, some of the getaddrinfo constants used by MicroPython were introduced in Windows Vista. My version of the Windows headers, from whatever Visual Studio 2019 release I happen to be using, has a new enough default version, so you don't need to define _WIN32_WINNT to use them. The version of the Windows headers used by the Linux cross-compile-to-Windows buildbot has a much older default version, so you can't rely on that default, you do need to define _WIN32_WINNT to use the getaddrinfo constants.

Basically, relying on the default for _WIN32_WINNT leads to a complicated mess of compile errors depending on the version of the Windows SDK you happen to have and the features used by a file. It also means you have no idea what version of Windows you support. It is possible to make this work, with enough work and enough care. But it's a really bad idea, and it takes programmer effort that could better be spent on writing new code.

Defining _WIN32_WINNT differently in different files, also leads to a different complicated mess of problems. These problems also arise if you use the default in some files, and explicitly define it to something else in other files.

The first problem is that certain Windows structures, e.g. MENUITEMINFOW, are defined differently based on the value of _WIN32_WINNT. Because newer versions of Windows add a new entry at the end of that structure. So now you can have one file allocate the smaller version of that structure, pass it to another file that accesses the structure, and you have a write-past-allocated-memory bug that is invisible in the source code and will be a nightmare to find, debug and fix. Even the possibility of that means that you have to either cross your fingers and hope, or invest a huge amount of programmer effort in checking every structure you use.

The second problem is it messes up IDEs and debuggers. There's no sane way for the debugger to show that a bit of memory containing a struct is interpreted one way in some files, and a different way in other files. Because no-one deliberately defines structures with different definitions in different files, this is something that debugger and IDE vendors haven't spent any effort on solving.

The third problem is it makes refactoring harder, and copying existing code harder. You can't just move or copy code between files, you have to check the _WIN32_WINNT setting on both files and maybe increase the value on the new file.

The fourth problem is that if you define _WIN32_WINNT as higher than the minimum version of Windows that MicroPython supports, then you run a risk of accidentally writing code that won't run on that minimum version of Windows.

The fifth problem is that there's really no point defining _WIN32_WINNT as lower than the minimum version of Windows that MicroPython supports. It doesn't do anything useful. But it will confuse programmers who will waste time trying to understand why you're doing that thing, and is there something really subtle there that they are missing.

Again, having different versions of _WIN32_WINNT in different places is possible, with enough work and enough care. But it's a really bad idea, and it takes programmer effort that could better be spent on writing new code.

Again, this is all very weird and subtle and complicated. And at best it fails with weird compile errors, making it awkward to debug unless you know about this problem already. At worst it causes incredibly hard-to-find bugs. Either way it creates a barrier to entry for people wanting to contribute. So we should do what we can to simplify it.

The only sane approach is to make sure _WIN32_WINNT is defined to a single, fixed value, in every place that windows.h is included (whether including it directly or indirectly). That value will be the minimum version of Windows that MicroPython supports.

A simple solution

The problems described above are all very weird and subtle and complicated and hard to debug. They create a barrier to entry for people wanting to contribute. So:

I suggest a simple coding style rule that contributors to the Windows port would have to know and follow:

• If you are going to #include <windows.h> or other Windows-specific header files, always do #include "ports/windows/windows_version.h" first. (Ideally as the very first include in your .c file).

Then windows_version.h can #define WIN32_LEAN_AND_MEAN and _WIN32_WINNT, and then everything works. All the problems described above, just go away.

As a sanity check, windows_version.h could also contain, at the start:

#ifdef _WIN32_WINNT
#error "windows_version.h included too late.  It must be included BEFORE any Windows-specific header files."
#endif

Note that contributors don't have to understand why that coding style rule exists. We should certainly make documentation - which could be based on this comment - available for those interested in the gory details.

But so long as people follow that simple rule, none of the problems described here will occur.

Ok thanks for the elaborate explanation - all clear. Just a couple of the relevant paragraphs are imo sufficient to understand why things are needed and could make it into the windows_version.h file, like this bit

If an #include line is needed for the Windows.h header file, this should be preceded with the #define WIN32_LEAN_AND_MEAN macro.

In other words, this file doesn't compile:

#include <windows.h>
#include <winsock2.h>

But this does compile:

#define WIN32_LEAN_AND_MEAN
#include <windows.h>
#include <winsock2.h>

And this does compile, too:

#include <winsock2.h>

and also

Basically, relying on the default for _WIN32_WINNT leads to a complicated mess of compile errors depending on the version of the Windows SDK you happen to have and the features used by a file. It also means you have no idea what version of Windows you support.

Again, having different versions of _WIN32_WINNT in different places is possible, with enough work and enough care. But it's a really bad idea, and it takes programmer effort that could better be spent on writing new code.

The only sane approach is to make sure _WIN32_WINNT is defined to a single, fixed value, in every place that windows.h is included (whether including it directly or indirectly). That value will be the minimum version of Windows that MicroPython supports.

The only annoying thing left is that mingw might treat things in a different way, but then there's probably a good explanation for that as well.

Then wrt

If you are going to #include <windows.h> or other Windows-specific header files, always do #include "ports/windows/windows_version.h" first.

that could als be "If you are need #include <windows.h> instead use #include "ports/windows/windows.h", likewise include that file first before any other windows-specific headers" and ports/windows/windows.h would be like

#define _WIN32_WINNT ...
#define WIN32_LEAN_AND_MEAN
#include <windows.h>

Advantage being there's only one go-to include which is going to cover the majority of cases in the micropython codebase.

The only sane approach is to make sure _WIN32_WINNT is defined to a single, fixed value, in every place that windows.h is included (whether including it directly or indirectly).

We'll just need to doublecheck none of the external code also defines it then.