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: Implement socket/ssl modules #12810

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

stinos
Copy link
Contributor

@stinos stinos commented Oct 26, 2023

Socket module is enabled for mingw and msvc builds, mbedtls only for the former; for no particular reason, I can add it there as well if that makes more sense.

As for testing I manually ran the relevant multi_net/net_inet/net_hosted tests and tried the requests module both with and without SSL, and all of that works fine so it's likely that everything is done correctly.

The new multi_net/tcp_recv_timeout.py fails sometimes on ci, likely because one of the recv() calls errors out because the remote disconnected already, which I thought multitest.next() would make sure didn't happen but I probably misunderstood what it does. Someone has an idea for that? I just want the recv calls on both ends to time out.

The test fixes with the numeric values I'm not super happy about, but the only proper way of doing this is to provide common functions to check these errors in a test_helpers module or something like that, and then perhaps implementing a complete errno module for windows which does include all WSAXXX error code, which is a bit much maybe?

@codecov
Copy link

codecov bot commented Oct 26, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.39%. Comparing base (51d05c4) to head (0aae1b9).

❗ Current head 0aae1b9 differs from pull request most recent head dc44e7c. Consider uploading reports for the commit dc44e7c to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #12810   +/-   ##
=======================================
  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.

@projectgus
Copy link
Contributor

This is an automated heads-up that we've just merged a Pull Request
that removes the STATIC macro from MicroPython's C API.

See #13763

A search suggests this PR might apply the STATIC macro to some C code. If it
does, then next time you rebase the PR (or merge from master) then you should
please replace all the STATIC keywords with static.

Although this is an automated message, feel free to @-reply to me directly if
you have any questions about this.

Note these are mostly the 'bare' socket tests, not the ssl/tls ones
for instance: most of these don't run on CPython because of
incompatible wrap_socket() arguments.
The change mostly consists of checking the WSA error codes next to
the errno ones and these are written as numeric values because the
names (like WSAEAGAIN) are only available in CPython and not in
micropython.

Signed-off-by: stijn <stijn@ignitron.net>
Signed-off-by: stijn <stijn@ignitron.net>
Upcoming commits are going to port this file to work with sockets
on windows so prepare for that with the platform-specific bits.

Signed-off-by: stijn <stijn@ignitron.net>
Signed-off-by: stijn <stijn@ignitron.net>
Copy link

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

Signed-off-by: stijn <stijn@ignitron.net>
@stinos stinos force-pushed the winsock branch 2 times, most recently from 7304306 to f1f67ef Compare March 25, 2024 13:02
This is more consistent with the Makefile/CMake-based builds.

Signed-off-by: stijn <stijn@ignitron.net>
This is consistent with extmod.mk's split between SRC_EXTMOD_C and
SRC_THIRDPARTY_C, the latter specifying source files which get
compiled but not included in qstr generation.

Signed-off-by: stijn <stijn@ignitron.net>
Supports preprocessor definitions like /Dval="quotedvalue" by turning
that into /Dval=\"quotedvalue\".

Signed-off-by: stijn <stijn@ignitron.net>
Signed-off-by: stijn <stijn@ignitron.net>
@stinos
Copy link
Contributor Author

stinos commented Mar 25, 2024

Apart from the STATIC changes, this went rather stale all recent mbedtls changes as well, and the switch from Appveyor to Github Actions; I updated everything now. The 2 mingw standard variant builds currently fail, but there's something weird going on with how Github Actions runs these builds and I'm still looking into that.

@stinos
Copy link
Contributor Author

stinos commented Mar 26, 2024

The 2 mingw standard variant builds currently fail, but there's something weird going on with how Github Actions runs these builds and I'm still looking into that.

Fix in #14188

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants