Skip to content

gh-121342: Fixed pkgutil.iter_zipimport_modules on an invalidated cache (#121342) #121705

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

beachmachine
Copy link
Contributor

@beachmachine beachmachine commented Jul 13, 2024

It is no longer safe to directly access zipimport._zip_directory_cache since #103208. It is not guaranteed that the cache is acutally filled. This changed fixes this by using the internal method _get_files instead, which may not be the best solution, but fixes the issue.

Copy link
Member

@ZeroIntensity ZeroIntensity left a comment

Choose a reason for hiding this comment

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

Only a few formatting nitpicks. Failing CI is caused by pkgutil.iter_zipimport_modules being undocumented. Ideally, you should document it with this PR.

@@ -522,6 +522,54 @@ def test_mixed_namespace(self):
del sys.modules['foo.bar']
del sys.modules['foo.baz']


Copy link
Member

Choose a reason for hiding this comment

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

Only one blank line between methods, per PEP 8

Suggested change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed the blank line. Also, I've learned that you prefer not to have force-pushes on CPython PRs. I'll respect that for future PRs. :)

…ated cache (python#121342)

It is no longer safe to directly access `zipimport._zip_directory_cache` since python#103208. It is
not guaranteed that the cache is acutally filled. This changed fixes this by using the
internal method `_get_files` instead, which may not be the best solution, but fixes the issue.
@beachmachine
Copy link
Contributor Author

Only a few formatting nitpicks. Failing CI is caused by pkgutil.iter_zipimport_modules being undocumented. Ideally, you should document it with this PR.

Added documentation to that, but now the CI complaints that py:func reference target not found: pkgutil.iter_zipimport_modules, which confuses me as it is clearly there. 🤔

As `pkgutil.iter_zipimport_modules` isn't supposed to be consumed directly
by anything other than `pkgutils` itself, and is registered via as an
importer module for `zipimporter`, I've decided to not include it in the
public documentation. Thus, the `:func:` reference
of `pkgutil.iter_zipimport_modules` in the news file is removed now.
@beachmachine
Copy link
Contributor Author

Only a few formatting nitpicks. Failing CI is caused by pkgutil.iter_zipimport_modules being undocumented. Ideally, you should document it with this PR.

Added documentation to that, but now the CI complaints that py:func reference target not found: pkgutil.iter_zipimport_modules, which confuses me as it is clearly there. 🤔

Ah, it wasn't clear to me that the public documentation and the docstrings are a separate thing.

As pkgutil.iter_zipimport_modules isn't supposed to be consumed directly by anything other than pkgutils itself, and is registered as an importer module for zipimporter, I've decided to not include it in the public documentation. Thus, the :func: reference of pkgutil.iter_zipimport_modules in the news file is removed now. However, I've added a docstring for the function.

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