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

gh-117953: Split Up _PyImport_LoadDynamicModuleWithSpec() #118203

Conversation

ericsnowcurrently
Copy link
Member

@ericsnowcurrently ericsnowcurrently commented Apr 23, 2024

I've pulled this out of #118116.

Basically, I've turned most of _PyImport_LoadDynamicModuleWithSpec() into two new functions (_PyImport_GetModInitFunc() and _PyImport_RunModInitFunc()) and moved the rest of it out into _imp_create_dynamic_impl(). There shouldn't be any changes in behavior.

This change makes some future changes simpler. This is particularly relevant to potentially calling each module init function in the main interpreter first. Thus the critical part of the PR is the addition of _PyImport_RunModInitFunc(), which is strictly focused on running the init func and validating the result. A later PR will take it a step farther by capturing error information rather than raising exceptions.

FWIW, this change also helps readers by clarifying a bit more about what happens when an extension/builtin module is imported.

@ericsnowcurrently ericsnowcurrently force-pushed the split-up-_PyImport_LoadDynamicModuleWithSpec-2 branch from 378778e to 52f0656 Compare April 24, 2024 19:59
@ericsnowcurrently ericsnowcurrently marked this pull request as ready for review April 24, 2024 19:59
@ericsnowcurrently ericsnowcurrently requested review from encukou and removed request for brettcannon, warsaw, ncoghlan and kumaraditya303 April 24, 2024 19:59
@ericsnowcurrently
Copy link
Member Author

@encukou, if you have a few minutes, I'm mostly looking for a sanity check on this PR.

@encukou
Copy link
Member

encukou commented Apr 26, 2024

This PR looks good, but see my comment here: #118193 (comment)

An indirect test of a multiphase module with m_slots=NULL triggers the //assert that's commented out here.

@ericsnowcurrently
Copy link
Member Author

Thanks. I'll take a look at that.

Copy link
Member

@encukou encukou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!
+1, I think this is good to merge. The is_singlephase thing can be sorted out later, see #117953 (comment)

@ericsnowcurrently ericsnowcurrently merged commit 44f57a9 into python:main Apr 29, 2024
35 checks passed
@ericsnowcurrently ericsnowcurrently deleted the split-up-_PyImport_LoadDynamicModuleWithSpec-2 branch April 29, 2024 15:29
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.

2 participants