Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 2 additions & 0 deletions docs/reference/config-reference.rst
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,8 @@ Source Resolver
.. autopydantic_model:: PyPIGitResolver
:inherited-members: AbstractPyPIResolver, CooldownMixin

.. autopydantic_model:: VersionMapGitResolver

.. autopydantic_model:: GitHubTagDownloadResolver
:inherited-members: AbstractGitSourceResolver, CooldownMixin

Expand Down
2 changes: 2 additions & 0 deletions src/fromager/packagesettings/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
PyPIGitResolver,
PyPIPrebuiltResolver,
PyPISDistResolver,
VersionMapGitResolver,
pep440_tag_matcher,
)
from ._settings import Settings, SettingsFile
Expand Down Expand Up @@ -82,6 +83,7 @@
"Variant",
"VariantChangelog",
"VariantInfo",
"VersionMapGitResolver",
"default_update_extra_environ",
"get_extra_environ",
"pep440_tag_matcher",
Expand Down
59 changes: 58 additions & 1 deletion src/fromager/packagesettings/_resolver.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@

from .. import resolver
from ..candidate import Cooldown
from ..versionmap import VersionMap
from ._typedefs import MODEL_CONFIG

if typing.TYPE_CHECKING:
Expand Down Expand Up @@ -233,7 +234,7 @@ class PyPIGitResolver(AbstractPyPIResolver):
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:
if not value.path or value.path == "/":

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[Recommendation]

  • src/fromager/packagesettings/_resolver.py (~line 237, PyPIGitResolver.validate_clone_url)
  • src/fromager/packagesettings/_resolver.py (~lines 518–523, VersionMapGitResolver.validate_clone_url)

The exact same check exists in two classes, word for word. This PR had to fix the same bug in both copies.

Fix: put the check in one shared helper function and have both classes call it.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[Recommendation]

The PR's stated goal is adding a new resolver, but it also alters the existing PyPIGitResolver by adding the value.path == "/" rejection — a behavior change to shipped code. The diff only adds an empty-path test to the new class, so the fix on PyPIGitResolver ships with no regression test and a later refactor could silently revert it.

Fix: split the PyPIGitResolver change into its own commit/PR with a dedicated test, or keep
it here but add test_clone_url_rejects_root_path under the PyPIGitResolver suite.

raise ValueError(f"url {value} has an empty path")
return value

Expand Down Expand Up @@ -482,6 +483,61 @@ def resolver_provider(
)


class VersionMapGitResolver(AbstractResolver):
"""Resolve version from a version map, build sdist from git clone.

The ``versionmap-git`` provider maps known version numbers to known git
refs (commit SHAs or ref paths such as ``refs/tags/1.1``). It clones a
git repo at the configured ref and builds an sdist with PEP 517.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[Nit]

The docstring says the provider "builds an sdist with PEP 517," which reads as fixed. But build_sdist is a configurable field where pep517 is only the default — the wording will mislead anyone configuring a
non-default build method.

Before:

git repo at the configured ref and builds an sdist with PEP 517.

After:

git repo at the configured ref and builds an sdist using the configured
``build_sdist`` method (default: PEP 517).


.. versionadded:: 0.79.0

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Why are we adding docstring annotation about when something was added? This is what release notest and git history are for.


Example::

provider: versionmap-git
clone_url: https://git.test/project/repo.git
build_sdist: pep517
versionmap:
'1.0': abad1dea
'1.1': refs/tags/1.1
"""

provider: typing.Literal["versionmap-git"]

clone_url: pydantic.AnyUrl
"""Git clone URL (``https`` or ``ssh`` scheme)."""

build_sdist: BuildSDist = BuildSDist.pep517
"""Source distribution build method."""

versionmap: dict[str, str]

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[Recommendation]src/fromager/packagesettings/_resolver.py (~line 513, versionmap field) and tests/test_packagesettings_resolver.py

An empty versionmap: {} passes validation, produces an empty VersionMap, and only fails later at resolve time with an opaque "no match" instead of a clear config error. There's also no positive test that the
ssh scheme is accepted (only the http-rejection negative), and no test for the empty-map case.

