Skip to content

gh-140011: Delete importdl assertion that prevents importing embedded modules from packages - #141605

Merged
encukou merged 8 commits into
python:mainfrom
itamaro:gh-140011-dotted-import
Nov 26, 2025
Merged

gh-140011: Delete importdl assertion that prevents importing embedded modules from packages#141605
encukou merged 8 commits into
python:mainfrom
itamaro:gh-140011-dotted-import

Conversation

@itamaro

@itamaroitamaro commented Nov 16, 2025•
edited by bedevere-app Bot
Loading

Copy link
Copy Markdown
Contributor

I still don't know what's the context behind this assertion, just that I didn't see any issues from removing it.

@itamaro
Modify test embed to hit the assertion
4949865
with this, test_embed fails with
```
Assertion failed: (PyUnicode_FindChar(name, '.', 0, PyUnicode_GetLength(name), -1) == -1), function _Py_ext_module_loader_info_init_for_builtin, file importdl.c, line 159.
```
@itamaro
Delete the assertion
34e81ce
with this, test_embed succeeds again
@itamaro
itamaro requested a review from encukouNovember 16, 2025 00:23
@bedevere-appbedevere-appBot mentioned this pull request Nov 16, 2025
Closed
@encukou

encukou commented Nov 17, 2025

Copy link
Copy Markdown
Member

It was added in #118204

@ericsnowcurrently, do you remember if there was a reason for this assert?
Can we assume you added asserts when exploring which code paths are taken, and since we had no tests for modules with a dot added via PyImport_AppendInittab, this assert just never fired?

@itamaro: I'd be a bit more careful in _testembed: from test import create_static_module assumes that the testpackage is available, which is not always the case. I don't think you'd necessarily want to run testembed without test, but I also don't think we should add an arbitrary requirement/assumption.

Could you add a different containing package to the inittab?

@brettcannon
brettcannon removed their request for review November 17, 2025 19:24
@itamaro

itamaro commented Nov 18, 2025

Copy link
Copy Markdown
ContributorAuthor

@itamaro: I'd be a bit more careful in _testembed: from test import create_static_module assumes that the testpackage is available, which is not always the case. I don't think you'd necessarily want to run testembed without test, but I also don't think we should add an arbitrary requirement/assumption.

Could you add a different containing package to the inittab?

Can _testembed even be meaningfully used without the test package? (and test_embed.py driving it)

Regardless, I agree this is an awkward way to test this. I think it would make more sense to add a net new test case for this functionality, if we decide it's a desired functionality we don't want to regress.
Happy to do that when we reach such a conclusion, but let's first make sure the assertion is safe to remove!

@encukou

encukou commented Nov 18, 2025

Copy link
Copy Markdown
Member

I'd say that as an embedder, you're probably in the best position both to test this, and to decide if it's desired functionality.

Allowing submodules in inittab looks doable. (@brettcannon, do you have any thoughts on BuiltinImporter loading submodules?)

Allowing packages, on the other hand, might be worth more discussion -- that would probably need new API, since struct _inittab doesn't have the is_package bit.

Can _testembed even be meaningfully used without the test package? (and test_embed.py driving it)

It can! Not sure if it's meaningful to run it without the test package available, but I do use the CLI on its own when debugging.

Anyway that's not the question to ask. More coupling bad, self-contained good :)

@itamaro

itamaro commented Nov 19, 2025

Copy link
Copy Markdown
ContributorAuthor

thanks @encukou
since the same assertion is triggered when importing submodules and modules within packages if they use inittab, addressing the issue for one use-case should also apply to the other!
note that the "modules in packages" use-case is not the same as "inittab-based packages". the name & initfunc in inittab will always be used to create a module or submodule, and the existence of a parent package is orthogonal.
I will look into adding dedicated test cases for both scenarios (and decouple from the test I hijacked), although I'm not quite sure how to set things up such that some package will be available at runtime for _testembed to use (without assuming something about the external environment, or having test_embed arrange things accordingly before invoking _testembed)

@brettcannon

brettcannon commented Nov 19, 2025

Copy link
Copy Markdown
Member

Allowing submodules in inittab looks doable. (@brettcannon, do you have any thoughts on BuiltinImporter loading submodules?)

I think it's fine to add. I suspect it didn't have it just because the need never came up.

@itamaro

itamaro commented Nov 23, 2025

Copy link
Copy Markdown
ContributorAuthor

@encukou I added new dedicated tests

encukou
encukou reviewed Nov 25, 2025
View reviewed changes

