Skip to content

feat(resolver): wire source_resolver into resolver and download pipelines #1202

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
smoparth wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat(resolver): wire source_resolver into resolver and download pipelines #1202

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 22 additions & 7 deletions src/fromager/packagesettings/_models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
from pydantic import AnyUrl, Field
from pydantic_core import core_schema

# from ._resolver import SourceResolver
from ._resolver import SourceResolver
from ._typedefs import (
MODEL_CONFIG,
BuildDirectory,
Expand Down Expand Up @@ -324,9 +324,10 @@ class VariantInfo(pydantic.BaseModel):
pre_built: bool = False
"""Use pre-built wheel from index server?"""

# TODO
# source: SourceResolver | None
# """Source resolver and downloader"""
source: typing.Annotated[
SourceResolver | None,
pydantic.Field(default=None, discriminator="provider"),
] = None


class GitOptions(pydantic.BaseModel):
Expand All @@ -336,6 +337,7 @@ class GitOptions(pydantic.BaseModel):

submodules: False
submodule_paths: []
remove_dot_git: True

@LalatenduMohanty LalatenduMohanty Jun 23, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick : The class docstring example shows remove_dot_git: True, but the default is False , so we should change the dc string to false

"""

model_config = MODEL_CONFIG
Expand All @@ -358,6 +360,18 @@ class GitOptions(pydantic.BaseModel):
- ["vendor/lib1", "vendor/lib2"]
"""

remove_dot_git: bool = False
"""Remove ``.git`` directory after cloning?

When True, the ``.git`` directory is removed from the cloned source
tree so it does not end up in the built sdist. Defaults to False
to preserve backward compatibility with existing ``req.url`` git
clones that rely on ``.git`` for version detection (e.g. via
setuptools-scm).

.. versionadded:: 0.85

@LalatenduMohanty LalatenduMohanty Jun 23, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you meant 0.86 ?

"""


_DictStrAny = dict[str, typing.Any]

Expand Down Expand Up @@ -452,9 +466,10 @@ class PackageSettings(pydantic.BaseModel):
project_override: ProjectOverride = Field(default_factory=ProjectOverride)
"""Patch project settings"""

# TODO
# source: SourceResolver | None
# """Source resolver and downloader"""
source: typing.Annotated[
SourceResolver | None,
pydantic.Field(default=None, discriminator="provider"),
] = None

variants: Mapping[Variant, VariantInfo] = Field(default_factory=dict)
"""Variant configuration"""
Expand Down
13 changes: 13 additions & 0 deletions src/fromager/packagesettings/_pbi.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@

if typing.TYPE_CHECKING:
from .. import build_environment
from ._resolver import SourceResolver
from ._settings import Settings

logger = logging.getLogger(__name__)
Expand Down Expand Up @@ -176,6 +177,18 @@ def pre_built(self) -> bool:
return vi.pre_built
return False

@property
def source_resolver(self) -> SourceResolver | None:
"""Effective source resolver for this package and variant.

Returns the variant-level ``source`` override if set, otherwise
the package-level ``source``, or ``None`` when neither is configured.
"""
vi = self._ps.variants.get(self._variant)
if vi is not None and vi.source is not None:
return vi.source
return self._ps.source

@property
def wheel_server_url(self) -> str | None:
"""Alternative package index for pre-build wheel"""
Expand Down
25 changes: 17 additions & 8 deletions src/fromager/resolver.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,14 +119,23 @@ def default_resolver_provider(
include_wheels: bool,
req_type: RequirementType | None = None,
ignore_platform: bool = False,
) -> (
PyPIProvider
| GenericProvider
| GitHubTagProvider
| GitLabTagProvider
| VersionMapProvider
):
"""Lookup resolver provider to resolve package versions"""
) -> BaseProvider:
"""Lookup resolver provider to resolve package versions.

When the package has a ``source`` configuration, the provider is
created from the declarative resolver model. Otherwise the default
``PyPIProvider`` is returned.
"""
pbi = ctx.package_build_info(req)
source = pbi.source_resolver
if source is not None:
logger.info(
"%s: using source resolver provider %r",
req.name,
source.provider,
)
return source.resolver_provider(ctx, typing.cast(RequirementType, req_type))

Comment on lines +129 to +138

@coderabbitai coderabbitai Bot Jun 22, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Avoid passing optional req_type through a type cast in the source-resolver path.

Line 137 casts req_type but does not enforce it at runtime; None can still flow into source.resolver_provider(...) (non-optional contract), causing runtime failures in provider logic.

Suggested fix 
     if source is not None:
+        if req_type is None:
+            raise ValueError(
+                f"req_type must be set when using source resolver for {req.name}"
+            )
         logger.info(
             "%s: using source resolver provider %r",
             req.name,
             source.provider,
         )
