bpo-35642: Remove asynciomodule.c from pythoncore.vcxproj #11410

indygreg · 2019-01-02T18:08:07Z

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

the-knights-who-say-ni · 2019-01-02T18:08:09Z

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!

indygreg · 2019-01-02T18:21:37Z

I have signed the CLA.

I'm not sure what to do about the missing NEWS.d entry. I'm assuming make patchcheck will give me instructions. However, this is my first CPython contribution, I'm developing on Windows, and the development guide at https://devguide.python.org/ doesn't seem to have usable instructions for running make patchcheck on Windows. For starters, it doesn't tell me how to install make. (I have it installed so no major problem). More critically, the build instructions for Windows don't transform Makefile.pre.in to Makefile to provide a usable make target! But I read the Makefile.pre.in file to find what the make target does and ran the Tools/scripts/patchcheck.py script manually. It simply printed Misc/NEWS.d updated with blurb ... NO which isn't very actionable. I clicked on details of the bedevere/news check and it seemed to want me to install some Windows application? (I'd prefer not to do that.) I'm confused about what I need to do to make the bots happy. Is there not some in-repo script I can run to generate the NEWS.d entry?

Mariatta · 2019-01-02T19:19:15Z

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 make patchcheck on Windows.

Regarding the news file, what the bot is looking for is a file in the Misc.d/News in the PR.
There are two ways to generate the misc.d/news file:

Using blurb, which is a command line tool that you need to install. It will generate the file, and you will need to commit and push it to the PR.
Using blurb_it, which is just a web page, so you don't have to install anything on your computer. Everything happens in the browser. You will be presented with a web form. There is some more information on how blurb_it works here. I've also created an issue Add a page explaining how to use blurb_it blurb_it#12.

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.

indygreg · 2019-01-03T01:04:54Z

Thank you for the pointer to blurb! I was able to generate a NEWS.d entry and update the PR without much issue. The robot overlords seem to be happy now, which is usually a good sign.

asvetlov · 2019-01-03T09:32:03Z

Good catch, thanks!

vstinner · 2019-01-04T22:47:47Z

Misc/NEWS.d/next/Build/2019-01-02-11-23-33.bpo-35642.pjkhJe.rst

@@ -0,0 +1 @@
+Remove asynciomodule.c from pythoncore.vcxproj


please mention somehow that it's already built by "_asyncio.vcxproj (providing _asyncio.pyd)".

vstinner · 2019-01-04T22:49:21Z

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.

indygreg · 2019-01-07T18:40:43Z

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.

zooba · 2019-01-08T01:26:18Z

We can rewrite the commits when we merge, so I'll do it. Thanks for the fix!

zooba · 2019-01-08T01:27:53Z

I'm assuming the CLA is incoming, but there was nothing novel about the fix so it hardly matters.

zooba · 2019-01-08T01:59:11Z

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

indygreg · 2019-01-09T16:49:28Z

I have associated my GitHub username with bugs.python.org. Thanks for the pointer!

miss-islington · 2019-02-03T00:27:19Z

Thanks @indygreg for the PR, and @zooba for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖

…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 <gregory.szorc@gmail.com>

bedevere-bot · 2019-02-03T00:27:27Z

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 <gregory.szorc@gmail.com>

indygreg requested a review from a team as a code owner January 2, 2019 18:08

the-knights-who-say-ni added the CLA not signed label Jan 2, 2019

bedevere-bot added the awaiting review label Jan 2, 2019

indygreg force-pushed the remove-duplicate-asynciomodule branch from 0c02ec5 to 7fda263 Compare January 2, 2019 19:24

asvetlov approved these changes Jan 3, 2019

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting review labels Jan 3, 2019

asvetlov added 🤖 automerge and removed CLA not signed labels Jan 3, 2019

the-knights-who-say-ni added the CLA not signed label Jan 3, 2019

asvetlov self-assigned this Jan 3, 2019

vstinner reviewed Jan 4, 2019

View reviewed changes

vstinner requested review from 1st1, zware and zooba January 4, 2019 22:48

asvetlov removed the CLA not signed label Jan 5, 2019

the-knights-who-say-ni added the CLA not signed label Jan 5, 2019

asvetlov removed the 🤖 automerge label Jan 5, 2019

zooba merged commit fbf5068 into python:master Jan 8, 2019

bedevere-bot removed the awaiting merge label Jan 8, 2019

zooba removed the CLA not signed label Jan 8, 2019

the-knights-who-say-ni added the CLA not signed label Jan 8, 2019

zooba added the needs backport to 3.7 label Feb 3, 2019

bedevere-bot removed the needs backport to 3.7 label Feb 3, 2019

Uh oh!

bpo-35642: Remove asynciomodule.c from pythoncore.vcxproj #11410

bpo-35642: Remove asynciomodule.c from pythoncore.vcxproj #11410

Uh oh!

indygreg commented Jan 2, 2019 •

edited by bedevere-bot

Loading

Uh oh!

the-knights-who-say-ni commented Jan 2, 2019

Uh oh!

indygreg commented Jan 2, 2019

Uh oh!

Mariatta commented Jan 2, 2019

Uh oh!

indygreg commented Jan 3, 2019

Uh oh!

asvetlov commented Jan 3, 2019

Uh oh!

vstinner Jan 4, 2019

Uh oh!

vstinner commented Jan 4, 2019

Uh oh!

indygreg commented Jan 7, 2019

Uh oh!

zooba commented Jan 8, 2019

Uh oh!

zooba commented Jan 8, 2019

Uh oh!

zooba commented Jan 8, 2019

Uh oh!

indygreg commented Jan 9, 2019

Uh oh!

miss-islington commented Feb 3, 2019

Uh oh!

bedevere-bot commented Feb 3, 2019

Uh oh!

Uh oh!

Uh oh!

bpo-35642: Remove asynciomodule.c from pythoncore.vcxproj #11410

bpo-35642: Remove asynciomodule.c from pythoncore.vcxproj #11410

Uh oh!

Conversation

indygreg commented Jan 2, 2019 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

the-knights-who-say-ni commented Jan 2, 2019

Uh oh!

indygreg commented Jan 2, 2019

Uh oh!

Mariatta commented Jan 2, 2019

Uh oh!

indygreg commented Jan 3, 2019

Uh oh!

asvetlov commented Jan 3, 2019

Uh oh!

vstinner Jan 4, 2019

Choose a reason for hiding this comment

Uh oh!

vstinner commented Jan 4, 2019

Uh oh!

indygreg commented Jan 7, 2019

Uh oh!

zooba commented Jan 8, 2019

Uh oh!

zooba commented Jan 8, 2019

Uh oh!

zooba commented Jan 8, 2019

Uh oh!

indygreg commented Jan 9, 2019

Uh oh!

miss-islington commented Feb 3, 2019

Uh oh!

bedevere-bot commented Feb 3, 2019

Uh oh!

Uh oh!

indygreg commented Jan 2, 2019 •

edited by bedevere-bot

Loading