@encukouencukou left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This looks good, thank you!

Comment threadPrograms/_testembed.c Outdated
@itamaro@encukou
Update Programs/_testembed.c
36dd0fe
Co-authored-by: Petr Viktorin <encukou@gmail.com>
itamaro
itamaro commented Nov 26, 2025
View reviewed changes
Comment threadPrograms/_testembed.c
@encukou

encukou commented Nov 26, 2025

Copy link
Copy Markdown
Member

Looks good, thank you!

@encukou
encukou merged commit 27f62eb into python:mainNov 26, 2025
48 checks passed
@itamaro
itamaro deleted the gh-140011-dotted-import branch November 26, 2025 16:20
@itamaroitamaro added needs backport to 3.13 bugs and security fixes needs backport to 3.14 bugs and security fixes labels Nov 26, 2025
@miss-islington-app

miss-islington-appBot commented Nov 26, 2025

Copy link
Copy Markdown

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

@miss-islington-app

miss-islington-appBot commented Nov 26, 2025

Copy link
Copy Markdown

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

@miss-islington-app

miss-islington-appBot commented Nov 26, 2025

Copy link
Copy Markdown

Sorry, @itamaro and @encukou, I could not cleanly backport this to 3.14 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 27f62eb711720c215f9798ae30728ee5a1b4d442 3.14

@miss-islington-app

miss-islington-appBot commented Nov 26, 2025

Copy link
Copy Markdown

Sorry, @itamaro and @encukou, I could not cleanly backport this to 3.13 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 27f62eb711720c215f9798ae30728ee5a1b4d442 3.13

@itamaro
itamaro restored the gh-140011-dotted-import branch November 26, 2025 18:22
itamaro added a commit to itamaro/cpython that referenced this pull request Nov 26, 2025
@itamaro
pythongh-140011: Delete importdl assertion that prevents importing em…
ef05b4b
…bedded modules from packages (pythonGH-141605)
(cherry picked from commit 27f62eb)
@bedevere-app

bedevere-appBot commented Nov 26, 2025

Copy link
Copy Markdown

GH-141986 is a backport of this pull request to the 3.14 branch.

@bedevere-appbedevere-appBot removed the needs backport to 3.14 bugs and security fixes label Nov 26, 2025
itamaro added a commit to itamaro/cpython that referenced this pull request Nov 26, 2025
@itamaro
pythongh-140011: Delete importdl assertion that prevents importing em…
953c22c
…bedded modules from packages (pythonGH-141605)
(cherry picked from commit 27f62eb)
@bedevere-app

bedevere-appBot commented Nov 26, 2025

Copy link
Copy Markdown

GH-141987 is a backport of this pull request to the 3.13 branch.

@bedevere-appbedevere-appBot removed the needs backport to 3.13 bugs and security fixes label Nov 26, 2025
gpshead pushed a commit that referenced this pull request Nov 29, 2025
@itamaro
[3.14]gh-140011: Delete importdl assertion that prevents importing e…
f7d17ac
…mbedded modules from packages (GH-141605) (#141986)
gh-140011: Delete importdl assertion that prevents importing embedded modules from packages (GH-141605)
(cherry picked from commit 27f62eb)
gpshead pushed a commit that referenced this pull request Nov 29, 2025
@itamaro
[3.13]gh-140011: Delete importdl assertion that prevents importing e…
385688d
…mbedded modules from packages (GH-141605) (#141987)
gh-140011: Delete importdl assertion that prevents importing embedded modules from packages (GH-141605)
(cherry picked from commit 27f62eb)
StanFromIreland pushed a commit to StanFromIreland/cpython that referenced this pull request Dec 6, 2025
@itamaro@StanFromIreland
pythongh-140011: Delete importdl assertion that prevents importing em…
72b0c23
…bedded modules from packages (pythonGH-141605)
ashm-dev pushed a commit to ashm-dev/cpython that referenced this pull request Dec 8, 2025
@itamaro@ashm-dev
pythongh-140011: Delete importdl assertion that prevents importing em…
f2285c1
…bedded modules from packages (pythonGH-141605)
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@encukouencukouencukou approved these changes
@ericsnowcurrentlyericsnowcurrentlyAwaiting requested review from ericsnowcurrentlyericsnowcurrently is a code owner
@ncoghlanncoghlanAwaiting requested review from ncoghlanncoghlan is a code owner
@warsawwarsawAwaiting requested review from warsawwarsaw is a code owner

Assignees

@encukouencukou

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@itamaro@encukou@brettcannon