Reject an empty map with min_length=1, and add the two missing tests.

Before:

versionmap: dict[str, str]

After:

versionmap: dict[str, str] = pydantic.Field(min_length=1)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

There should also be a unit test case that ensure the empty versionmap dictionary fails.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[Nit]

The field name versionmap breaks snake_case consistency with its siblings (clone_url, build_sdist), and the code internally uses version_map/VersionMap. Because the field name is the user-facing YAML
key, the single-word form becomes a permanent API-consistency wart.

Before:

versionmap: dict[str, str]

After:

version_map: dict[str, str] = pydantic.Field(alias="versionmap")  # if the YAML key must stay

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[Question]

Are the versionmap keys validated as PEP 440 versions anywhere? They become version strings in VersionMap, so a typo key like 'v1.0' or 'latest' is accepted at config-load time and would only blow up
later when parsed as a Version. If VersionMap.__init__ doesn't validate keys eagerly, consider a field_validator calling packaging.version.Version(k) per key to fail fast.

"""Mapping of version strings to git refs."""

@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 or value.path == "/":
raise ValueError(f"url {value} has an empty path")
return value
Comment thread
jskladan marked this conversation as resolved.

def resolver_provider(
self, ctx: context.WorkContext, req_type: requirements_file.RequirementType
) -> resolver.VersionMapProvider:
clone_url = str(self.clone_url)
url_map = {
ver: f"git+{clone_url}@{ref}" for ver, ref in self.versionmap.items()

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[Recommendation]

The ref values from versionmap are dropped straight into f"git+{clone_url}@{ref}" with no validation. A ref containing @, whitespace, or a newline could produce a malformed URL. It's operator-controlled
config so the risk is low, but a quick check would add defense-in-depth.

Validate refs against a simple allowlist in a field_validator("versionmap").

After:

@pydantic.field_validator("versionmap", mode="after")
@classmethod
def validate_refs(cls, value: dict[str, str]) -> dict[str, str]:
    for ref in value.values():
        if not re.fullmatch(r"[\w./-]+", ref):
            raise ValueError(f"invalid git ref {ref!r}")
    return value

}
version_map = VersionMap(url_map) # type: ignore[arg-type]

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[Question]src/fromager/packagesettings/_resolver.py (~line 532, resolver_provider)

VersionMap(url_map) # type: ignore[arg-type] — the type: ignore implies VersionMap is declared to accept something other than dict[str, str]. Runtime tests show a plain strstr mapping works, so the
contract seems satisfied, but the ignore hides whether VersionMap does any key/value coercion this call bypasses. Can you confirm VersionMap.__init__'s real signature rather than suppressing the type error
— or add the proper type so the ignore can be dropped?

return resolver.VersionMapProvider(
version_map=version_map,
package_name=None,

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[Recommendation]

  • src/fromager/packagesettings/_resolver.py (~line 535, resolver_provider passes package_name=None)
  • tests/test_resolver.py (~lines 938 & 972, tests pass package_name="testpkg")

The code invocation always passes package_name=None. But the tests always pass package_name="testpkg". So the tests are checking a situation that never actually happens. If the "no name" case is broken, every test still passes and nobody notices.

Fix: add a test that uses the code the way it really runs — with package_name=None
and actually resolves a package to make sure it works.

constraints=ctx.constraints,
req_type=req_type,
)


class NotAvailableResolver(AbstractResolver):
"""Prevent resolve and download"""

Expand Down Expand Up @@ -510,6 +566,7 @@ def resolver_provider(
| PyPIPrebuiltResolver
| PyPIDownloadResolver
| PyPIGitResolver
| VersionMapGitResolver
| GitHubTagCloneResolver
| GitHubTagDownloadResolver
| GitLabTagCloneResolver
Expand Down
45 changes: 45 additions & 0 deletions tests/test_packagesettings_resolver.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
PyPIPrebuiltResolver,
PyPISDistResolver,
SourceResolver,
VersionMapGitResolver,
)
from fromager.packagesettings._typedefs import MODEL_CONFIG
from fromager.requirements_file import RequirementType
Expand Down Expand Up @@ -426,6 +427,50 @@ def test_resolver_provider(self, tmp_context: WorkContext) -> None:
# -- Special resolvers --------------------------------------------------------