-        return source.resolver_provider(ctx, typing.cast(RequirementType, req_type))
+        return source.resolver_provider(ctx, req_type)
📝 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
pbi = ctx.package_build_info(req)
source = pbi.source_resolver
if source is not None:
logger.info(
"%s: using source resolver provider %r",
req.name,
source.provider,
)
return source.resolver_provider(ctx, typing.cast(RequirementType, req_type))
pbi = ctx.package_build_info(req)
source = pbi.source_resolver
if source is not None:
if req_type is None:
raise ValueError(
f"req_type must be set when using source resolver for {req.name}"
)
logger.info(
"%s: using source resolver provider %r",
req.name,
source.provider,
)
return source.resolver_provider(ctx, 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/resolver.py` around lines 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.

return PyPIProvider(
include_sdists=include_sdists,
include_wheels=include_wheels,
Expand Down
55 changes: 50 additions & 5 deletions src/fromager/sources.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,30 @@ def resolve_source(
raise


def _is_git_url(url: str) -> bool:
"""Return True if *url* is a VCS-style ``git+https://`` or ``git+ssh://`` URL."""
return url.startswith("git+https://") or url.startswith("git+ssh://")


def _parse_git_url(url: str) -> tuple[str, str | None]:
"""Split a VCS URL into clone URL and optional ref.

``git+https://host/repo@ref`` -> ``(https://host/repo, ref)``
``git+https://host/repo`` -> ``(https://host/repo, None)``
"""
clone_url = url
if clone_url.startswith("git+"):
clone_url = clone_url[len("git+") :]

parsed = urlparse(clone_url)
ref: str | None = None
if "@" in parsed.path:
new_path, _, ref = parsed.path.rpartition("@")
clone_url = parsed._replace(path=new_path).geturl()

return clone_url, ref
Comment on lines +207 to +223

@coderabbitai coderabbitai Bot Jun 22, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Strip query/fragment from git clone URL in _parse_git_url.

Line 221 can return clone URLs that still contain query/fragment data (for example #subdirectory=...), which is not a valid clone target and can break download_git_source().

Suggested fix 
 def _parse_git_url(url: str) -> tuple[str, str | None]:
@@
-    parsed = urlparse(clone_url)
+    parsed = urlparse(clone_url)
+    # Not part of git clone target
+    parsed = parsed._replace(params="", query="", fragment="")
     ref: str | None = None
     if "@" in parsed.path:
         new_path, _, ref = parsed.path.rpartition("@")
         clone_url = parsed._replace(path=new_path).geturl()
+    else:
+        clone_url = parsed.geturl()
 
     return clone_url, ref
🤖 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 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.



def default_download_source(
ctx: context.WorkContext,
req: Requirement,
Expand All @@ -208,10 +232,24 @@ def default_download_source(
) -> pathlib.Path:
"Download the requirement and return the name of the output path."
pbi = ctx.package_build_info(req)
destination_filename = pbi.download_source_destination_filename(version=version)
url = pbi.download_source_url(version=version, default=download_url)
if url is None:
raise ValueError(f"Could not determine download URL for {req}")

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}"
download_path.mkdir(parents=True, exist_ok=True)
download_git_source(
ctx=ctx,
req=req,
url_to_clone=clone_url,
destination_dir=download_path,
ref=ref,
)
return download_path

destination_filename = pbi.download_source_destination_filename(version=version)
if destination_filename is None:
url_filename = resolver.extract_filename_from_url(url)
if url_filename.endswith(".zip"):
Expand Down Expand Up @@ -239,21 +277,22 @@ def download_git_source(
destination_dir: pathlib.Path,
ref: str | None = None,
) -> None:
"""Clone a git repository into *destination_dir*.

Applies ``git_options`` from the package settings (submodules,
``remove_dot_git``).
"""
if url_to_clone.startswith("git+"):
url_to_clone = url_to_clone[len("git+") :]

logger.info(f"cloning source from {url_to_clone}@{ref} to {destination_dir}")
# Get git options from package settings
pbi = ctx.package_build_info(req)
git_opts = pbi.git_options

# Configure submodules based on package settings
submodules: bool | list[str] = False
if git_opts.submodule_paths:
# If specific paths are configured, use those
submodules = git_opts.submodule_paths
elif git_opts.submodules:
# If general submodule support is enabled, clone all submodules
submodules = True

gitutils.git_clone(
Expand All @@ -265,6 +304,12 @@ def download_git_source(
ref=ref,
)

if git_opts.remove_dot_git:
dot_git = destination_dir / ".git"
if dot_git.exists():
logger.info("removing %s", dot_git)
shutil.rmtree(dot_git)


# Helper method to check whether .zip /.tar / .tgz is able to extract and check its content.
# It will throw exception if any other file is encountered. Eg: index.html
Expand Down
10 changes: 10 additions & 0 deletions tests/test_packagesettings.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@
"git_options": {
"submodules": False,
"submodule_paths": [],
"remove_dot_git": False,
},
"name": "test-pkg",
"has_config": True,
Expand All @@ -88,6 +89,7 @@
"use_pypi_org_metadata": True,
"min_release_age": None,
},
"source": None,
"variants": {
"cpu": {
"annotations": {
Expand All @@ -96,6 +98,7 @@
"env": {"EGG": "spam ${EGG}", "EGG_AGAIN": "$EGG"},
"wheel_server_url": "https://wheel.test/simple",
"pre_built": False,
"source": None,
},
"rocm": {
"annotations": {
Expand All @@ -104,12 +107,14 @@
"env": {"SPAM": ""},
"wheel_server_url": None,
"pre_built": True,
"source": None,
},
"cuda": {
"annotations": None,
"env": {},
"wheel_server_url": None,
"pre_built": False,
"source": None,
},
},
}
Expand All @@ -134,6 +139,7 @@
"git_options": {
"submodules": False,
"submodule_paths": [],
"remove_dot_git": False,
},
"has_config": True,
"purl": None,
Expand All @@ -150,6 +156,7 @@
"use_pypi_org_metadata": None,
"min_release_age": None,
},
"source": None,
"variants": {},
}

Expand All @@ -175,6 +182,7 @@
"git_options": {
"submodules": False,
"submodule_paths": [],
"remove_dot_git": False,
},
"has_config": True,
"purl": None,
Expand All @@ -191,12 +199,14 @@
"use_pypi_org_metadata": None,
"min_release_age": None,
},
"source": None,
"variants": {
"cpu": {
"annotations": None,
"env": {},
"pre_built": True,
"wheel_server_url": None,
"source": None,
},
},
}
Expand Down
Loading
Loading