-
-
Notifications
You must be signed in to change notification settings - Fork 29.9k
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
bpo-35642: Remove asynciomodule.c from pythoncore.vcxproj #11410
Conversation
Hello, and thanks for your contribution! I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA). Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA (this might be simply due to a missing "GitHub Name" entry in your b.p.o account settings). This is necessary for legal reasons before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue. You can check yourself to see if the CLA has been received. Thanks again for your contribution, we look forward to reviewing it! |
I have signed the CLA. I'm not sure what to do about the missing NEWS.d entry. I'm assuming |
Thanks for all the feedback. I'm not familiar with the windows environment myself. Perhaps you can file an issue in devguide to suggest improvements related to running Regarding the news file, what the bot is looking for is a file in the Misc.d/News in the PR.
|
Before this patch, asynciomodule.c was compiled by both the pythoncore and _asyncio projects. asynciomodule.c defines the _asyncio extension module. The PC\config.c does not reference the _asyncio module. And the official CPython distributions today are shipping _asyncio as a standalone extension module (_asyncio.pyd) - not a built-in as part of libpythonXY.dll. I think the presence of asynciomodule.c in the pythoncore project is incorrect. Using dumpbin.exe, the distributed python37.dll does contain some symbols from asynciomodule.c. I'm not an expert on Windows loader semantics, so I'm not sure whether the symbols from pythonXY.dll or _asyncio.pyd will be used when _asyncio.pyd is loaded. Whatever the case, having symbols provided by 2 binaries is probably not a good idea. This commit removes asynciomodule.c from the pythoncore project and reinforces that _asyncio is a standalone extension module and not a built-in. Because public symbols (at least PyInit__asyncio) are being dropped from pythonXY.dll, this change is backwards incompatible at the API layer. There is a possibility someone, somewhere is relying on PyInit__asyncio being exported from pythonXY.dll. So caution may be warranted before backporting.
0c02ec5
to
7fda263
Compare
Thank you for the pointer to |
Good catch, thanks! |
@@ -0,0 +1 @@ | |||
Remove asynciomodule.c from pythoncore.vcxproj |
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.
please mention somehow that it's already built by "_asyncio.vcxproj (providing _asyncio.pyd)".
nitpick: the commit message is too long. IMHO such long rationale belongs better to the issue https://bugs.python.org/issue35642 There is no need to explain everything in the commit message for such simple change. |
My work on the Firefox and Mercurial projects has groomed me to include a summary of changes and their rationale in the commit message because - unlike links to bugs/issues - the commit message can be accessed without an Internet connection and more importantly doesn't require the reader to peruse possibly dozens of updates/comments to glean knowledge: they just have to look in one place (the commit message) to achieve understanding. This approach facilitates easier code archeology and from my experience helps complex projects scale. So that's why I did what I did. But if this isn't Python's commit message style, I can reword the message accordingly. That being said, since we're talking about nits to the commit message and news blurb and I haven't established a baseline understanding of this project's standards, it might be more efficient for a maintainer to simply wordsmith my commit to something adequate so we can skip extra review rounds. I'm assuming Python's review/landing workflow supports accommodating contributors in this way and I won't be bothered/offended if this is done on this contribution or any of my future ones. |
We can rewrite the commits when we merge, so I'll do it. Thanks for the fix! |
I'm assuming the CLA is incoming, but there was nothing novel about the fix so it hardly matters. |
@indygreg You have indeed signed the CLA, but because you haven't linked your GitHub name on https://bugs.python.org it doesn't reflect it here. If you visit https://bugs.python.org/user20674 then you should be able to add your GitHub name to the field there. It will make future contributions much easier. |
I have associated my GitHub username with bugs.python.org. Thanks for the pointer! |
…1410) This module is built by _asyncio.vcxproj and does not need to be included in pythoncore. (cherry picked from commit fbf5068) Co-authored-by: Gregory Szorc <[email protected]>
GH-11747 is a backport of this pull request to the 3.7 branch. |
This module is built by _asyncio.vcxproj and does not need to be included in pythoncore. (cherry picked from commit fbf5068) Co-authored-by: Gregory Szorc <[email protected]>
Before this patch, asynciomodule.c was compiled by both the
pythoncore and _asyncio projects.
asynciomodule.c defines the _asyncio extension module.
The PC\config.c does not reference the _asyncio module. And the
official CPython distributions today are shipping _asyncio as a
standalone extension module (_asyncio.pyd) - not a built-in
as part of libpythonXY.dll.
I think the presence of asynciomodule.c in the pythoncore project
is incorrect.
Using dumpbin.exe, the distributed python37.dll does contain
some symbols from asynciomodule.c. I'm not an expert on Windows
loader semantics, so I'm not sure whether the symbols from
pythonXY.dll or _asyncio.pyd will be used when _asyncio.pyd is
loaded. Whatever the case, having symbols provided by 2 binaries
is probably not a good idea.
This commit removes asynciomodule.c from the pythoncore project
and reinforces that _asyncio is a standalone extension module and
not a built-in.
Because public symbols (at least PyInit__asyncio) are being dropped
from pythonXY.dll, this change is backwards incompatible at the API
layer. There is a possibility someone, somewhere is relying on
PyInit__asyncio being exported from pythonXY.dll. So caution may
be warranted before backporting.
https://bugs.python.org/issue35642