class TestVersionMapGitResolver:
YAML = """\
source:
provider: versionmap-git
clone_url: https://git.test/project/repo.git
build_sdist: pep517
versionmap:
'1.0': abad1dea
'1.1': refs/tags/1.1
"""

def test_parse(self) -> None:
r = _parse(self.YAML)
assert isinstance(r, VersionMapGitResolver)
assert r.provider == "versionmap-git"
assert str(r.clone_url) == "https://git.test/project/repo.git"
assert r.build_sdist == BuildSDist.pep517
assert r.versionmap == {"1.0": "abad1dea", "1.1": "refs/tags/1.1"}

def test_resolver_provider(self, tmp_context: WorkContext) -> None:
r = _parse(self.YAML)
p = r.resolver_provider(tmp_context, _REQ_TYPE)
assert isinstance(p, resolver.VersionMapProvider)
clone_url = "https://git.test/project/repo.git"
assert p.version_map["1.0"] == f"git+{clone_url}@abad1dea"
assert p.version_map["1.1"] == f"git+{clone_url}@refs/tags/1.1"
Comment thread
coderabbitai[bot] marked this conversation as resolved.

def test_clone_url_rejects_http(self) -> None:
with pytest.raises(pydantic.ValidationError):
VersionMapGitResolver(
provider="versionmap-git",
clone_url="http://git.test/project/repo.git", # type: ignore[arg-type]
versionmap={"1.0": "abc123"},
)

def test_clone_url_rejects_empty_path(self) -> None:
with pytest.raises(pydantic.ValidationError):
VersionMapGitResolver(
provider="versionmap-git",
clone_url="https://git.test", # type: ignore[arg-type]
versionmap={"1.0": "abc123"},
)


class TestNotAvailableResolver:
YAML = """\
source:
Expand Down
58 changes: 58 additions & 0 deletions tests/test_resolver.py
Original file line number Diff line number Diff line change
Expand Up @@ -920,6 +920,64 @@ def test_resolve_versionmap_no_match() -> None:
rslvr.resolve([Requirement("testpkg>=2.0")])


def test_resolve_versionmap_git() -> None:
from fromager.versionmap import VersionMap

clone_url = "https://git.test/project/repo.git"
version_map = VersionMap(
{
"1.0": f"git+{clone_url}@abad1dea",
"1.1": f"git+{clone_url}@refs/tags/1.1",
"1.2": f"git+{clone_url}@d3adb33f",
}
)

provider = resolver.VersionMapProvider(
version_map=version_map,
package_name="testpkg",
)
reporter: resolvelib.BaseReporter = resolvelib.BaseReporter()
rslvr = resolvelib.Resolver(provider, reporter)

result = rslvr.resolve([Requirement("testpkg")])
assert "testpkg" in result.mapping

candidate = result.mapping["testpkg"]
assert str(candidate.version) == "1.2"
assert candidate.url == f"git+{clone_url}@d3adb33f"


def test_resolve_versionmap_git_with_constraint() -> None:
from fromager.versionmap import VersionMap

clone_url = "https://git.test/project/repo.git"
version_map = VersionMap(
{
"1.0": f"git+{clone_url}@abad1dea",
"1.1": f"git+{clone_url}@refs/tags/1.1",
"1.2": f"git+{clone_url}@d3adb33f",
}
)

c = constraints.Constraints()
c.add_constraint("testpkg<1.2")

provider = resolver.VersionMapProvider(
version_map=version_map,
package_name="testpkg",
constraints=c,
)
reporter: resolvelib.BaseReporter = resolvelib.BaseReporter()
rslvr = resolvelib.Resolver(provider, reporter)

result = rslvr.resolve([Requirement("testpkg")])
assert "testpkg" in result.mapping

candidate = result.mapping["testpkg"]
assert str(candidate.version) == "1.1"
assert candidate.url == f"git+{clone_url}@refs/tags/1.1"


_gitlab_submodlib_repo_response = """
[
{
Expand Down
Loading