gh-141671: PyMODINIT_FUNC: apply `__declspec(dllexport)` on Windows by encukou · Pull Request #141672 · python/cpython
|
|
||
| #if defined(_WIN32) || defined(__CYGWIN__) | ||
| #if defined(Py_ENABLE_SHARED) | ||
| #if !defined(Py_BUILD_CORE) || defined(Py_ENABLE_SHARED) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not 100% sure about this... don't we need it for our own exports?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only for Py_ENABLE_SHARED -- see #99888 that added the #else.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see we do these checks again everywhere else we use the macros. No problem them.
TBH, I have no view. My knowledge of C is very out of date, so I wouldn't trust anything I might say anyway...
The /EXPORT from setuptools has definitely caused Cython an amount of pain before (although some of that was self-inflicted....). It'd be nice not to have to define PyInit_ just to to make the name exist. Although I suspect we can't really avoid it in a realistic timeline.
(Edit: admittedly we also aren't the people that it's designed to help either)
Here's a version that removes some redundant defines (setting things to defaults).
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. This change mostly moves code around, and just add __declspec(dllexport) on Windows.
|
|
||
| #define PyMODINIT_FUNC _PyINIT_FUNC_DECLSPEC PyObject* | ||
| #define PyMODEXPORT_FUNC _PyINIT_FUNC_DECLSPEC PyModuleDef_Slot* | ||
| #ifndef PyMODINIT_FUNC |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Supporting an already defined PyMODINIT_FUNC/PyMODEXPORT_FUNC is a new feature. Is it really worth it? I'm not against it, just curious.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's an escape hatch: if there's an unexpected problem with this PR, this can allow people to hotfix it without patching or waiting for a new release.
| #if defined(Py_BUILD_CORE) | ||
| #define _PyINIT_EXPORTED_SYMBOL Py_EXPORTED_SYMBOL | ||
| #else | ||
| #define _PyINIT_EXPORTED_SYMBOL __declspec(dllexport) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a better option instead is to define Py_EXPORTED_SYMBOL here and then use that.
Also I feel like an escape hatch like Py_EMBED_MODULE where it basically uses Py_LOCAL_SYMBOL instead could be an option as well for when one does not want the module init function exported due to intending to use PyImport_AppendInitTab within the exe that they directly build the module into.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't want to change the meaning of Py_EXPORTED_SYMBOL.
For the inittab, you can avoid PyMODINIT_FUNC/PyMODEXPORT_FUNC, and just use a normal declaration.
thunder-coding pushed a commit to thunder-coding/cpython that referenced this pull request
