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.

Sign up for GitHub

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

Jump to bottom

Adapt to removed stdlib APIs in Python 3.12. #2170

Merged
merged 4 commits into from
Jul 5, 2023
Merged

Adapt to removed stdlib APIs in Python 3.12. #2170

merged 4 commits into from
Jul 5, 2023

Conversation

jsirois
Copy link
Member

@jsirois jsirois commented Jul 5, 2023 •
edited
Loading

Closes #2137

@jsirois
Copy link
Member Author

jsirois commented Jul 5, 2023

This was proved out in my Python 3.12 branch here: #2168
Broken off as an independent chunk. This PR just proves this works with all the existing Pythons.

jsirois
jsirois commented Jul 5, 2023
View reviewed changes
pex/pex_builder.py Outdated
from importlib.abc import Loader
if isinstance(__loader__, Loader):
__entry_point__ = __entry_point_from_filename__(__loader__.get_filename())
except ImportError:
Copy link
Member Author

Choose a reason for hiding this comment

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

This order is a bit better since it stresses the code now as well as getting rid of warnings under newer Pythons. It penalizes existing Python 2.7 users with a small speed bump for the cold cache case in the ~ms range though.

Copy link
Member Author

Choose a reason for hiding this comment

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

Alright - took the feedback but went further given the prompt to think.

Eric-Arellano
Eric-Arellano approved these changes Jul 5, 2023
View reviewed changes
Copy link
Contributor

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

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

Thank you for splitting this out. It may be worth a comment in the new code explaining which branch corresponds to which Python versions.

benjyw
benjyw approved these changes Jul 5, 2023
View reviewed changes
Copy link
Collaborator

@benjyw benjyw left a comment

Choose a reason for hiding this comment

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

Agree with Eric re documenting the control flow

jsirois
jsirois commented Jul 5, 2023
View reviewed changes
pex/pex_builder.py
if hasattr(__loader__, 'archive'):
__entry_point__ = __loader__.archive
elif isinstance(__loader__, ImpLoader):
elif hasattr(__loader__, 'get_filename'):
Copy link
Member Author

Choose a reason for hiding this comment

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

Although it's still true Pex contains vendored code that uses ImpLoader (inside Pip), this adjustment as well as the support for find_spec / PEP-451 below is enough to close #2137.

@jsirois jsirois merged commit 814ac93 into pex-tool:main Jul 5, 2023
@jsirois jsirois deleted the python3.12/deprecated-api-use-prep branch July 5, 2023 19:09
This was referenced Jul 5, 2023
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benjyw benjyw benjyw approved these changes

@Eric-Arellano Eric-Arellano Eric-Arellano approved these changes

@huonw huonw Awaiting requested review from huonw

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Replace deprecated imp module with importlib
3 participants
@jsirois @benjyw @Eric-Arellano