Skip to content

Conversation

@dr-carlos
Copy link
Contributor

@dr-carlos dr-carlos commented Nov 28, 2025

  • Prior to 1e4e59b, Py_NewRef(Py_None) was returned in this case; so, theoretically, this should be re-introducing behaviour that was previously broken by that commit.

@dr-carlos
Copy link
Contributor Author

On further inspection, is_core_module() is only defined under an #ifndef NDEBUG statement (since it was only used in assert() calls), so it's not valid to use in non-debug builds. Would it be acceptable to move this out of the #if?

@dr-carlos
Copy link
Contributor Author

On further inspection, is_core_module() is only defined under an #ifndef NDEBUG statement (since it was only used in assert() calls), so it's not valid to use in non-debug builds. Would it be acceptable to move this out of the #if?

No longer relevant for the improved implementation. Thanks to @itamaro.

@dr-carlos
Copy link
Contributor Author

Tests seem to be failing on Android/iOS because pickled __loader__ is not builtins.__loader__. This is probably because __loader__.create_module calls _imp.create_builtin(), which was modified in this PR.

Unfortunately it's not reproducible on my Linux computer (hence the Linux tests passing fine). Not sure if it's trying to compare the new __loader__ with the old __loader__ from a previous Python version?

Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

I prefer raising an exception as done in #142033 instead of returning None.

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