gh-152433: Windows: _ssl module: improve UWP compatibility#152791
gh-152433: Windows: _ssl module: improve UWP compatibility#152791thexai wants to merge 1 commit into
_ssl module: improve UWP compatibility#152791Conversation
picnixz
left a comment
picnixz
left a comment
There was a problem hiding this comment.
- Please use
if defined()andelif defined(MS_WINDOWS) && ...andelsefor the fallback branch. - Can you determine why we are only raising a NotImplementedError on MS DEBUG builds for those functions? it's a separate issue but I think we should also raise unconditionally (or does it mean that those functions are available on regular MS builds only in release builds?) If there is something worth investigating, you can open a separate issue for that.
| #ifdef MS_WINDOWS_APP_PURE | ||
| PyErr_SetString(PyExc_NotImplementedError, | ||
| "set_keylog_filename: unavailable on UWP build"); | ||
| return -1; | ||
| #else |
There was a problem hiding this comment.
If this large if-else-endif guard is necessary because some symbols are not available on UWP, I'm ok. Otherwise, I would prefer that you simply add an if guard afterwards. It would be dead code in some sense.
I'm actually surprised that we raise NotImplementedError only on MS DEBUG builds though.
There was a problem hiding this comment.
IIRC, that may be because the API passes FILE * across the boundary, and because we always use a release build of OpenSSL even for debug builds of CPython. So it'll (potentially) crash (and will certainly crash if anyone uses fileno on it) becuase the FILE data is in a different instance of the runtime.
Skipping the entire function made more sense than ~doubling the debug build time or redistributing a debug build of libssl.
There was a problem hiding this comment.
Yes, is because some OpenSSL symbols not available on UWP.
e.g. BIO_new_fp and PEM_read_DHparams (basically direct file I/O manipulation because UWP requires very specific Win API methods) and not worth patch OpenSSL only for this.
Anyway changed to #if defined(...) && style and removed changes in pyconfig.h
There was a problem hiding this comment.
Actually, the MS_WINDOWS_APP_PURE was ok. I just wondered whether it was also ok to use if-elif-endif instead of if/else + another if inside the else due to the debug branch of MS_WINDOWS:
#if defined(your_thing)
unconditional return
#elif MS_WINDOWS + Py_NDEBUG
unconditional return
#else
the linux + release MS branch
#endifrather than
#if defined(your_thing)
unconditional return
#else
#if MS_WINDOWS + Py_NDEBUG
unconditional return
#endif
the linux + release MS branch
#endif| PyErr_SetString(PyExc_NotImplementedError, "load_dh_params: unavailable on UWP build"); | ||
| return NULL; | ||
| #else | ||
| #if defined(MS_WINDOWS) && defined(Py_DEBUG) |
There was a problem hiding this comment.
The fact that we raise only on DEBUG MS builds is really weird here. Likewise, I would rather prefer an elif branch instead of a ifdef/else + nested.
3 participants
Windows:
_sslmodule: improve UWP compatibility.