- Notifications
You must be signed in to change notification settings - Fork1.3k
Udpate ensurepip from CPython 3.13.2#5740
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Need#5590 first from what it seems |
Well typing is now broken in new and interesting ways :) |
arihant2math commentedMay 1, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
The error is a typing_extensions bug: |
coderabbitaibot commentedJun 15, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
""" WalkthroughThe ensurepip module was refactored to streamline pip bootstrapping by focusing exclusively on pip, removing multi-package support and simplifying wheel discovery. The new implementation uses pathlib.Path for wheel directory handling, introduces context managers for locating the pip wheel, and updates tests to match the new logic and data structures. Changes
Sequence Diagram(s)sequenceDiagram participant User participant ensurepip participant SystemWheelDir participant BundledResource User->>ensurepip: _bootstrap() ensurepip->>SystemWheelDir: _find_wheel_pkg_dir_pip() alt pip wheel found in system dir SystemWheelDir-->>ensurepip: Return pip wheel path context else pip wheel not found ensurepip->>BundledResource: Use bundled pip wheel path context end ensurepip->>ensurepip: Copy pip wheel to temp dir ensurepip->>ensurepip: _run_pip("pip", [copied_wheel_path])Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat withCodeRabbit:
SupportNeed help? Create a ticket on oursupport page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
Lib/test/test_ensurepip.py (1)
9-9:Remove unused import.The
Traversableimport is not used anywhere in this test file.-from importlib.resources.abc import Traversable from pathlib import PathLib/ensurepip/__init__.py (1)
53-61:Consider more robust version extraction.While the current string manipulation works for the standard wheel naming convention, consider using a more robust approach to handle edge cases.
def _get_pip_version(): with _get_pip_whl_path_ctx() as bundled_wheel_path: wheel_name = bundled_wheel_path.name- return (- # Extract '21.2.4' from 'pip-21.2.4-py3-none-any.whl'- wheel_name.- removeprefix('pip-').- partition('-')[0]- )+ # Extract version from wheel filename: pip-{version}-py3-none-any.whl+ if wheel_name.startswith('pip-') and wheel_name.endswith('.whl'):+ # Remove prefix and suffix, then split on '-'+ name_parts = wheel_name[4:-4].split('-', 1)+ if name_parts:+ return name_parts[0]+ # Fallback to the bundled version if parsing fails+ return _PIP_VERSION
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Lib/ensurepip/__init__.py(5 hunks)Lib/test/test_ensurepip.py(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
Lib/test/test_ensurepip.py (2)
Lib/importlib/resources/abc.py (1)
Traversable(63-150)Lib/ensurepip/__init__.py (2)
version(90-94)_get_pip_whl_path_ctx(41-50)
Lib/ensurepip/__init__.py (3)
Lib/contextlib.py (1)
nullcontext(761-785)Lib/shutil.py (1)
copy2(441-479)Lib/importlib/resources/_common.py (1)
as_file(162-167)
🪛 Pylint (3.3.7)
Lib/ensurepip/__init__.py
[error] 19-19: Module 'sysconfig' has no 'get_config_var' member
(E1101)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: Run tests under miri
- GitHub Check: Check the WASM package and demo
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Check Rust code with rustfmt and clippy
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Ensure compilation on various targets
🔇 Additional comments (9)
Lib/test/test_ensurepip.py (4)
25-25:LGTM!Correctly updated to patch with a
Pathobject, aligning with the main module's type change.
28-38:Good test coverage for the no-directory case.These new tests properly verify the fallback behavior when
_WHEEL_PKG_DIRis None, ensuring the bundled pip wheel is used.
40-53:Well-structured test for wheel selection.The test properly verifies that
_get_pip_whl_path_ctx()selects only the pip wheel file from the directory, correctly ignoring other wheels and non-wheel files.
68-69:Comment accurately updated.The comment now correctly references
_find_wheel_pkg_dir_pip()instead of the old function name.Lib/ensurepip/__init__.py (5)
6-9:LGTM!All new imports are appropriately used in the refactored code.
13-13:Pip version updated as intended.The update to pip 25.1.1 aligns with the PR objective of updating from CPython 3.13.2.
19-22:Clean handling of optional wheel directory.The code properly converts the config variable to a resolved
Pathobject when present, orNoneotherwise. The static analysis warning aboutget_config_varis a false positive.🧰 Tools
🪛 Pylint (3.3.7)
[error] 19-19: Module 'sysconfig' has no 'get_config_var' member
(E1101)
156-172:Clean refactoring of the bootstrap logic.The simplified approach focusing solely on pip makes the code more maintainable. Good use of context managers and proper file copying with metadata preservation.
203-203:Consistent with single-package approach.The change correctly reflects the module's focus on pip only.
This is weird. We have typing.ParamSpec but it hits the else block |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
Lib/test/test_ensurepip.py (1)
9-9:Remove unused import or clarify its purpose.The
Traversableimport appears unused in the current code. If it's intended for future use or type annotations, consider adding a comment to clarify its purpose.-from importlib.resources.abc import TraversableLib/ensurepip/__init__.py (1)
25-39:Consider semantic version-aware sorting.The function uses lexicographical sorting (
sorted()[-1]) to select the latest pip wheel, which may not work correctly for semantic versioning. For example, "pip-10.0.0.whl" would sort before "pip-9.0.0.whl" lexicographically.Consider using a more robust version comparison:
- last_matching_dist_wheel = sorted(dist_matching_wheels)[-1]+ from packaging import version+ def extract_version(wheel_path):+ wheel_name = wheel_path.name+ return version.parse(wheel_name.removeprefix('pip-').partition('-')[0])++ last_matching_dist_wheel = max(dist_matching_wheels, key=extract_version)However, if the current approach works reliably in practice for the expected wheel naming patterns, the change may not be necessary.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Lib/ensurepip/__init__.py(5 hunks)Lib/test/test_ensurepip.py(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
`Lib/**/*`: Files in the Lib/ directory (Python standard library copied from CPy...
Lib/**/*: Files in the Lib/ directory (Python standard library copied from CPython) should be edited very conservatively; modifications should be minimal and only to work around RustPython limitations.
📄 Source: CodeRabbit Inference Engine (.github/copilot-instructions.md)
List of files the instruction was applied to:
Lib/ensurepip/__init__.pyLib/test/test_ensurepip.py
`Lib/test/**/*`: Tests in Lib/test often use markers such as '# TODO: RUSTPYTHON...
Lib/test/**/*: Tests in Lib/test often use markers such as '# TODO: RUSTPYTHON', 'unittest.skip("TODO: RustPython ")', or 'unittest.expectedFailure' with a '# TODO: RUSTPYTHON ' comment when modifications are made.
NEVER comment out or delete any test code lines except for removing '@unittest.expectedFailure' decorators and upper TODO comments.
NEVER modify test assertions, test logic, or test data in Lib/test files.
The only acceptable modifications to test files are: (1) removing '@unittest.expectedFailure' decorators and the upper TODO comments when tests actually pass, (2) adding '@unittest.expectedFailure' decorators when tests cannot be fixed.
When a test cannot pass due to missing language features, keep it as expectedFailure and document the reason.
📄 Source: CodeRabbit Inference Engine (.github/copilot-instructions.md)
List of files the instruction was applied to:
Lib/test/test_ensurepip.py
🧬 Code Graph Analysis (2)
Lib/ensurepip/__init__.py (3)
Lib/contextlib.py (1)
nullcontext(761-785)Lib/shutil.py (1)
copy2(441-479)Lib/importlib/resources/_common.py (1)
as_file(162-167)
Lib/test/test_ensurepip.py (2)
Lib/importlib/resources/abc.py (1)
Traversable(63-150)Lib/ensurepip/__init__.py (1)
_get_pip_whl_path_ctx(41-50)
🪛 Pylint (3.3.7)
Lib/ensurepip/__init__.py
[error] 19-19: Module 'sysconfig' has no 'get_config_var' member
(E1101)
🪛 Flake8 (7.2.0)
Lib/test/test_ensurepip.py
[error] 9-9: 'importlib.resources.abc.Traversable' imported but unused
(F401)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Ensure compilation on various targets
- GitHub Check: Run rust tests (ubuntu-latest)
🔇 Additional comments (11)
Lib/test/test_ensurepip.py (3)
25-25:LGTM! Correctly updated to use Path object.The change from string to
Path(tmpdir)aligns with the refactored implementation where_WHEEL_PKG_DIRis now aPathobject.
28-53:Excellent test coverage for the new functionality.The new test methods properly cover:
- Version extraction when
_WHEEL_PKG_DIRisNone(bundled wheel scenario)- Context manager behavior in both bundled and system wheel directory scenarios
- Proper wheel selection from directories with multiple files
The test logic and assertions are correct and align with the refactored implementation.
68-68:LGTM! Comment correctly updated.The comment properly reflects the new function name
_find_wheel_pkg_dir_pip()that replaced_find_packages().Lib/ensurepip/__init__.py (8)
6-9:LGTM! All new imports are properly utilized.The added imports support the refactored functionality:
nullcontextfor conditional context managementPathfor wheel directory handlingcopy2for wheel file copying
19-23:LGTM! Clean implementation of wheel directory handling.The logic properly:
- Uses walrus operator for concise conditional assignment
- Converts string path to resolved
Pathobject when available- Defaults to
Nonewhen no system wheel directory is configured
41-50:LGTM! Well-designed context manager for wheel path resolution.The function properly:
- Prioritizes system wheel directory over bundled wheels
- Uses
resources.as_file()for proper resource handling- Returns appropriate context managers in both scenarios
- Uses clear path construction with the
/operator
53-61:LGTM! Clean version extraction implementation.The function correctly:
- Uses the context manager to access the wheel path
- Parses the version from wheel filename using appropriate string methods
- Follows the wheel naming convention (pip-VERSION-py3-none-any.whl)
- Uses readable method chaining with proper line breaks
94-94:LGTM! Version function now correctly returns dynamic version.The change from returning a hardcoded constant to calling
_get_pip_version()ensures the version reflects the actual wheel being used, whether bundled or from the system directory.
156-172:LGTM! Bootstrap logic simplified and improved.The refactored bootstrap code:
- Uses the new context manager for consistent wheel path handling
- Properly copies wheel files with
copy2(preserving metadata)- Explicitly passes "pip" as the package argument for clarity
- Uses
os.fsdecodefor proper path encoding in subprocess callsThe logic is cleaner and more maintainable than the previous multi-package approach.
203-203:LGTM! Explicit package specification improves clarity.The change to explicitly pass "pip" as an argument makes the uninstall behavior clear and consistent with the refactored bootstrap logic.
13-13:Verify the pip version update.The pip version was updated from "23.2.1" to "25.1.1", which represents a significant jump. Please verify this version number is correct for CPython 3.13.2 and that the corresponding wheel file exists in the
_bundleddirectory.#!/bin/bash# Verify the pip wheel file exists with the updated versionfd"pip-25.1.1-py3-none-any.whl" Lib/ensurepip/_bundled/
Uh oh!
There was an error while loading.Please reload this page.
Summary by CodeRabbit
Refactor
Tests