Skip to content
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

Closed
wants to merge 8 commits into from
Closed

[PEP 547] bpo-30403: Running extension modules using -m switch #1761

wants to merge 8 commits into from

Conversation

ghost
Copy link

@ghost ghost commented May 23, 2017

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

  1. I split _get_module_details into same named function and seperated the part acquiring module code
    into another function _get_code.
  2. Inside of _run_module_as_main, I added few lines of code calling a new
    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

@mention-bot
Copy link

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

@the-knights-who-say-ni
Copy link

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!

@ghost
Copy link
Author

ghost commented May 24, 2017

My account has received confirmation of signing CLA, it didn't pass the check because of a delay, it should be okay now.

Copy link
Member

@zooba zooba left a 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.

@@ -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
Copy link
Member

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.

@ncoghlan ncoghlan changed the title bpo-30403: Running extension modules using -m switch [PEP required] bpo-30403: Running extension modules using -m switch May 25, 2017
@ncoghlan
Copy link
Contributor

While I'm in favour of merging this, I think we should go through the PEP process first, as:

  1. We've been burned before by not properly advertising changes to Python's command line behaviour (while zip archive execution was added back in 2.6, a lot of folks didn't learn about it until the addition of the better advertised zipapp utility module in 3.5)
  2. We should discuss the approach with the Cython developers and make sure that they're willing and able to support it when targeting 3.7+ before merging the implementation

@ncoghlan ncoghlan changed the title [PEP required] bpo-30403: Running extension modules using -m switch [PEP 547] bpo-30403: Running extension modules using -m switch Jun 3, 2017

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 */
Copy link
Contributor

@scoder scoder Jul 22, 2017

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?

Copy link
Contributor

@ncoghlan ncoghlan Jul 24, 2017

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.

@ghost ghost self-requested a review September 5, 2017 13:17
@encukou
Copy link
Member

encukou commented Oct 13, 2017

@ncoghlan wrote:

While I'm in favour of merging this, I think we should go through the PEP process first, as:

  1. We've been burned before by not properly advertising changes to Python's command line behaviour (while zip archive execution was added back in 2.6, a lot of folks didn't learn about it until the addition of the better advertised zipapp utility module in 3.5)

True. However, in this case, Python should rather advertise that -m is now available for extension modules. Most extension module authors will need to add support for -m if it is to work.

  1. We should discuss the approach with the Cython developers and make sure that they're willing and able to support it when targeting 3.7+ before merging the implementation

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.

@brettcannon
Copy link
Member

So what's the state of this PR (other than the merge conflicts)?

@ncoghlan
Copy link
Contributor

ncoghlan commented Jun 9, 2018

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.

@ncoghlan ncoghlan closed this Jun 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants