refactor(downloads): centralize download helpers in new module#1227
refactor(downloads): centralize download helpers in new module#1227tiran wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (8)
📝 WalkthroughWalkthroughThis PR adds a centralized Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant BuildCmd as commands/build._is_wheel_built
participant Sources as sources.default_download_source
participant Wheels as wheels.download_wheel
participant Downloads as downloads module
BuildCmd->>Downloads: extract_filename_from_url(url)
Sources->>Downloads: extract_filename_from_url(url)
Sources->>Downloads: download_sdist(destination_dir, url, destination_filename)
Wheels->>Downloads: extract_filename_from_url(url)
Wheels->>Downloads: download_wheel(destination_dir, url, destination_filename)
Downloads-->>Sources: resolved sdist path
Downloads-->>Wheels: resolved wheel path
Compact metadataRelated issues: None specified Poem 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 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: 3
🧹 Nitpick comments (3)
src/fromager/downloads.py (2)
184-190: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueException chaining discards original traceback.
from Nonesuppresses the originalFileExistsErrorcontext. Chaining withfrom errpreserves the underlying cause for debugging.As per coding guidelines, "Chain exceptions using
raise ValueError(...) from errsyntax."🔧 Proposed fix
try: destination_dir.mkdir(parents=True, exist_ok=False) - except FileExistsError: + except FileExistsError as err: raise FileExistsError( f"destination directory {destination_dir} already exists, " f"cannot clone {vcs_url}" - ) from None + ) from err🤖 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 `@src/fromager/downloads.py` around lines 184 - 190, The FileExistsError handling in downloads.py suppresses the original exception context by using from None. Update the destination_dir.mkdir exception path to capture the caught FileExistsError as a named variable in the except block and re-raise the new FileExistsError from that original exception so the traceback and underlying cause are preserved. Keep the existing error message in the clone/download flow around destination_dir and vcs_url, but change the chaining to follow the project guideline of raising from the original err.Source: Coding guidelines
120-129: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
typing.assert_neveron a plainstrrequires a suppression comment; a simpleelse: raiseis clearer.Since
basenameis typedstr(not narrowed via a literal exhaustiveness check), this relies on# type: ignore[arg-type]to pass mypy. An explicitraise ValueError(...)avoids the type-ignore and better documents intent for an unreachable-but-defensive branch.🔧 Proposed fix
- else: - typing.assert_never(basename) # type: ignore[arg-type] + else: + raise ValueError(f"unsupported sdist extension: {basename}")🤖 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 `@src/fromager/downloads.py` around lines 120 - 129, The fallback branch in downloads validation uses typing.assert_never on basename, but basename is just a str here, so replace that unreachable check with an explicit defensive exception. Update the logic in the tar/zip handling block in downloads.py so the final else branch raises a clear ValueError for unsupported file types instead of relying on assert_never and a type-ignore. Keep the tarfile.open and zipfile.ZipFile checks unchanged, and preserve the existing basename-based branching.src/fromager/sources.py (1)
42-67: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a Sphinx
versionchanged/versionremoveddirective.Deprecation drops the
reqparameter's effect and delegates to a new module — this is a user-facing behavior change. As per coding guidelines,**/*.{rst,py}should use Sphinxversionadded,versionremoved,versionchangeddirectives for user-facing changes; only.. deprecated::is present here.📝 Proposed docstring addition
"""Download a URL to destination_dir, returning the local path. .. deprecated:: Use :func:`fromager.downloads.download_url` instead. The ``req`` parameter is unused and will be removed. + + .. versionchanged:: NEXT + Delegates to :func:`fromager.downloads.download_url`; the + ``req`` parameter no longer has any effect. """🤖 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 `@src/fromager/sources.py` around lines 42 - 67, Add a Sphinx versionchanged/versionremoved note to the download_url docstring in sources.download_url, since the function’s user-facing behavior changed when it stopped using req and delegated to fromager.downloads.download_url. Keep the existing deprecation notice, and document that req is now unused/removed so the change is surfaced consistently with the coding guidelines.Source: Coding guidelines
🤖 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 `@src/fromager/downloads.py`:
- Around line 21-196: The new public functions in downloads.py are missing the
project’s Sphinx version metadata convention. Add appropriate `..
versionadded::` directives to the docstrings of `extract_filename_from_url`,
`download_url`, `download_sdist`, `download_wheel`, and `download_git_source`,
matching the style used by `gitutils.parse_vcs_url` so the new user-facing API
is documented consistently.
- Around line 82-89: The atomic file move in the download save path still uses
os.rename, which can fail if the destination already exists and can race with
another writer. Update the save/rename logic in the function that writes tmp
files and returns outfile to use os.replace instead of os.rename so the move
remains atomic and safely overwrites existing files on all platforms.
In `@tests/test_downloads.py`:
- Around line 125-130: The test in test_download_sdist_rejects_invalid_name uses
a hardcoded /tmp path, which is non-portable and trips static analysis; update
this test and the other affected download test block to use the existing
tmp_path fixture instead of pathlib.Path("/tmp"). Keep the rest of the
assertions and the download_sdist call unchanged, and reference the test
function names to locate both occurrences.
---
Nitpick comments:
In `@src/fromager/downloads.py`:
- Around line 184-190: The FileExistsError handling in downloads.py suppresses
the original exception context by using from None. Update the
destination_dir.mkdir exception path to capture the caught FileExistsError as a
named variable in the except block and re-raise the new FileExistsError from
that original exception so the traceback and underlying cause are preserved.
Keep the existing error message in the clone/download flow around
destination_dir and vcs_url, but change the chaining to follow the project
guideline of raising from the original err.
- Around line 120-129: The fallback branch in downloads validation uses
typing.assert_never on basename, but basename is just a str here, so replace
that unreachable check with an explicit defensive exception. Update the logic in
the tar/zip handling block in downloads.py so the final else branch raises a
clear ValueError for unsupported file types instead of relying on assert_never
and a type-ignore. Keep the tarfile.open and zipfile.ZipFile checks unchanged,
and preserve the existing basename-based branching.
In `@src/fromager/sources.py`:
- Around line 42-67: Add a Sphinx versionchanged/versionremoved note to the
download_url docstring in sources.download_url, since the function’s user-facing
behavior changed when it stopped using req and delegated to
fromager.downloads.download_url. Keep the existing deprecation notice, and
document that req is now unused/removed so the change is surfaced consistently
with the coding guidelines.
🪄 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: 213bf07d-7120-4fa4-b9ca-ca8c45b162e7
📒 Files selected for processing (8)
src/fromager/commands/build.pysrc/fromager/downloads.pysrc/fromager/resolver.pysrc/fromager/sources.pysrc/fromager/wheels.pytests/test_downloads.pytests/test_sources.pytests/test_wheels.py
tiran
force-pushed
the
tiran/downloads-module
branch
3 times, most recently
from
c495428 to
ce67c94
Compare
rd4398
left a comment
rd4398
left a comment
There was a problem hiding this comment.
Overall looks good. I have added a comment for a clarification question.
| if not wheel_filename.exists(): | ||
| logger.info(f"downloading pre-built wheel {wheel_url}") | ||
| wheel_filename = _download_wheel_check(req, output_directory, wheel_url) | ||
| wheel_filename = downloads.download_wheel( |
There was a problem hiding this comment.
The old _download_wheel_check validated using wheel.wheelfile.WheelFile(wheel_filename), which opens the zip and checks wheel-specific structure. The new download_wheel only does:
with zipfile.ZipFile(filepath) as zf:
if not zf.namelist():
raise zipfile.BadZipFile(f"empty wheel file: {filepath}")
This catches empty zips but won't catch structurally invalid wheels (e.g., a zip that has files but no .dist-info/WHEEL). The parse_wheel_filename call before download validates the name, not the content.
Was this intentional? The validation is slightly weaker now.
There was a problem hiding this comment.
I have re-added the WheelFile check with some improvements. The original code did not close the file. WheelFile also does not check for the presence of METADATA. It only checks for RECORD.
Create `fromager.downloads` as a single import point for all download helpers, avoiding cyclic import issues between `sources`, `wheels`, and `resolver`. Move `download_url` and `extract_filename_from_url` from `sources` and `resolver` into the new module. Add `download_sdist` and `download_wheel` that validate filenames with `parse_sdist_filename` / `parse_wheel_filename` before downloading and verify archives are non-empty afterwards. Add `download_git_source` for cloning from pip VCS URLs using `git_clone_fast` with destination directory conflict detection. Improve `download_url` temp file handling: use `NamedTemporaryFile` in the target directory with `os.rename` for atomic moves. Replace `_download_source_check` and `_download_wheel_check` with the new `download_sdist` and `download_wheel` functions. Co-Authored-By: Claude <claude@anthropic.com> Signed-off-by: Christian Heimes <cheimes@redhat.com>
tiran
force-pushed
the
tiran/downloads-module
branch
from
ce67c94 to
65c4642
Compare
2 participants
Pull Request Description
What
Create
fromager.downloadsas a single import point for all download helpers, avoiding cyclic import issues betweensources,wheels, andresolver.Move
download_urlandextract_filename_from_urlfromsourcesandresolverinto the new module.Add
download_sdistanddownload_wheelthat validate filenames withparse_sdist_filename/parse_wheel_filenamebefore downloading and verify archives are non-empty afterwards.Add
download_git_sourcefor cloning from pip VCS URLs usinggit_clone_fastwith destination directory conflict detection.Improve
download_urltemp file handling: useNamedTemporaryFilein the target directory withos.renamefor atomic moves.Replace
_download_source_checkand_download_wheel_checkwith the newdownload_sdistanddownload_wheelfunctions.Why
Make the code reusable without triggering cyclic import issues.