fix(downloads): accept wheels whose dist-info omits the local version…#1240
Closed
pmattsso wants to merge 1 commit into
Closed
fix(downloads): accept wheels whose dist-info omits the local version…#1240pmattsso wants to merge 1 commit into
pmattsso wants to merge 1 commit into
Conversation
… segment Some vendor-built wheels (e.g. AMD's tensorflow-rocm) name their dist-info directory using only the base version while the wheel filename includes a PEP 440 local segment. wheelfile.WheelFile rejects these with "Missing RECORD file" because it expects the directory name to match the full filename version. Add a fallback that catches WheelError and checks for RECORD and METADATA under the base-version dist-info directory when the filename carries a local version. The wheel is accepted with a warning if both files exist. Closes: python-wheel-build#1239 Signed-off-by: pmattsso <pmattsso@redhat.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe change adds a fallback validation function in downloads.py to handle wheels whose filenames contain a local version segment (e.g., "+local") not reflected in the dist-info directory name. When the standard WheelFile-based validation raises WheelError, download_wheel now attempts this fallback, which checks the ZIP for RECORD and METADATA under the base-version dist-info path, re-raising the original error if the fallback also fails. Tests were added covering both the helper function and download_wheel's updated behavior. Estimated code review effort: 2 (Simple) | ~15 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_downloads.py`:
- Around line 224-250: Add a test in tests/test_downloads.py covering
_validate_wheel_with_local_version when the wheel has a local-version filename
but the base-version dist-info is missing METADATA; mirror the existing
test_validate_wheel_with_local_version_no_record setup, omit METADATA, and
assert the helper returns False so both required base-version files are
exercised.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d38e6cc7-eecc-42f3-9b79-fa314d671b0a
📒 Files selected for processing (2)
src/fromager/downloads.pytests/test_downloads.py
Comment on lines
+224
to
+250
| def test_validate_wheel_with_local_version_valid(tmp_path: pathlib.Path) -> None: | ||
| """Fallback validation passes for a well-formed wheel with base-only dist-info.""" | ||
| filename = "pkg-1.0+local-py3-none-any.whl" | ||
| f = tmp_path / filename | ||
| with zipfile.ZipFile(f, "w") as zf: | ||
| zf.writestr("pkg-1.0.dist-info/METADATA", "Metadata-Version: 1.0\n") | ||
| zf.writestr("pkg-1.0.dist-info/RECORD", "") | ||
| assert _validate_wheel_with_local_version(f, filename) is True | ||
|
|
||
|
|
||
| def test_validate_wheel_with_local_version_no_local(tmp_path: pathlib.Path) -> None: | ||
| """Fallback returns False when the filename has no local version.""" | ||
| filename = "pkg-1.0-py3-none-any.whl" | ||
| f = tmp_path / filename | ||
| with zipfile.ZipFile(f, "w") as zf: | ||
| zf.writestr("pkg-1.0.dist-info/METADATA", "Metadata-Version: 1.0\n") | ||
| zf.writestr("pkg-1.0.dist-info/RECORD", "") | ||
| assert _validate_wheel_with_local_version(f, filename) is False | ||
|
|
||
|
|
||
| def test_validate_wheel_with_local_version_no_record(tmp_path: pathlib.Path) -> None: | ||
| """Fallback returns False when RECORD is missing from base-version dist-info.""" | ||
| filename = "pkg-1.0+local-py3-none-any.whl" | ||
| f = tmp_path / filename | ||
| with zipfile.ZipFile(f, "w") as zf: | ||
| zf.writestr("pkg-1.0.dist-info/METADATA", "Metadata-Version: 1.0\n") | ||
| assert _validate_wheel_with_local_version(f, filename) is False |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Missing test for absent METADATA in base-version dist-info.
There's a test for missing RECORD but not for missing METADATA. The implementation checks for both files, so this edge case should be covered.
🧪 Proposed test addition
def test_validate_wheel_with_local_version_no_record(tmp_path: pathlib.Path) -> None:
"""Fallback returns False when RECORD is missing from base-version dist-info."""
filename = "pkg-1.0+local-py3-none-any.whl"
f = tmp_path / filename
with zipfile.ZipFile(f, "w") as zf:
zf.writestr("pkg-1.0.dist-info/METADATA", "Metadata-Version: 1.0\n")
assert _validate_wheel_with_local_version(f, filename) is False
+
+
+def test_validate_wheel_with_local_version_no_metadata(tmp_path: pathlib.Path) -> None:
+ """Fallback returns False when METADATA is missing from base-version dist-info."""
+ filename = "pkg-1.0+local-py3-none-any.whl"
+ f = tmp_path / filename
+ with zipfile.ZipFile(f, "w") as zf:
+ zf.writestr("pkg-1.0.dist-info/RECORD", "")
+ assert _validate_wheel_with_local_version(f, filename) is False📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def test_validate_wheel_with_local_version_valid(tmp_path: pathlib.Path) -> None: | |
| """Fallback validation passes for a well-formed wheel with base-only dist-info.""" | |
| filename = "pkg-1.0+local-py3-none-any.whl" | |
| f = tmp_path / filename | |
| with zipfile.ZipFile(f, "w") as zf: | |
| zf.writestr("pkg-1.0.dist-info/METADATA", "Metadata-Version: 1.0\n") | |
| zf.writestr("pkg-1.0.dist-info/RECORD", "") | |
| assert _validate_wheel_with_local_version(f, filename) is True | |
| def test_validate_wheel_with_local_version_no_local(tmp_path: pathlib.Path) -> None: | |
| """Fallback returns False when the filename has no local version.""" | |
| filename = "pkg-1.0-py3-none-any.whl" | |
| f = tmp_path / filename | |
| with zipfile.ZipFile(f, "w") as zf: | |
| zf.writestr("pkg-1.0.dist-info/METADATA", "Metadata-Version: 1.0\n") | |
| zf.writestr("pkg-1.0.dist-info/RECORD", "") | |
| assert _validate_wheel_with_local_version(f, filename) is False | |
| def test_validate_wheel_with_local_version_no_record(tmp_path: pathlib.Path) -> None: | |
| """Fallback returns False when RECORD is missing from base-version dist-info.""" | |
| filename = "pkg-1.0+local-py3-none-any.whl" | |
| f = tmp_path / filename | |
| with zipfile.ZipFile(f, "w") as zf: | |
| zf.writestr("pkg-1.0.dist-info/METADATA", "Metadata-Version: 1.0\n") | |
| assert _validate_wheel_with_local_version(f, filename) is False | |
| def test_validate_wheel_with_local_version_valid(tmp_path: pathlib.Path) -> None: | |
| """Fallback validation passes for a well-formed wheel with base-only dist-info.""" | |
| filename = "pkg-1.0+local-py3-none-any.whl" | |
| f = tmp_path / filename | |
| with zipfile.ZipFile(f, "w") as zf: | |
| zf.writestr("pkg-1.0.dist-info/METADATA", "Metadata-Version: 1.0\n") | |
| zf.writestr("pkg-1.0.dist-info/RECORD", "") | |
| assert _validate_wheel_with_local_version(f, filename) is True | |
| def test_validate_wheel_with_local_version_no_local(tmp_path: pathlib.Path) -> None: | |
| """Fallback returns False when the filename has no local version.""" | |
| filename = "pkg-1.0-py3-none-any.whl" | |
| f = tmp_path / filename | |
| with zipfile.ZipFile(f, "w") as zf: | |
| zf.writestr("pkg-1.0.dist-info/METADATA", "Metadata-Version: 1.0\n") | |
| zf.writestr("pkg-1.0.dist-info/RECORD", "") | |
| assert _validate_wheel_with_local_version(f, filename) is False | |
| def test_validate_wheel_with_local_version_no_record(tmp_path: pathlib.Path) -> None: | |
| """Fallback returns False when RECORD is missing from base-version dist-info.""" | |
| filename = "pkg-1.0+local-py3-none-any.whl" | |
| f = tmp_path / filename | |
| with zipfile.ZipFile(f, "w") as zf: | |
| zf.writestr("pkg-1.0.dist-info/METADATA", "Metadata-Version: 1.0\n") | |
| assert _validate_wheel_with_local_version(f, filename) is False | |
| def test_validate_wheel_with_local_version_no_metadata(tmp_path: pathlib.Path) -> None: | |
| """Fallback returns False when METADATA is missing from base-version dist-info.""" | |
| filename = "pkg-1.0+local-py3-none-any.whl" | |
| f = tmp_path / filename | |
| with zipfile.ZipFile(f, "w") as zf: | |
| zf.writestr("pkg-1.0.dist-info/RECORD", "") | |
| assert _validate_wheel_with_local_version(f, filename) is False |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_downloads.py` around lines 224 - 250, Add a test in
tests/test_downloads.py covering _validate_wheel_with_local_version when the
wheel has a local-version filename but the base-version dist-info is missing
METADATA; mirror the existing test_validate_wheel_with_local_version_no_record
setup, omit METADATA, and assert the helper returns False so both required
base-version files are exercised.
Source: Path instructions
Collaborator
|
These wheels are in violation of https://packaging.python.org/en/latest/specifications/binary-distribution-format/ and therefore invalid . The wheel file name, dist-info directory, and content of METADATA file must match. |
Contributor
Author
|
Thanks @tiran I will close this PR. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
… segment
Some vendor-built wheels (e.g. AMD's tensorflow-rocm) name their dist-info directory using only the base version while the wheel filename includes a PEP 440 local segment. wheelfile.WheelFile rejects these with "Missing RECORD file" because it expects the directory name to match the full filename version. Add a fallback that catches WheelError and checks for RECORD and METADATA under the base-version dist-info directory when the filename carries a local version. The wheel is accepted with a warning if both files exist.
Closes: #1239
Pull Request Description
What
Add a fallback in download_wheel() for wheels whose dist-info directory uses the base version instead of the full version including the local segment. When wheelfile.WheelFile raises WheelError, the new _validate_wheel_with_local_version() helper checks whether RECORD and METADATA exist under {name}-{public_version}.dist-info/. The wheel is accepted with a warning if valid; the original error is re-raised otherwise. Four tests cover the new code path.
Why
What
Add a fallback in download_wheel() for wheels whose dist-info directory uses the base version instead of the full version including the local segment. When wheelfile.WheelFile raises WheelError, the new _validate_wheel_with_local_version() helper checks whether RECORD and METADATA exist under {name}-{public_version}.dist-info/. The wheel is accepted with a warning if valid; the original error is re-raised otherwise. Four tests cover the new code path.
Why
Some vendor-built wheels (e.g. AMD's tensorflow_rocm-2.19.1+rocm7.14.0a20260624) name their dist-info directory with only the base version (tensorflow_rocm-2.19.1.dist-info) while the filename carries a PEP 440 local segment. WheelFile rejects these with "Missing RECORD file" because it expects the directory name to match the full filename version. This blocks fromager bootstrap for any pre-built wheel that follows this pattern.