-
Notifications
You must be signed in to change notification settings - Fork 85
Don't automatically add PyInit_<name> to export symbols in Python 3.15+ #395
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
base: main
Are you sure you want to change the base?
Conversation
Since PEP 793, it's valid for modules to not have a PyInit function. Telling MSVC to export a nonexistent function will make it fail. The function should be exported in code, using `PyMODINIT_FUNC` (which adds `__declspec(dllexport)`, which is preferred over `/EXPORT` according to [Microsoft docs].) Microsoft docs: https://learn.microsoft.com/en-us/cpp/build/reference/export-exports-a-function?view=msvc-170
rgommers
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This implementation looks good to me, for now I only have a few very small suggestions for the docstring. I still need to do some testing with a few real-world packages.
For other reviewers: this relies on python/cpython#141672, which was merged only last week.
|
I tested this change with the With the latest released version of After applying the patch in this PR to That repo uses test packages with I then started with I then also added a new Testing on 3.15-alpha is pretty cumbersome given the many packaging components and packages that don't yet support 3.15. That said, I'm confident that this PR works, and the testing I was able to do all worked as expected - so I'm +1 for merging this PR. |
Co-authored-by: Ralf Gommers <ralf.gommers@gmail.com>
|
@encukou, could you do the reformatting, etc. to make CI pass? |
|
The following should resolve most of the unrelated CI failures. |
|
There's also #371. As far as I can see the failures are in code that this PR doesn't touch, due to updated mypy & ruff. |
From my personal experience with maintainers (namely @jaraco ), I don't think that's necessary, but I'm also obviously not talking for them. They prefer to not pin unless absolutely necessary to see new failures as soon as they arrive and to not deal with micro-managing pins on many dozen of projects that could conflict with a shared base (https://github.com/jaraco/skeleton/). In the same vein, whilst annoying, unrelated failures not caused by the current changes are often not blocking. IMO, it could make sense for you to pin in your PR if it helps you ensure you haven't broken stuff. I'll likely end up unpinning in my own PRs and/or a maintainer may end up asking you to remove the pin once they finally get to review 🤷♀️ Up to you really, I'm fairly certain the only thing blocking review of your PR is maintainers' free time. |
That's my impression as well. If there's anything I can help with, let me know. |
Since PEP 793, it's valid for modules to not have a PyInit function. Telling MSVC to export a nonexistent function will make it fail.
The function should be exported in code, using
PyMODINIT_FUNC(which adds__declspec(dllexport), which is preferred over/EXPORTaccording to Microsoft docs.)Fixes: #387