refactor: remove deprecated req.url (build-from-URL) code paths#1260
refactor: remove deprecated req.url (build-from-URL) code paths#1260rd4398 wants to merge 1 commit into
req.url (build-from-URL) code paths#1260Conversation
|
Warning Review limit reached
Next review available in: 58 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (28)
📝 WalkthroughWalkthroughThe change removes dedicated direct Git URL resolution and build-dependency preparation from bootstrapping. Source download, preparation, source typing, and sdist building now use provider and override mechanisms. Build commands use pinned package versions instead of URL-bearing top-level requirements, and Git source types and graph helpers are removed. Resolver cache behavior and provider routing receive updated tests. Git URL end-to-end tests and related documentation are removed or revised. Estimated code review effort: 4 (Complex) | ~45 minutes 🚥 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/fromager/bootstrap_requirement_resolver.py (1)
27-39: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winStale docstring: Git URL resolution no longer "stays in Bootstrapper."
This docstring still claims git URL resolution is handled by
Bootstrapper, but per this PR's own objective, Bootstrapper-specific Git URL version discovery and build-dependency preparation are removed entirely, not relocated. Leaves a misleading breadcrumb for future readers.📝 Proposed docstring fix
- Git URL resolution is NOT handled here - that stays in Bootstrapper - because it requires BuildEnvironment and build dependencies. + Direct Git URL (PEP 508) resolution is no longer supported; use a + declarative ``source`` provider (e.g. ``pypi-git``) instead.🤖 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/bootstrap_requirement_resolver.py` around lines 27 - 39, Update the BootstrapRequirementResolver class docstring to remove the claim that Git URL resolution stays in Bootstrapper. State only the currently supported resolution responsibilities and strategies, without implying that removed Bootstrapper-specific Git URL handling was relocated.src/fromager/sources.py (1)
68-105: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winMissing docstrings on public functions.
get_source_typeanddownload_sourceare public functions with no docstrings. As per coding guidelines,**/*.py: "Add docstrings to all public functions and classes."📝 Proposed docstrings
def get_source_type(ctx: context.WorkContext, req: Requirement) -> SourceType: + """Determine how this requirement's source should be provided (sdist vs. override).""" source_type = SourceType.SDISTdef download_source( *, ctx: context.WorkContext, req: Requirement, version: Version, download_url: str, ) -> pathlib.Path: + """Download the source distribution for ``req``, delegating to any package override.""" logger.info(f"downloading source for {req}")🤖 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 68 - 105, Add docstrings to the public functions get_source_type and download_source in src/fromager/sources.py, describing their purpose, parameters, and return values. Keep their existing behavior and implementation unchanged.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 `@docs/customization.md`:
- Around line 246-247: Update the customization documentation example near the
resolve-phase note to remove “git-URL bootstrapping” terminology. Reword it
around provider-based bootstrapping and replace the example host with a reserved
.example domain, while preserving the surrounding explanation that the version
is unavailable during resolve.
---
Outside diff comments:
In `@src/fromager/bootstrap_requirement_resolver.py`:
- Around line 27-39: Update the BootstrapRequirementResolver class docstring to
remove the claim that Git URL resolution stays in Bootstrapper. State only the
currently supported resolution responsibilities and strategies, without implying
that removed Bootstrapper-specific Git URL handling was relocated.
In `@src/fromager/sources.py`:
- Around line 68-105: Add docstrings to the public functions get_source_type and
download_source in src/fromager/sources.py, describing their purpose,
parameters, and return values. Keep their existing behavior and implementation
unchanged.
🪄 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: 9519e03e-79d3-4fcd-aa0f-f167c7f073ea
📒 Files selected for processing (26)
docs/customization.mddocs/example/git-submodules-example.yamldocs/example/requirements-git-example.txtdocs/how-tos/build-from-git-repo.rstdocs/how-tos/index.rste2e/ci_bootstrap_parallel_suite.she2e/ci_bootstrap_suite.she2e/ci_build_suite.she2e/test_bootstrap_git_url.she2e/test_bootstrap_git_url_tag.she2e/test_bootstrap_parallel_git_url.she2e/test_bootstrap_parallel_git_url_tag.she2e/test_build_sequence_git_url.she2e/test_version_env_git_url.shsrc/fromager/bootstrap_requirement_resolver.pysrc/fromager/bootstrapper/_bootstrapper.pysrc/fromager/commands/build.pysrc/fromager/constraints.pysrc/fromager/dependency_graph.pysrc/fromager/requirements_file.pysrc/fromager/sources.pytests/test_bootstrap.pytests/test_bootstrap_requirement_resolver.pytests/test_constraints.pytests/test_packagesettings.pytests/test_sources.py
💤 Files with no reviewable changes (18)
- docs/example/requirements-git-example.txt
- docs/example/git-submodules-example.yaml
- e2e/test_bootstrap_git_url_tag.sh
- e2e/test_bootstrap_parallel_git_url_tag.sh
- e2e/test_bootstrap_git_url.sh
- docs/how-tos/index.rst
- e2e/test_bootstrap_parallel_git_url.sh
- docs/how-tos/build-from-git-repo.rst
- e2e/ci_build_suite.sh
- e2e/test_version_env_git_url.sh
- e2e/test_build_sequence_git_url.sh
- e2e/ci_bootstrap_parallel_suite.sh
- src/fromager/requirements_file.py
- src/fromager/dependency_graph.py
- e2e/ci_bootstrap_suite.sh
- tests/test_sources.py
- tests/test_bootstrap_requirement_resolver.py
- src/fromager/bootstrapper/_bootstrapper.py
7095d4b to
4aa3efe
Compare
dhellmann
left a comment
dhellmann
left a comment
There was a problem hiding this comment.
This is making the code so much simpler, thank you. I had a couple of small questions inline.
c32c6f6 to
f46742c
Compare
|
I'm happy with this version, but I'll leave it open for the rest of the team to review. |
Remove PEP 508 direct reference URL support (`pkg @ git+https://...`) which was deprecated in proposal python-wheel-build#1225. The `source` provider config system covers all use cases declaratively without special-casing. This removes: - 19 `if req.url:` branches across 6 source files - `_resolve_version_from_git_url()`, `_get_version_from_package_metadata()`, `_prepare_build_dependencies()`, `_handle_build_requirements()` methods - `SourceType.GIT` enum value and `get_top_level_requirement()` method - `extend_known_versions()` public method and git URL guard in resolver - 6 e2e test scripts, 11 unit tests, and all git URL documentation Retained: `gitutils.py` and `downloads.download_git_source()` which are used by git-clone source providers (`pypi-git`, `github-tag-git`, etc.). Closes: python-wheel-build#1220 Closes: python-wheel-build#1225 Co-Authored-By: Claude <claude@anthropic.com> Signed-off-by: Rohan Devasthale <rdevasth@redhat.com> Co-Authored-By: Claude <claude@anthropic.com> Signed-off-by: Rohan Devasthale <rdevasth@redhat.com>
27a94b4 to
ecede64
Compare
3 participants
Remove PEP 508 direct reference URL support (
pkg @ git+https://...) which was deprecated in proposal #1225. Thesourceprovider config system covers all use cases declaratively without special-casing.This removes:
if req.url:branches across 6 source files_resolve_version_from_git_url(),_get_version_from_package_metadata(),_prepare_build_dependencies(),_handle_build_requirements()methodsSourceType.GITenum value andget_top_level_requirement()methodextend_known_versions()public method and git URL guard in resolverRetained:
gitutils.pyanddownloads.download_git_source()which are used by git-clone source providers (pypi-git,github-tag-git, etc.).Closes: #1220
Closes: #1225