feat: source resolver configuration#1052
Open
tiran wants to merge 1 commit intopython-wheel-build:mainfrom
Open
feat: source resolver configuration#1052tiran wants to merge 1 commit intopython-wheel-build:mainfrom
tiran wants to merge 1 commit intopython-wheel-build:mainfrom
Conversation
📝 WalkthroughWalkthroughAdds a source-resolver subsystem: a new Pydantic-backed ChangesSource-resolver subsystem & wiring
Sequence Diagram(s)sequenceDiagram
participant PBI as PackageBuildInfo
participant PS as PackageSettings
participant SR as SourceResolver (Pydantic model)
participant Prov as Resolver Provider (PyPI/GitHub/GitLab/VersionMap)
PBI->>PS: read package settings
PBI->>SR: resolve source_resolver (variant or package-level)
SR->>Prov: resolver_provider(ctx, req_type)
Prov->>Prov: prepare/provider-specific URL or clone info
Prov-->>PBI: provider instance for fetching versions/artifacts
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
Collaborator
Author
|
The tests are going to fail. I have not updated the test cases and test configs. |
tiran
force-pushed
the
new-resolver-config2
branch
from
599609d to
976deff
Compare
tiran
force-pushed
the
new-resolver-config2
branch
from
976deff to
19a9602
Compare
![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 (2)
src/fromager/packagesettings/_models.py (1)
338-342: 💤 Low value
sourcefield missing Python-level= None— may confuse type checkersPydantic v2 reads the
default=NonefromField()insideAnnotated, so runtime behavior is correct. However, unlike all other nullable fields inVariantInfoandPackageSettings(which use= Noneexplicitly), this field has no Python-level default assignment. Static type checkers (e.g., pyright) may flag it as a required field.♻️ Proposed fix (apply to both `VariantInfo` and `PackageSettings`)
- source: typing.Annotated[ - SourceResolver | None, - pydantic.Field(default=None, discriminator="provider"), - ] + source: typing.Annotated[ + SourceResolver | None, + pydantic.Field(default=None, discriminator="provider"), + ] = None🤖 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/_models.py` around lines 338 - 342, The Annotated field declaration for source in _models.py (the "source" field on VariantInfo and PackageSettings) lacks a Python-level default assignment which can make static type checkers treat it as required; update the field declaration to include an explicit Python default (add " = None" to the Annotated[...] expression for the source attribute in both VariantInfo and PackageSettings) while keeping the existing pydantic.Field(default=None, discriminator="provider") inside the Annotated so runtime behavior remains unchanged.src/fromager/packagesettings/__init__.py (1)
16-30: 💤 Low valueConsider exporting
SourceResolverfor external type annotationsThe
SourceResolverdiscriminated union is used as the field type inVariantInfoandPackageSettings, but it's not re-exported from the package's public API. External callers who want to annotate against it would have to reach intofromager.packagesettings._resolver.♻️ Proposed addition
from ._resolver import ( DEFAULT_TAG_MATCHER, BuildSDist, GitHubTagCloneResolver, GitHubTagDownloadResolver, GitLabTagCloneResolver, GitLabTagDownloadResolver, HookResolver, NotAvailableResolver, PyPIDownloadResolver, PyPIGitResolver, PyPIPrebuiltResolver, PyPISDistResolver, + SourceResolver, VersionMapResolver, )And add
"SourceResolver"to__all__.🤖 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/__init__.py` around lines 16 - 30, The package does not re-export the SourceResolver discriminated union used by VariantInfo and PackageSettings, forcing consumers to import from fromager.packagesettings._resolver; update src/fromager/packagesettings/__init__.py to import SourceResolver from ._resolver (alongside DEFAULT_TAG_MATCHER, BuildSDist, etc.) and add "SourceResolver" to the module's __all__ so callers can annotate against SourceResolver via fromager.packagesettings.SourceResolver.
🤖 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/packagesettings/_resolver.py`:
- Around line 247-281: VersionMapResolver currently relies on
AbstractResolver.resolver_provider which raises NotImplementedError; add an
explicit override on the VersionMapResolver class that either implements the
provider or immediately raises NotImplementedError("versionmap-git not yet
implemented") to make behavior explicit (i.e., define a resolver_provider method
on VersionMapResolver that raises the clear NotImplementedError string or
implements the provider logic), referencing the VersionMapResolver class and
AbstractResolver.resolver_provider to locate where to add the
stub/implementation.
- Around line 404-432: The resolver_provider methods currently pass the literal
"FIXME" as override_download_url which silently produces broken providers;
instead, update each resolver_provider (e.g.,
GitHubTagCloneResolver.resolver_provider and
GitHubTagDownloadResolver.resolver_provider, and do the same for
GitLabTagDownloadResolver and GitLabTagCloneResolver) to fail fast by raising
NotImplementedError with a clear message like "override_download_url template
not implemented for <ResolverClassName>" rather than calling
self._github_provider(...) (or self._gitlab_provider(...)) with the "FIXME"
string.
In `@tests/test_packagesettings.py`:
- Around line 944-975: The four commented-out assertions for matcher_factory
leave test coverage incomplete for the tag-based resolver fixtures (see
TEST_GITHUB_DOWNLOAD, TEST_GITHUB_GIT, TEST_GITLAB_DOWNLOAD, TEST_GITLAB_GIT);
either uncomment the matcher_factory checks or explicitly assert the serialized
JSON omits matcher_factory depending on intended behavior: if matcher_factory
should be present assert it equals
"fromager.packagesettings:DEFAULT_TAG_MATCHER" for each of those fixtures,
otherwise update the test to assert the key is absent/None and remove the
commented lines to reflect the intended serialization.
---
Nitpick comments:
In `@src/fromager/packagesettings/__init__.py`:
- Around line 16-30: The package does not re-export the SourceResolver
discriminated union used by VariantInfo and PackageSettings, forcing consumers
to import from fromager.packagesettings._resolver; update
src/fromager/packagesettings/__init__.py to import SourceResolver from
._resolver (alongside DEFAULT_TAG_MATCHER, BuildSDist, etc.) and add
"SourceResolver" to the module's __all__ so callers can annotate against
SourceResolver via fromager.packagesettings.SourceResolver.
In `@src/fromager/packagesettings/_models.py`:
- Around line 338-342: The Annotated field declaration for source in _models.py
(the "source" field on VariantInfo and PackageSettings) lacks a Python-level
default assignment which can make static type checkers treat it as required;
update the field declaration to include an explicit Python default (add " =
None" to the Annotated[...] expression for the source attribute in both
VariantInfo and PackageSettings) while keeping the existing
pydantic.Field(default=None, discriminator="provider") inside the Annotated so
runtime behavior remains 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: 42e22219-5705-4c79-b697-560c94d2a617
📒 Files selected for processing (14)
docs/reference/config-reference.rstsrc/fromager/packagesettings/__init__.pysrc/fromager/packagesettings/_models.pysrc/fromager/packagesettings/_pbi.pysrc/fromager/packagesettings/_resolver.pytests/test_packagesettings.pytests/testdata/context/overrides/settings/test_github-download.yamltests/testdata/context/overrides/settings/test_github-git.yamltests/testdata/context/overrides/settings/test_gitlab-download.yamltests/testdata/context/overrides/settings/test_gitlab-git.yamltests/testdata/context/overrides/settings/test_pypidownload.yamltests/testdata/context/overrides/settings/test_pypigit.yamltests/testdata/context/overrides/settings/test_pypiprebuilt.yamltests/testdata/context/overrides/settings/test_pypisdist.yaml
tiran
force-pushed
the
new-resolver-config2
branch
from
19a9602 to
4c9f9fb
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/fromager/packagesettings/_resolver.py (1)
519-523:HookResolveris unimplemented — offer to helpThe
TODOandraise NotImplementedErrormean any config usingprovider: hookwill always fail.Happy to implement the hook-dispatch logic if you want to open a tracking issue for it.
🤖 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/_resolver.py` around lines 519 - 523, The resolver_provider method currently raises NotImplementedError for provider dispatch; implement it to return the hook provider when config requests provider: "hook" by instantiating and returning the HookResolver (or the project's resolver.HookResolver) with the provided ctx and req_type, otherwise delegate to the existing default provider factory/logic—replace the TODO/raise with a conditional that constructs resolver.HookResolver(ctx, req_type) for hook providers and falls back to the existing resolver selection path (returning an instance of resolver.BaseProvider).
🤖 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/packagesettings/_resolver.py`:
- Around line 439-446: The override_download_url passed from
GitHubTagCloneResolver.resolver_provider and
GitLabTagCloneResolver.resolver_provider currently uses a plain string with
"{self.project_url}" / "{self.clone_url}" which causes str.format to look up a
nonexistent "self" key; change those literals to f-strings (prefix with f) so
self.project_url or self.clone_url is interpolated first (e.g.,
f"git+{self.project_url}@{{tagname}}" and f"git+{self.clone_url}@{{tagname}}")
before the provider calls .format(...).
---
Nitpick comments:
In `@src/fromager/packagesettings/_resolver.py`:
- Around line 519-523: The resolver_provider method currently raises
NotImplementedError for provider dispatch; implement it to return the hook
provider when config requests provider: "hook" by instantiating and returning
the HookResolver (or the project's resolver.HookResolver) with the provided ctx
and req_type, otherwise delegate to the existing default provider
factory/logic—replace the TODO/raise with a conditional that constructs
resolver.HookResolver(ctx, req_type) for hook providers and falls back to the
existing resolver selection path (returning an instance of
resolver.BaseProvider).
🪄 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: 8fe16787-0813-4bdb-9b85-a04d5522f92d
📒 Files selected for processing (15)
docs/reference/config-reference.rstsrc/fromager/packagesettings/__init__.pysrc/fromager/packagesettings/_models.pysrc/fromager/packagesettings/_pbi.pysrc/fromager/packagesettings/_resolver.pysrc/fromager/resolver.pytests/test_packagesettings.pytests/testdata/context/overrides/settings/test_github-download.yamltests/testdata/context/overrides/settings/test_github-git.yamltests/testdata/context/overrides/settings/test_gitlab-download.yamltests/testdata/context/overrides/settings/test_gitlab-git.yamltests/testdata/context/overrides/settings/test_pypidownload.yamltests/testdata/context/overrides/settings/test_pypigit.yamltests/testdata/context/overrides/settings/test_pypiprebuilt.yamltests/testdata/context/overrides/settings/test_pypisdist.yaml
✅ Files skipped from review due to trivial changes (11)
- tests/testdata/context/overrides/settings/test_gitlab-download.yaml
- tests/testdata/context/overrides/settings/test_pypidownload.yaml
- tests/testdata/context/overrides/settings/test_gitlab-git.yaml
- tests/testdata/context/overrides/settings/test_github-git.yaml
- tests/testdata/context/overrides/settings/test_pypisdist.yaml
- tests/testdata/context/overrides/settings/test_pypiprebuilt.yaml
- src/fromager/packagesettings/_pbi.py
- tests/testdata/context/overrides/settings/test_pypigit.yaml
- docs/reference/config-reference.rst
- src/fromager/packagesettings/_models.py
- tests/test_packagesettings.py
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/testdata/context/overrides/settings/test_github-download.yaml
- src/fromager/packagesettings/init.py
tiran
force-pushed
the
new-resolver-config2
branch
from
4c9f9fb to
5d955ff
Compare
rd4398
reviewed
May 5, 2026
Contributor
rd4398
left a comment
rd4398
left a comment
There was a problem hiding this comment.
This looks mostly okay. I have left a few comments / suggestions.
I will prefer if @LalatenduMohanty and / or @dhellmann takes a look as well
| raise ValueError("package is not available") | ||
|
|
||
|
|
||
| class HookResolver(pydantic.BaseModel): |
Contributor
There was a problem hiding this comment.
Same here,
it implements resolver_provider() with the same signature but aren't part of the type hierarchy
Can we make it inherit from AbstractResolver for consistency?
| def resolver_provider( | ||
| self, ctx: context.WorkContext, req_type: requirements_file.RequirementType | ||
| ) -> resolver.BaseProvider: | ||
| # TODO |
Contributor
There was a problem hiding this comment.
Users who configure provider: hook will get an unhelpful crash. How about we add an error message something like "hook resolver is not yet implemented"?
Introduce new configuration for source resolver and downloads. The new
system uses profiles for common tasks:
- `pypi-sdist`: resolve versions of sdists from PyPI, download sdist
- `pypi-prebuilt`: resolve versions of platform wheels from PyPI,
download pre-built wheel from PyPI.
- `pypi-download`: resolve versions of any package from PyPI, download
from external URL (with `{version}` variable)
- `pypi-git`: resolve versions for any package from PyPI, git clone
(with `{version}` variable)
- `versionmap-git`: resolve with a version map, git clone
- `github-tag-download`: resolve from GitHub tag, download tarball
- `github-tag-git`: resolve from GitHub tag, git clone
- `gitlab-tag-download`: resolve from GitLab tag, download tarball
- `gitlab-tag-git`: resolve from GitLab tag, git clone
- `not-available`: block resolving
- `hook`: call Fromager hooks
The new settings will eventually replace `download_source`,
`resolver_dist`, and `git_options` top-level options as well as
`wheel_server_url` and `pre_built` flags for variants.
Signed-off-by: Christian Heimes <cheimes@redhat.com>
tiran
force-pushed
the
new-resolver-config2
branch
from
5d955ff to
ac069e7
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/packagesettings/_models.py`:
- Around line 338-342: The new "source" field must be made mutually exclusive
with the legacy fields (download_source, resolver_dist, wheel_server_url,
pre_built): add a model-level validator (e.g., a pydantic root_validator or
`@model_validator`) on the class that declares the "source" field to check if
source is not None and any of
download_source/resolver_dist/wheel_server_url/pre_built are also set, and if so
raise a ValueError with a clear message; implement the same validator logic for
the other occurrence of the "source" field in this file so both model variants
reject mixed configs.
In `@src/fromager/packagesettings/_resolver.py`:
- Around line 376-392: The server_url built in _gitlab_provider drops an
explicit port because it uses self.project_url.host; update the server_url
construction so it preserves any configured port (e.g., use
self.project_url.port or self.project_url.netloc to include host:port) when
creating the resolver.GitLabTagProvider; keep project_path set via
self.project_url.path.lstrip("/") and pass the rest of the same arguments to
GitLabTagProvider.
- Around line 272-294: The version_map entries are being passed as raw refs
(e.g., "refs/tags/1.1") so they become Candidate.url values that aren't
fetchable and the default build_sdist=tarball doesn't match a git checkout flow;
update resolver_provider to transform version_map values that look like git refs
(e.g., start with "refs/" or match a tag/commit pattern) into concrete fetchable
Git URLs using the validated clone_url (combine clone_url and the ref into a
git+<clone_url>@<ref> URL or otherwise construct the provider-specific URL)
before creating versionmap.VersionMap(self.version_map), or use a Git-specific
provider that accepts clone_url + ref (e.g., switch to resolver.GitProvider or
pass package_name plus clone_url/ref into VersionMapProvider); also change the
class default build_sdist (build_sdist: typing.ClassVar[BuildSDist | None]) from
BuildSDist.tarball to a value consistent with git checkouts (e.g., None or the
git/checkout option) so sdist building behavior matches the git resolution flow.
- Around line 506-516: HookResolver currently advertises provider: "hook" but
leaves resolver_provider unimplemented, causing valid configs to pass validation
but crash at runtime; update HookResolver so the settings system rejects "hook"
at validation time by implementing a validation entry that raises a clear
ConfigError (or ValueError) during config parsing instead of
NotImplementedError. Locate the HookResolver class and its resolver_provider
method and either (a) remove/register it from the public SourceResolver union so
it isn't accepted, or (b) add a class-level validation hook (e.g., __post_init__
/ validate / from_config depending on your config framework) that checks
provider == "hook" and raises a configuration error with a helpful message
indicating the provider is not yet supported. Ensure references to HookResolver,
resolver_provider, and provider are used so the fix is applied in the right
place.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: f2a3dfa4-708b-48ab-a138-e0ac21eb10f5
📒 Files selected for processing (15)
docs/reference/config-reference.rstsrc/fromager/packagesettings/__init__.pysrc/fromager/packagesettings/_models.pysrc/fromager/packagesettings/_pbi.pysrc/fromager/packagesettings/_resolver.pysrc/fromager/resolver.pytests/test_packagesettings.pytests/testdata/context/overrides/settings/test_github-download.yamltests/testdata/context/overrides/settings/test_github-git.yamltests/testdata/context/overrides/settings/test_gitlab-download.yamltests/testdata/context/overrides/settings/test_gitlab-git.yamltests/testdata/context/overrides/settings/test_pypidownload.yamltests/testdata/context/overrides/settings/test_pypigit.yamltests/testdata/context/overrides/settings/test_pypiprebuilt.yamltests/testdata/context/overrides/settings/test_pypisdist.yaml
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/testdata/context/overrides/settings/test_github-download.yaml
- tests/testdata/context/overrides/settings/test_pypiprebuilt.yaml
- tests/testdata/context/overrides/settings/test_pypisdist.yaml
Comment on lines
+338
to
+342
| source: typing.Annotated[ | ||
| SourceResolver | None, | ||
| pydantic.Field(default=None, discriminator="provider"), | ||
| ] | ||
| """Source resolver and downloader""" |
There was a problem hiding this comment.
Make source mutually exclusive with the legacy source settings.
These models now allow source alongside download_source, resolver_dist, wheel_server_url, and pre_built. Since src/fromager/packagesettings/_pbi.py Line 181 reads the new resolver while the older accessors still exist, mixed configs can silently disagree about how a package should be resolved or downloaded.
Proposed fix
class VariantInfo(pydantic.BaseModel):
@@
source: typing.Annotated[
SourceResolver | None,
pydantic.Field(default=None, discriminator="provider"),
]
"""Source resolver and downloader"""
+
+ `@pydantic.model_validator`(mode="after")
+ def validate_source_conflicts(self) -> typing.Self:
+ if self.source is not None and (
+ self.wheel_server_url is not None or self.pre_built
+ ):
+ raise ValueError(
+ "'source' cannot be combined with 'wheel_server_url' or 'pre_built'"
+ )
+ return self
@@
class PackageSettings(pydantic.BaseModel):
@@
source: typing.Annotated[
SourceResolver | None,
pydantic.Field(default=None, discriminator="provider"),
]
"""Source resolver and downloader"""
+
+ `@pydantic.model_validator`(mode="after")
+ def validate_source_conflicts(self) -> typing.Self:
+ if self.source is not None:
+ if (
+ self.download_source.url is not None
+ or self.download_source.destination_filename is not None
+ or self.resolver_dist != ResolverDist()
+ ):
+ raise ValueError(
+ "'source' cannot be combined with legacy source settings"
+ )
+ return selfAlso applies to: 468-472
🤖 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/_models.py` around lines 338 - 342, The new
"source" field must be made mutually exclusive with the legacy fields
(download_source, resolver_dist, wheel_server_url, pre_built): add a model-level
validator (e.g., a pydantic root_validator or `@model_validator`) on the class
that declares the "source" field to check if source is not None and any of
download_source/resolver_dist/wheel_server_url/pre_built are also set, and if so
raise a ValueError with a clear message; implement the same validator logic for
the other occurrence of the "source" field in this file so both model variants
reject mixed configs.
Comment on lines
+272
to
+294
| version_map: dict[PackageVersion, str] | ||
|
|
||
| build_sdist: typing.ClassVar[BuildSDist | None] = BuildSDist.tarball | ||
|
|
||
| @pydantic.field_validator("clone_url", mode="after") | ||
| @classmethod | ||
| def validate_clone_url(cls, value: pydantic.AnyUrl) -> pydantic.AnyUrl: | ||
| if value.scheme not in {"https", "ssh"}: | ||
| raise ValueError(f"invalid scheme in url {value}") | ||
| if not value.path: | ||
| raise ValueError(f"url {value} has an empty path") | ||
| return value | ||
|
|
||
| def resolver_provider( | ||
| self, ctx: context.WorkContext, req_type: requirements_file.RequirementType | ||
| ) -> resolver.BaseProvider: | ||
| return resolver.VersionMapProvider( | ||
| # PackageVersion is a subclass of Version | ||
| version_map=versionmap.VersionMap(self.version_map), # type: ignore[arg-type] | ||
| package_name=None, | ||
| constraints=ctx.constraints, | ||
| req_type=req_type, | ||
| ) |
There was a problem hiding this comment.
versionmap-git currently resolves to refs, not fetchable source URLs.
resolver.VersionMapProvider treats each map value as the final Candidate.url in src/fromager/resolver.py Line 1327, but this resolver passes raw commit/tag refs from version_map and never uses clone_url. That means a config like refs/tags/1.1 becomes the download URL, so the new profile cannot fetch sources. The default build_sdist=tarball is also inconsistent with a git checkout flow.
Proposed fix
class VersionMapResolver(AbstractResolver):
@@
- build_sdist: typing.ClassVar[BuildSDist | None] = BuildSDist.tarball
+ build_sdist: typing.ClassVar[BuildSDist | None] = BuildSDist.pep517
@@
def resolver_provider(
self, ctx: context.WorkContext, req_type: requirements_file.RequirementType
) -> resolver.BaseProvider:
+ mapped_urls = {
+ version: f"git+{self.clone_url}@{ref}"
+ for version, ref in self.version_map.items()
+ }
return resolver.VersionMapProvider(
- # PackageVersion is a subclass of Version
- version_map=versionmap.VersionMap(self.version_map), # type: ignore[arg-type]
+ version_map=versionmap.VersionMap(mapped_urls),
package_name=None,
constraints=ctx.constraints,
req_type=req_type,
)🤖 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/_resolver.py` around lines 272 - 294, The
version_map entries are being passed as raw refs (e.g., "refs/tags/1.1") so they
become Candidate.url values that aren't fetchable and the default
build_sdist=tarball doesn't match a git checkout flow; update resolver_provider
to transform version_map values that look like git refs (e.g., start with
"refs/" or match a tag/commit pattern) into concrete fetchable Git URLs using
the validated clone_url (combine clone_url and the ref into a
git+<clone_url>@<ref> URL or otherwise construct the provider-specific URL)
before creating versionmap.VersionMap(self.version_map), or use a Git-specific
provider that accepts clone_url + ref (e.g., switch to resolver.GitProvider or
pass package_name plus clone_url/ref into VersionMapProvider); also change the
class default build_sdist (build_sdist: typing.ClassVar[BuildSDist | None]) from
BuildSDist.tarball to a value consistent with git checkouts (e.g., None or the
git/checkout option) so sdist building behavior matches the git resolution flow.
Comment on lines
+376
to
+392
| def _gitlab_provider( | ||
| self, | ||
| *, | ||
| ctx: context.WorkContext, | ||
| req_type: requirements_file.RequirementType, | ||
| override_download_url: str | None = None, | ||
| ) -> resolver.GitLabTagProvider: | ||
| if not self.project_url.path: | ||
| raise ValueError(f"Empty path in {self.project_url}") | ||
| return resolver.GitLabTagProvider( | ||
| project_path=self.project_url.path.lstrip("/"), | ||
| server_url=f"https://{self.project_url.host}", | ||
| constraints=ctx.constraints, | ||
| matcher=self._get_matcher(ctx), | ||
| req_type=req_type, | ||
| override_download_url=override_download_url, | ||
| ) |
There was a problem hiding this comment.
Preserve the configured GitLab port when building server_url.
self.project_url.host drops any explicit port. A config like https://gitlab.example:8443/group/proj will therefore query https://gitlab.example/api/v4/... and generate tarball URLs on the wrong endpoint.
Proposed fix
return resolver.GitLabTagProvider(
project_path=self.project_url.path.lstrip("/"),
- server_url=f"https://{self.project_url.host}",
+ server_url=(
+ f"https://{self.project_url.host}:{self.project_url.port}"
+ if self.project_url.port is not None
+ else f"https://{self.project_url.host}"
+ ),
constraints=ctx.constraints,
matcher=self._get_matcher(ctx),
req_type=req_type,
override_download_url=override_download_url,
)🤖 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/_resolver.py` around lines 376 - 392, The
server_url built in _gitlab_provider drops an explicit port because it uses
self.project_url.host; update the server_url construction so it preserves any
configured port (e.g., use self.project_url.port or self.project_url.netloc to
include host:port) when creating the resolver.GitLabTagProvider; keep
project_path set via self.project_url.path.lstrip("/") and pass the rest of the
same arguments to GitLabTagProvider.
Comment on lines
+506
to
+516
| class HookResolver(AbstractResolver): | ||
| """Call resolver_provider and download_source hook""" | ||
|
|
||
| provider: typing.Literal["hook"] | ||
|
|
||
| def resolver_provider( | ||
| self, ctx: context.WorkContext, req_type: requirements_file.RequirementType | ||
| ) -> resolver.BaseProvider: | ||
| # TODO | ||
| raise NotImplementedError("Hook resolver needs a hook") | ||
|
|
There was a problem hiding this comment.
Don’t accept provider: hook until the resolver exists.
This class is part of the public SourceResolver union, so config validation succeeds, but every use still ends in NotImplementedError. That turns a valid-looking settings file into a runtime crash instead of a validation error.
Proposed fix
class HookResolver(AbstractResolver):
"""Call resolver_provider and download_source hook"""
provider: typing.Literal["hook"]
+ `@pydantic.model_validator`(mode="after")
+ def validate_supported(self) -> typing.Self:
+ raise ValueError("'hook' source resolver is not implemented yet")
+
def resolver_provider(
self, ctx: context.WorkContext, req_type: requirements_file.RequirementType
) -> resolver.BaseProvider:
- # TODO
- raise NotImplementedError("Hook resolver needs a hook")
+ raise AssertionError("unreachable")🤖 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/_resolver.py` around lines 506 - 516,
HookResolver currently advertises provider: "hook" but leaves resolver_provider
unimplemented, causing valid configs to pass validation but crash at runtime;
update HookResolver so the settings system rejects "hook" at validation time by
implementing a validation entry that raises a clear ConfigError (or ValueError)
during config parsing instead of NotImplementedError. Locate the HookResolver
class and its resolver_provider method and either (a) remove/register it from
the public SourceResolver union so it isn't accepted, or (b) add a class-level
validation hook (e.g., __post_init__ / validate / from_config depending on your
config framework) that checks provider == "hook" and raises a configuration
error with a helpful message indicating the provider is not yet supported.
Ensure references to HookResolver, resolver_provider, and provider are used so
the fix is applied in the right place.
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.
Pull Request Description
What
Introduce new configuration for source resolver and downloads. The new system uses profiles for common tasks:
pypi-sdist: resolve versions of sdists from PyPI, download sdistpypi-prebuilt: resolve versions of platform wheels from PyPI, download pre-built wheel from PyPI.pypi-download: resolve versions of any package from PyPI, download from external URL (with{version}variable)pypi-git: resolve versions for any package from PyPI, git clone (with{version}variable)versionmap-git: resolve with a version map, git clonegithub-tag-download: resolve from GitHub tag, download tarballgithub-tag-git: resolve from GitHub tag, git clonegitlab-tag-download: resolve from GitLab tag, download tarballgitlab-tag-git: resolve from GitLab tag, git clonenot-available: block resolvinghook: call Fromager hooksThe new settings will eventually replace
download_source,resolver_dist, andgit_optionstop-level options as well aswheel_server_urlandpre_builtflags for variants.Why
New resolver configuration with tag provider and git clone #937
[proposal] New resolver configuration #938
PR follows CONTRIBUTING.md guidelines
Summary by CodeRabbit
New Features
Documentation