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

fix pkg_resources import failures w/ py3-only loaders #2918

Merged
merged 2 commits into from Jan 11, 2022

Conversation

nitzmahone
Copy link
Contributor

@nitzmahone nitzmahone commented Dec 7, 2021

Summary of changes

#1563 introduced a subtle bug in the fallback code for pkg_resources enumeration. The original code that only used find_module later accounts for a None loader return value (ie, the requested module couldn't be found), but the new code blindly dots into the return from find_spec (which can also be None) to grab its loader. When this occurs, it inadvertently trips the AttributeError that was supposed to be there for py2 loaders that don't implement find_spec, causing find_module to be called even on a py3-native loader. Loaders that still implement the deprecated 2.x methods will (correctly) return None, triggering the old fallback path, all is well. However, new loaders that don't implement find_module will blow up the pkg_resources enumeration with another AttributeError.

The inadvertent trip of the py2 fallback happens a lot even with just the built-in loaders (eg, instrument the fallback exception handler with a print or something, then install straight.plugin in a fresh venv and import pkg_resources)- it works by accident until the builtin loaders start dropping find_module (as is planned for Python 3.11+), at which point it gets ugly fast.

This PR minimizes the try block's surface area to only the necessary loader method call, and defensively checks for a non-empty response from find_spec before trying to grab its loader (otherwise setting the loader to None to restore the previous early exit behavior for modules that can't be loaded.

This one's pretty tricky to add tests for without wiring up a bespoke py3 loader. Happy to add a changelog entry if the fix otherwise looks good...

Pull Request Checklist

@jaraco
Copy link
Member

jaraco commented Jan 11, 2022

This looks good. Thanks for reporting it.

My only concern is that someone could "optimize" the change back to the old code and no one would notice. Usually in such a situation, where creating a test is difficult, I'll instead protect the change with a comment or separate function. I'll illustrate that in a subsequent change.

@jaraco
Copy link
Member

jaraco commented Jan 11, 2022

On further consideration, after struggling to come up with a clean abstraction, I've decided to proceed with this implementation. Thanks!

@jaraco jaraco merged commit 8fd31fc into pypa:main Jan 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants