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