GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-117953: Split Up _PyImport_LoadDynamicModuleWithSpec() by ericsnowcurrently · Pull Request #118203 · python/cpython

Conversation

@ericsnowcurrently

@ericsnowcurrently ericsnowcurrently commented Apr 23, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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.

  • Issue: Single-Phase Init Extension Module Init Functions Still Run in Isolated Interpreters #117953

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@ericsnowcurrently ericsnowcurrently added the skip news label Apr 23, 2024
@bedevere-app bedevere-app Bot mentioned this pull request Apr 23, 2024
Single-Phase Init Extension Module Init Functions Still Run in Isolated Interpreters #117953
Closed
@ericsnowcurrently
ericsnowcurrently force-pushed the split-up-_PyImport_LoadDynamicModuleWithSpec-2 branch from cd4c52d to 378778e Compare April 23, 2024 23:31
ericsnowcurrently added 12 commits April 24, 2024 13:58
@ericsnowcurrently
Factor out _PyImport_RunDynamicModule().
0163940
@ericsnowcurrently
Drop _PyImport_LoadDynamicModuleWithSpec().
e0ac947
@ericsnowcurrently
Split up _PyImport_RunDynamicModule() along cross-interpreter-safe bo…
1fee8ca
…undaries.
@ericsnowcurrently
Drop _PyImport_RunDynamicModule().
a11eddb
@ericsnowcurrently
Factor out create_dynamic().
daf7295
@ericsnowcurrently
Drop _PyImport_FixupExtensionObject().
3a5eba6
@ericsnowcurrently
Add some asserts to _PyImport_RunModInitFunc().
2ab4bd8
@ericsnowcurrently
Check the module returned by import_find_extension() to ensure single…
9b39844
…-phase init.
@ericsnowcurrently
We already know the def is okay.
c253654
@ericsnowcurrently
Drop create_dynamic().
8172748
@ericsnowcurrently
Update the comment about extensions.
4e72371
@ericsnowcurrently
Move setting m_init to _imp_create_dynamic_impl().
52f0656
@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 brettcannon, kumaraditya303, ncoghlan and warsaw as code owners April 24, 2024 19:59
@bedevere-app bedevere-app Bot added the awaiting core review label Apr 24, 2024
@ericsnowcurrently
ericsnowcurrently requested review from encukou and removed request for brettcannon, kumaraditya303, ncoghlan and warsaw April 24, 2024 19:59
@ericsnowcurrently
Fix a refleak.
a1d1258
@ericsnowcurrently

ericsnowcurrently commented Apr 25, 2024

Copy link
Copy Markdown
Member Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@encukou

encukou commented Apr 26, 2024

Copy link
Copy Markdown
Member

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 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@ericsnowcurrently

ericsnowcurrently commented Apr 26, 2024

Copy link
Copy Markdown
Member Author

Thanks. I'll take a look at that.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

encukou
encukou approved these changes Apr 29, 2024
View reviewed changes

@encukou encukou left a comment

Copy link
Copy Markdown
Member

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting core review labels Apr 29, 2024
@ericsnowcurrently
ericsnowcurrently merged commit 44f57a9 into python:main Apr 29, 2024
@bedevere-app bedevere-app Bot removed the awaiting merge label Apr 29, 2024
@ericsnowcurrently
ericsnowcurrently deleted the split-up-_PyImport_LoadDynamicModuleWithSpec-2 branch April 29, 2024 15:29
@micwoj92 micwoj92 mentioned this pull request Dec 23, 2024
Build issue with python 3.13 and GCC14 grisha/mod_python#143
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@encukou encukou encukou approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

2 participants

@ericsnowcurrently @encukou
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.