feat(resolver): wire source_resolver into resolver and download pipelines#1202
Open
smoparth wants to merge 2 commits into
Open
feat(resolver): wire source_resolver into resolver and download pipelines#1202smoparth wants to merge 2 commits into
smoparth wants to merge 2 commits into
Conversation
📝 WalkthroughWalkthroughThis PR wires the source resolver model layer into runtime resolution and source downloading. It adds Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 (1)
src/fromager/packagesettings/_pbi.py (1)
180-186: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd
versionaddedmetadata to the new publicsource_resolverproperty.This accessor is a new user-facing API surface and should carry a Sphinx
.. versionadded:: 0.85directive for release-doc consistency.Suggested patch
def source_resolver(self) -> SourceResolver | None: """Effective source resolver for this package and variant. + .. versionadded:: 0.85 + Returns the variant-level ``source`` override if set, otherwise the package-level ``source``, or ``None`` when neither is configured. """As per coding guidelines, "Use Sphinx
versionadded,versionremoved,versionchangeddirectives for user-facing changes".🤖 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/packagesettings/_pbi.py` around lines 180 - 186, The `source_resolver` property in the _pbi.py file is a new public API but is missing the required Sphinx version metadata. Add a `.. versionadded:: 0.85` directive to the docstring of the `source_resolver` property to document when this accessor was introduced and maintain consistency with coding guidelines for user-facing API changes.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/resolver.py`:
- Around line 129-138: The code uses typing.cast to convert req_type to
RequirementType without enforcing that req_type is actually non-None at runtime.
Since source.resolver_provider() requires a non-optional RequirementType
argument, add a runtime validation check before the return statement in the
source resolver block to ensure req_type is not None. If it is None, either
raise an appropriate error or handle it appropriately so that None is never
passed to the source.resolver_provider() call.
In `@src/fromager/sources.py`:
- Around line 207-223: The `_parse_git_url` function returns clone URLs that may
still contain query parameters and fragment identifiers (such as
`#subdirectory`=...), which are invalid for git cloning. When reconstructing the
clone URL in the `_parse_git_url` function using the `_replace()` method on the
parsed URL object before calling `geturl()`, you need to also clear the query
and fragment components by setting them to empty strings in addition to updating
the path. This ensures the returned clone URL contains only the protocol, host,
and path components needed for git operations.
In `@tests/test_source_resolver_wiring.py`:
- Around line 74-77: Replace all real domain names in test URL fixtures with
their `.test` equivalents throughout the test file. Specifically, change domains
like github.com to github.test, pypi.org to pypi.test, and any other real
domains to their corresponding .test TLD versions. This applies to test URL
literals used in test methods including test_is_git_url_ssh and other test
methods that construct URLs as part of their fixtures, ensuring consistent
adherence to test isolation conventions.
---
Nitpick comments:
In `@src/fromager/packagesettings/_pbi.py`:
- Around line 180-186: The `source_resolver` property in the _pbi.py file is a
new public API but is missing the required Sphinx version metadata. Add a `..
versionadded:: 0.85` directive to the docstring of the `source_resolver`
property to document when this accessor was introduced and maintain consistency
with coding guidelines for user-facing API changes.
🪄 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: 0a276ad7-135e-47ad-af4f-351f66a07123
📒 Files selected for processing (6)
src/fromager/packagesettings/_models.pysrc/fromager/packagesettings/_pbi.pysrc/fromager/resolver.pysrc/fromager/sources.pytests/test_packagesettings.pytests/test_source_resolver_wiring.py
smoparth
force-pushed
the
feat/wire-source-resolver
branch
from
fac1b7f to
55aca35
Compare
This was referenced Jun 22, 2026
Member
|
@tiran PTAL |
LalatenduMohanty
requested changes
Jun 23, 2026
Member
|
The PR looks good to me overall. Lets see if we get review from @tiran . |
tiran
reviewed
Jun 24, 2026
| return results[0] | ||
|
|
||
|
|
||
| def default_resolver_provider( |
Collaborator
There was a problem hiding this comment.
Is default_resolver_provider the right place to hook into source resolver? How are you going to implement https://fromager.readthedocs.io/en/latest/proposals/new-resolver-config.html#default-behavior-and-hooks ?
Contributor
Author
There was a problem hiding this comment.
Created a separate commit refactor(resolver): check source config before plugin lookup to move the source_resolver check before overrides.find_and_invoke in resolve(), get_source_provider(), and download_source().
When provider != "hook", YAML config is used directly, plugins are skipped for both resolver and download. When provider == "hook" or no source config, the old plugin system runs as before. Let me know what you think.
"at least one source setting must use provider: hook when a plugin exists" needs a runtime check since plugin discovery isn't available at config load time. I'm not sure what to do here, need help with this.
|
|
||
| if _is_git_url(url): | ||
| clone_url, ref = _parse_git_url(url) | ||
| download_path = ctx.work_dir / f"{req.name}-{version}" / f"{req.name}-{version}" |
Collaborator
There was a problem hiding this comment.
Isn't that the same path as sdists_downloads_dir?
Contributor
Author
There was a problem hiding this comment.
My understanding is sdists_downloads_dir is for downloaded archive files (.tar.gz, .zip). Git clones produce a directory, not an archive, so they go to ctx.work_dir, same path the existing req.url git clone uses.
smoparth
force-pushed
the
feat/wire-source-resolver
branch
from
55aca35 to
3474a69
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/sources.py`:
- Around line 239-247: The git URL branch in default_download_source() is
ignoring configured submodule settings because it calls
gitutils.git_clone_fast(), which does not take submodules arguments. Update this
path to use gitutils.git_clone(..., submodules=...) with the parsed
git_options.submodules and git_options.submodule_paths, or extend
git_clone_fast() to accept and forward those options so submodules are honored.
Use the default_download_source() and
gitutils.git_clone_fast()/gitutils.git_clone symbols to locate the change.
In `@tests/test_source_resolver_wiring.py`:
- Around line 279-302: The default `remove_dot_git` tests are asserting the
opposite of the intended behavior, so update the assertions in
`test_keeps_dot_git_by_default` and the related default-check test to match the
PR’s default-`True` behavior. Use `sources.download_git_source` as the reference
point and change the expectations so the default path verifies `.git` is removed
unless `remove_dot_git=False` is explicitly passed, keeping the tests aligned
with the new contract.
- Around line 76-77: The test `test_is_git_url_ssh_not_supported` is asserting
the wrong contract for `sources._is_git_url`; update it so `git+ssh://` is
expected to be treated as a supported git URL, alongside `git+https://`, and
rename the test if needed to reflect the new behavior. Adjust the assertion in
this test module to match the runtime routing through git cloning rather than
marking SSH URLs unsupported.
🪄 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: ba7175d4-59ef-4683-8272-4f1fa0edcb2b
📒 Files selected for processing (6)
src/fromager/packagesettings/_models.pysrc/fromager/packagesettings/_pbi.pysrc/fromager/resolver.pysrc/fromager/sources.pytests/test_packagesettings.pytests/test_source_resolver_wiring.py
✅ Files skipped from review due to trivial changes (1)
- tests/test_packagesettings.py
🚧 Files skipped from review as they are similar to previous changes (2)
- src/fromager/packagesettings/_pbi.py
- src/fromager/resolver.py
smoparth
force-pushed
the
feat/wire-source-resolver
branch
from
3474a69 to
2284b46
Compare
Contributor
|
This pull request has merge conflicts that must be resolved before it can be merged. |
…ines Activate the `source` field on `PackageSettings` and `VariantInfo` so YAML-configured providers are used by the resolver and download pipelines. This is the wiring layer for the model classes introduced in PR python-wheel-build#1052. - Uncomment `source: SourceResolver | None` in `PackageSettings` and `VariantInfo` - Add `PackageBuildInfo.source_resolver` property (variant overrides package level) - `default_resolver_provider()` checks `pbi.source_resolver` before returning the default `PyPIProvider` - `default_download_source()` detects `git+https://` URL and routes to `git_clone_fast()` with automatic submodule handling - Add `GitOptions.remove_dot_git` field (default `False`) and removes `.git` recursively including submodules - Change `resolver_provider()` in `_resolver.py` to accept `RequirementType | None` for consistency with all Provider constructors; removes need for `typing.cast` - Update existing test snapshots and add new tests Closes: python-wheel-build#1048 Co-Authored-By: Claude <claude@anthropic.com> Signed-off-by: Shanmukh Pawan <smoparth@redhat.com>
Move the source_resolver dispatch from inside default_resolver_provider (which only runs when no plugin exists) to before overrides.find_and_invoke in resolve(), get_source_provider(), and download_source(). This ensures YAML source config takes priority over plugins for both resolver and download pipelines. When provider is "hook", the dispatch falls through to the plugin system. All other provider values bypass plugins entirely. Co-Authored-By: Claude <claude@anthropic.com> Signed-off-by: Shanmukh Pawan <smoparth@redhat.com>
smoparth
force-pushed
the
feat/wire-source-resolver
branch
from
9f9f8e2 to
c3031b1
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/resolver.py`:
- Around line 93-114: The resolver lookup in resolve() is bypassing plugin
execution when source.provider is not "hook", so package plugins never get a
chance to run. Move the source-resolver fallback logic into
default_resolver_provider() and make resolve() always call
overrides.find_and_invoke() first, using source.resolver_provider(ctx, req_type)
only as the fallback path when no plugin handles get_resolver_provider. Keep the
change centered around resolve(), default_resolver_provider(), and
source.resolver_provider so the lookup order is always plugin first, source
resolver second.
In `@src/fromager/sources.py`:
- Around line 89-110: The download flow in the source handling path is bypassing
configured plugins whenever a declarative source exists, so restore plugin
precedence by keeping overrides.find_and_invoke() as the entry point in the
source download logic. Update the source selection in the source/download path
around default_download_source and any equivalent fallback branch so configured
hooks still run first, and move the YAML/source_resolver fallback into the
default implementation rather than preempting the plugin dispatch.
In `@tests/test_source_resolver_wiring.py`:
- Around line 115-139: The dispatch coverage only checks source-config behavior,
so add a test in TestSourceResolverDispatch that patches the plugin lookup path
used by get_source_provider (for example find_and_invoke or the equivalent
override hook) and verifies it is chosen even when ctx contains a source
provider config. Assert the plugin result takes precedence over the
source-config-derived resolver path, using the existing get_source_provider and
_make_context_with_source helpers to locate the flow.
🪄 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: 8bd2c03f-72e7-4d08-9e22-382c90f3bf7a
📒 Files selected for processing (7)
src/fromager/packagesettings/_models.pysrc/fromager/packagesettings/_pbi.pysrc/fromager/packagesettings/_resolver.pysrc/fromager/resolver.pysrc/fromager/sources.pytests/test_packagesettings.pytests/test_source_resolver_wiring.py
✅ Files skipped from review due to trivial changes (1)
- tests/test_packagesettings.py
🚧 Files skipped from review as they are similar to previous changes (2)
- src/fromager/packagesettings/_pbi.py
- src/fromager/packagesettings/_resolver.py
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
3 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.
Activate the
sourcefield onPackageSettingsandVariantInfoso YAML-configured providers are used by the resolver and download pipelines. This is the wiring layer for the model classes introduced in PR #1052.source: SourceResolver | NoneinPackageSettingsandVariantInfoPackageBuildInfo.source_resolverproperty (variant overrides package level)default_download_source()detectsgit+https:///git+ssh://URLs and routes todownload_git_source()GitOptions.remove_dot_gitfield (defaultFalse) and removes.gitrecursively including submodulesresolver_provider()in_resolver.pyto acceptRequirementType | Nonefor consistency with all Provider constructorssource_resolver dispatch is added in resolve(), get_source_provider(), anddownload_source().
Closes: #1048
Pull Request Description
What
Why