-
-
Notifications
You must be signed in to change notification settings - Fork 31k
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
[PEP 547] bpo-30403: Running extension modules using -m switch #1761
Conversation
@Traceur759, thanks for your PR! By analyzing the history of the files in this pull request, we identified @birkenfeld, @ncoghlan and @benjaminp to be potential reviewers. |
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 our records indicate you have not signed the CLA. For legal reasons we need you to sign this before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue. Thanks again to your contribution and we look forward to looking at it! |
My account has received confirmation of signing CLA, it didn't pass the check because of a delay, it should be okay now. |
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.
Overall the patch looks great - I didn't try to check all the functionality though. Just the one comment on the limited API.
Include/modsupport.h
Outdated
@@ -130,6 +130,11 @@ PyAPI_FUNC(int) PyModule_AddFunctions(PyObject *, PyMethodDef *); | |||
PyAPI_FUNC(int) PyModule_ExecDef(PyObject *module, PyModuleDef *def); | |||
#endif | |||
|
|||
#if !defined(Py_LIMITED_API) || Py_LIMITED_API+0 >= 0x03070000 |
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.
If this should be in the stable API, it needs to be listed in PC/python3.def
as well. If it does not (and I suspect it does not), remove the second half of this condition.
While I'm in favour of merging this, I think we should go through the PEP process first, as:
|
|
||
for (cur_slot = def->m_slots; cur_slot && cur_slot->slot; cur_slot++) { | ||
if (cur_slot->slot == Py_mod_create) { | ||
/* Modules with Py_mod_create cannot be directly executed */ |
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.
Modules with Py_mod_create cannot be directly executed
Why is that?
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.
The interpreter itself creates __main__
during startup, so it needs to be able to hand the loader that pre-existing module rather than asking the loader to create a new one. If we don't do that, then we end up with subtle problems like -i
and PYTHONINTERACTIVE=1
not working as expected.
That does raise an interesting difference with runpy.run_module
though: that can ask the loader to create its own module object, so it should be able to handle arbitrary extension modules, even those that define Py_mod_create
.
If the restriction poses a potential problem for Cython, then it would make sense to look more closely at the feasibility of just lifting the restriction against replacing __main__
, and instead improve the post-mortem introspection code to better handle the case where sys.modules["__main__"]
isn't the main module the interpreter originally created.
Extension and built-in modules that support multi-phase initialization can now be executed this way.
@ncoghlan wrote:
True. However, in this case, Python should rather advertise that
Cython is not ready for multi-phase initialization yet. It keeps global state in C-level static variables. See discussion at cython/cython#1923. Given that Cython, as the most important use case and the only explicit one, is not ready, we'll probably need to defer the PEP for now. |
So what's the state of this PR (other than the merge conflicts)? |
The related PEP is deferred until Cython has gained multi-phase initialization support (since it doesn't make sense to do this in a way that Cython can't use, and we can't properly assess Cython's needs until it's reached the point of being able to use multi-phase init in the first place). So I'm going to close this for now, and a new PR can be created once that situation changes. |
This allows using the -m switch to run C extension and built-in modules that
support PEP 489 multi-phase initialization.
To do this this, I had to do following changes:
Lib/runpy.py
into another function _get_code.
optional loader method (described below) running the extension module inside the main namespace.
Lib/importlib_bootstrap_external.py
I added a new loader method, exec_module_in_namespace, to ExtensionFileLoader. This takes a spec and a module,
and initializes the module based on the spec. The module's metadata (name, loader, etc.) are ignored.
Python/importdl.c
I split _PyImport_LoadDynamicModuleWithSpec into two functions, one has the old name and always returns a module.
The second, PyImport_LoadDynamicModuleDef, is called by the first one and returns what the PyInit* function returns:
either a fully initialized module (for single-phase init) or PyModuleDef (for multi-phase init).
Python/import.c
I added a new function, exec_in_module. This takes spec and a module as arguments,
then loads a module def using new function _PyImport_LoadDynamicModuleDef,
and uses the def to initialize the module using PyModule_ExecInModule.
Objects/moduleobject.c
I added a new function, PyModule_ExecInModule, that receives module and def arguments, then it initializes the module based on the def.
https://bugs.python.org/issue30403