diff --git a/docs/concepts/resolver-architecture.rst b/docs/concepts/resolver-architecture.rst index 06899f799..0da851d7c 100644 --- a/docs/concepts/resolver-architecture.rst +++ b/docs/concepts/resolver-architecture.rst @@ -118,6 +118,14 @@ the artifact kind available for source preparation; consuming it in preparation and integrating prebuilt detection into the bootstrapper are separate steps. See :ref:`download_source_hook` for the return contract and compatibility rules. +.. versionchanged:: 0.102.0 + Configured source resolvers also select the sdist build method. Git checkout + resolvers use PEP 517 by default; hook sdist resolvers require an explicit + ``build_sdist: pep517`` or ``build_sdist: tarball``. PyPI sdist resolvers + repack a tarball. Both configured strategies take precedence over an installed + ``build_sdist`` override and log a warning when it is ignored. Legacy + settings continue to use the override. + Version Filtering Window ------------------------- diff --git a/docs/reference/hooks.rst b/docs/reference/hooks.rst index b8e90ef3b..c92149684 100644 --- a/docs/reference/hooks.rst +++ b/docs/reference/hooks.rst @@ -264,8 +264,8 @@ Source hooks downloads require a download hook. The ``get_resolver_provider`` hook remains required for both profiles. - To migrate a raw-tarball hook to ``source: {provider: hook-sdist}``, return - its artifact kind explicitly: + To migrate a raw-tarball hook to ``hook-sdist``, specify ``build_sdist`` + in YAML and return its artifact kind explicitly: .. code-block:: python diff --git a/src/fromager/packagesettings/_resolver.py b/src/fromager/packagesettings/_resolver.py index 8c0494491..ac57756ac 100644 --- a/src/fromager/packagesettings/_resolver.py +++ b/src/fromager/packagesettings/_resolver.py @@ -69,6 +69,9 @@ class AbstractResolver(pydantic.BaseModel): provider: str + build_sdist: BuildSDist | None + """Sdist build strategy; None for resolvers that do not build from source.""" + supports_override_hooks: typing.ClassVar[bool] = False """Does resolver support override hooks?""" @@ -208,7 +211,7 @@ class PyPISDistResolver(AbstractPyPIResolver): # It is not safe to use PEP 517 to re-generate a source distribution. # Some PEP 517 backends require VCS to generate correct sdist. - build_sdist: typing.ClassVar[BuildSDist | None] = BuildSDist.tarball + build_sdist: typing.Literal[BuildSDist.tarball] = BuildSDist.tarball download_kinds: typing.ClassVar[DownloadKindSet] = frozenset({DownloadKind.sdist}) def resolver_provider( @@ -258,7 +261,7 @@ class PyPIPrebuiltResolver(AbstractPyPIResolver): provider: typing.Literal["pypi-prebuilt"] - build_sdist: typing.ClassVar[BuildSDist | None] = None + build_sdist: None = None download_kinds: typing.ClassVar[DownloadKindSet] = frozenset( {DownloadKind.prebuilt_wheel} ) @@ -322,7 +325,7 @@ class PyPIDownloadResolver(AbstractPyPIResolver): URL must contain '{version}' template string. """ - build_sdist: typing.ClassVar[BuildSDist | None] = BuildSDist.tarball + build_sdist: typing.Literal[BuildSDist.tarball] = BuildSDist.tarball download_kinds: typing.ClassVar[DownloadKindSet] = frozenset( {DownloadKind.sdist, DownloadKind.tarball} ) @@ -747,6 +750,7 @@ class NotAvailableResolver(AbstractResolver): """Prevent resolve and download""" provider: typing.Literal["not-available"] + build_sdist: None = None def resolver_provider( self, @@ -901,13 +905,18 @@ class HookSDistResolver(AbstractHookResolver): a tarball, or a git checkout. Download hooks can return ``DownloadedSource`` with a concrete kind, or a legacy archive ``Path`` treated as an sdist. Without a download hook, the candidate is downloaded as an sdist. + Specify ``build_sdist`` explicitly: ``tarball`` to repack an existing + sdist, or ``pep517`` to build from a git checkout. Example:: provider: hook-sdist + build_sdist: tarball """ provider: typing.Literal["hook-sdist"] + build_sdist: BuildSDist + """Source distribution build method.""" download_kinds: typing.ClassVar[DownloadKindSet] = frozenset( {DownloadKind.sdist, DownloadKind.tarball, DownloadKind.git_checkout} @@ -957,6 +966,7 @@ class HookPrebuiltResolver(AbstractHookResolver): """ provider: typing.Literal["hook-prebuilt"] + build_sdist: None = None download_kinds: typing.ClassVar[DownloadKindSet] = frozenset( {DownloadKind.prebuilt_wheel} diff --git a/src/fromager/sources.py b/src/fromager/sources.py index 12051259d..efd2f20ea 100644 --- a/src/fromager/sources.py +++ b/src/fromager/sources.py @@ -512,8 +512,9 @@ def build_sdist( sdist_root_dir: pathlib.Path, build_env: build_environment.BuildEnvironment, ) -> pathlib.Path: - """Build source distribution""" + """Build a source distribution using the configured strategy or legacy hook.""" pbi = ctx.package_build_info(req) + source_resolver = pbi.source_resolver build_dir = pbi.build_dir(sdist_root_dir) logger.info(f"building {ctx.variant} source distribution for {req} in {build_dir}") @@ -524,18 +525,56 @@ def build_sdist( sdist_root_dir=sdist_root_dir, build_env=build_env, ) - sdist_file: pathlib.Path = overrides.find_and_invoke( - req.name, - "build_sdist", - default_build_sdist, - ctx=ctx, - extra_environ=extra_environ, - req=req, - version=version, - sdist_root_dir=sdist_root_dir, - build_dir=build_dir, - build_env=build_env, - ) + if source_resolver is not None: + if overrides.find_override_method(req.name, "build_sdist") is not None: + logger.warning( + "%s: ignoring build_sdist override because source.build_sdist is %s", + req.name, + source_resolver.build_sdist, + ) + if ( + source_resolver is not None + and source_resolver.build_sdist is packagesettings.BuildSDist.pep517 + ): + sdist_file = pep517_build_sdist( + ctx=ctx, + extra_environ=extra_environ, + req=req, + version=version, + sdist_root_dir=sdist_root_dir, + build_env=build_env, + ) + elif ( + source_resolver is not None + and source_resolver.build_sdist is packagesettings.BuildSDist.tarball + ): + sdist_file = default_build_sdist( + ctx=ctx, + extra_environ=extra_environ, + req=req, + version=version, + sdist_root_dir=sdist_root_dir, + build_dir=build_dir, + build_env=build_env, + ) + elif source_resolver is None: + sdist_file = overrides.find_and_invoke( + req.name, + "build_sdist", + default_build_sdist, + ctx=ctx, + extra_environ=extra_environ, + req=req, + version=version, + sdist_root_dir=sdist_root_dir, + build_dir=build_dir, + build_env=build_env, + ) + else: + raise ValueError( + f"{req.name}: unsupported source.build_sdist strategy " + f"{source_resolver.build_sdist!r}" + ) logger.info(f"built source distribution {sdist_file}") # validate location and file name diff --git a/tests/test_packagesettings_resolver.py b/tests/test_packagesettings_resolver.py index 54adfa56c..8a942d12c 100644 --- a/tests/test_packagesettings_resolver.py +++ b/tests/test_packagesettings_resolver.py @@ -19,6 +19,7 @@ from fromager.candidate import Candidate, Cooldown from fromager.context import WorkContext from fromager.packagesettings._resolver import ( + AbstractResolver, BuildSDist, DownloadedSource, DownloadKind, @@ -128,6 +129,22 @@ def _parse(raw_yaml: str) -> SourceResolver: return wrapper.source +def test_resolver_requires_build_sdist_strategy() -> None: + """Require every resolver to declare an sdist build strategy.""" + with pytest.raises(pydantic.ValidationError, match="build_sdist"): + AbstractResolver.model_validate({"provider": "test"}) + + +@pytest.mark.parametrize( + "provider", + ["pypi-sdist", "pypi-prebuilt", "hook-prebuilt", "not-available"], +) +def test_resolver_rejects_unsupported_build_sdist_strategy(provider: str) -> None: + """Keep fixed resolver strategies from accepting a different build method.""" + with pytest.raises(pydantic.ValidationError, match="build_sdist"): + _parse(f"source:\n provider: {provider}\n build_sdist: pep517\n") + + # -- PyPI resolvers ----------------------------------------------------------- @@ -679,18 +696,34 @@ class TestHookSDistResolver: YAML = """\ source: provider: hook-sdist + build_sdist: tarball """ def test_parse(self) -> None: r = _parse(self.YAML) assert isinstance(r, HookSDistResolver) assert r.provider == "hook-sdist" + assert r.build_sdist is BuildSDist.tarball assert r.supports_override_hooks is True assert r.resolves_prebuilt_wheel is False assert r.download_kinds == frozenset( {DownloadKind.sdist, DownloadKind.tarball, DownloadKind.git_checkout} ) + @pytest.mark.parametrize("build_method", list(BuildSDist)) + def test_build_sdist_method(self, build_method: BuildSDist) -> None: + """Load the hook sdist build method from YAML.""" + r = _parse( + f"source:\n provider: hook-sdist\n build_sdist: {build_method.value}\n" + ) + + assert r.build_sdist is build_method + + def test_build_sdist_method_required(self) -> None: + """Require a build method because hook artifacts need different strategies.""" + with pytest.raises(pydantic.ValidationError, match="build_sdist"): + _parse("source:\n provider: hook-sdist\n") + @mock.patch("fromager.packagesettings._resolver.overrides.find_override_method") def test_resolver_provider( self, @@ -805,7 +838,11 @@ def test_hook_download_preserves_typed_result_and_arguments( tmp_context: WorkContext, provider: str, kind: DownloadKind ) -> None: """Preserve the hook's explicit kind and forward the legacy arguments.""" - r = _parse(f"source:\n provider: {provider}") + r = _parse( + TestHookSDistResolver.YAML + if provider == "hook-sdist" + else TestHookPrebuiltResolver.YAML + ) candidate = _CANDIDATE_WHEEL if r.resolves_prebuilt_wheel else _CANDIDATE_SDIST expected = DownloadedSource(path=tmp_context.work_dir / "artifact", kind=kind) captured: dict[str, object] = {} @@ -862,7 +899,11 @@ def test_hook_download_adapts_legacy_path_and_narrow_signature( tmp_context: WorkContext, provider: str, filename: str, kind: DownloadKind ) -> None: """Adapt bare paths without requiring hooks to accept every argument.""" - r = _parse(f"source:\n provider: {provider}") + r = _parse( + TestHookSDistResolver.YAML + if provider == "hook-sdist" + else TestHookPrebuiltResolver.YAML + ) expected = tmp_context.work_dir / filename def hook(req: Requirement, version: Version) -> pathlib.Path: @@ -924,7 +965,11 @@ def test_hook_download_rejects_invalid_results_without_fallback( message: str, ) -> None: """Reject malformed hook results with package and provider context.""" - r = _parse(f"source:\n provider: {provider}") + r = _parse( + TestHookSDistResolver.YAML + if provider == "hook-sdist" + else TestHookPrebuiltResolver.YAML + ) def hook() -> object: return hook_result @@ -953,7 +998,11 @@ def test_hook_download_rejects_kind_incompatible_with_profile( tmp_context: WorkContext, provider: str, kind: DownloadKind ) -> None: """Do not relabel an explicit kind to fit the selected profile.""" - r = _parse(f"source:\n provider: {provider}") + r = _parse( + TestHookSDistResolver.YAML + if provider == "hook-sdist" + else TestHookPrebuiltResolver.YAML + ) def hook() -> DownloadedSource: return DownloadedSource(path=tmp_context.work_dir, kind=kind) @@ -983,7 +1032,11 @@ def test_hook_download_requires_kind_for_unsupported_legacy_paths( tmp_context: WorkContext, provider: str, filename: str, is_directory: bool ) -> None: """Require an explicit kind when a path cannot use the legacy default.""" - r = _parse(f"source:\n provider: {provider}") + r = _parse( + TestHookSDistResolver.YAML + if provider == "hook-sdist" + else TestHookPrebuiltResolver.YAML + ) path = tmp_context.work_dir / filename if is_directory: path.mkdir() @@ -1005,7 +1058,11 @@ def test_hook_download_uses_profile_default_when_hook_is_missing( tmp_context: WorkContext, provider: str ) -> None: """Select the correct download helper and destination without a hook.""" - r = _parse(f"source:\n provider: {provider}") + r = _parse( + TestHookSDistResolver.YAML + if provider == "hook-sdist" + else TestHookPrebuiltResolver.YAML + ) prebuilt = r.resolves_prebuilt_wheel candidate = _CANDIDATE_WHEEL if prebuilt else _CANDIDATE_SDIST destination = ( @@ -1032,7 +1089,11 @@ def test_hook_download_chains_failure_without_fallback( tmp_context: WorkContext, provider: str ) -> None: """Preserve the hook failure and do not attempt a default download.""" - r = _parse(f"source:\n provider: {provider}") + r = _parse( + TestHookSDistResolver.YAML + if provider == "hook-sdist" + else TestHookPrebuiltResolver.YAML + ) cause = OSError("download failed") def hook() -> pathlib.Path: @@ -1087,7 +1148,9 @@ def legacy_hook( return provider find_override_method.return_value = legacy_hook - resolver_model = HookSDistResolver(provider="hook-sdist") + resolver_model = HookSDistResolver( + provider="hook-sdist", build_sdist=BuildSDist.tarball + ) result = resolver_model.resolver_provider(tmp_context, _REQ, _REQ_TYPE) @@ -1105,7 +1168,9 @@ def test_hook_resolver_does_not_fall_back_when_hook_is_missing( tmp_context: WorkContext, ) -> None: """A missing hook fails instead of selecting the default PyPI provider.""" - resolver_model = HookSDistResolver(provider="hook-sdist") + resolver_model = HookSDistResolver( + provider="hook-sdist", build_sdist=BuildSDist.tarball + ) with pytest.raises(ValueError, match="requires a get_resolver_provider"): resolver_model.resolver_provider(tmp_context, _REQ, _REQ_TYPE) @@ -1164,7 +1229,9 @@ def hook( raise ValueError("hook failure") find_override_method.return_value = hook - resolver_model = HookSDistResolver(provider="hook-sdist") + resolver_model = HookSDistResolver( + provider="hook-sdist", build_sdist=BuildSDist.tarball + ) with pytest.raises(RuntimeError, match="hook-sdist") as exc_info: resolver_model.resolver_provider(tmp_context, _REQ, _REQ_TYPE) diff --git a/tests/test_sources.py b/tests/test_sources.py index 94d0ca635..2816db5b3 100644 --- a/tests/test_sources.py +++ b/tests/test_sources.py @@ -116,6 +116,7 @@ def hook( tmp_context.cooldown = Cooldown(min_age=datetime.timedelta(days=7)) source_resolver = packagesettings.HookSDistResolver( provider="hook-sdist", + build_sdist=packagesettings.BuildSDist.tarball, min_release_age=14, ) @@ -968,7 +969,9 @@ def hook( packagesettings.PackageBuildInfo, "source_resolver", new_callable=PropertyMock, - return_value=packagesettings.HookSDistResolver(provider="hook-sdist"), + return_value=packagesettings.HookSDistResolver( + provider="hook-sdist", build_sdist=packagesettings.BuildSDist.pep517 + ), ), patch("fromager.overrides.find_override_method", return_value=hook), ): @@ -1061,6 +1064,203 @@ def test_prepare_source_plugin_returns_bad_tuple( ) +@pytest.mark.parametrize( + "source_resolver", + [ + packagesettings.GitHubTagCloneResolver( + provider="github-tag-git", + project_url=pydantic.HttpUrl("https://github.test/org/pkg"), + ), + packagesettings.HookSDistResolver( + provider="hook-sdist", build_sdist=packagesettings.BuildSDist.pep517 + ), + ], + ids=["git-clone", "hook-source"], +) +@pytest.mark.parametrize("has_override", [False, True]) +def test_build_sdist_uses_pep517_for_configured_source( + source_resolver: ( + packagesettings.GitHubTagCloneResolver | packagesettings.HookSDistResolver + ), + has_override: bool, + tmp_context: context.WorkContext, + tmp_path: pathlib.Path, + caplog: pytest.LogCaptureFixture, +) -> None: + """Prefer PEP 517 and warn when it replaces an installed build hook.""" + req = Requirement("pkg==1.0") + version = Version("1.0") + build_env = Mock() + sdist_root_dir = tmp_path / "src" + sdist_file = tmp_context.sdists_builds / "pkg-1.0.tar.gz" + sdist_file.write_text("fake sdist") + override = Mock() if has_override else None + with ( + patch.object( + packagesettings.PackageBuildInfo, + "source_resolver", + new_callable=PropertyMock, + return_value=source_resolver, + ), + patch("fromager.packagesettings.get_extra_environ", return_value={}), + patch("fromager.sources.pep517_build_sdist", return_value=sdist_file) as pep517, + patch("fromager.sources.overrides.find_and_invoke") as find_and_invoke, + patch( + "fromager.sources.overrides.find_override_method", return_value=override + ) as find_override, + ): + result = sources.build_sdist( + ctx=tmp_context, + req=req, + version=version, + sdist_root_dir=sdist_root_dir, + build_env=build_env, + ) + + assert result == sdist_file.resolve() + pep517.assert_called_once_with( + ctx=tmp_context, + extra_environ={}, + req=req, + version=version, + sdist_root_dir=sdist_root_dir, + build_env=build_env, + ) + find_and_invoke.assert_not_called() + find_override.assert_called_once_with(req.name, "build_sdist") + warning = "ignoring build_sdist override because source.build_sdist is pep517" + assert (warning in caplog.text) is has_override + if override is not None: + override.assert_not_called() + + +@pytest.mark.parametrize( + "source_resolver", + [ + None, + packagesettings.HookSDistResolver( + provider="hook-sdist", build_sdist=packagesettings.BuildSDist.tarball + ), + packagesettings.GitHubTagCloneResolver( + provider="github-tag-git", + project_url=pydantic.HttpUrl("https://github.test/org/pkg"), + build_sdist=packagesettings.BuildSDist.tarball, + ), + ], + ids=["legacy", "hook-source", "configured-tarball"], +) +@pytest.mark.parametrize("has_override", [False, True]) +def test_build_sdist_uses_configured_tarball_or_legacy_hook( + source_resolver: ( + packagesettings.HookSDistResolver + | packagesettings.GitHubTagCloneResolver + | None + ), + has_override: bool, + tmp_context: context.WorkContext, + tmp_path: pathlib.Path, + caplog: pytest.LogCaptureFixture, +) -> None: + """Use configured tarball builds directly and keep overrides for legacy settings.""" + req = Requirement("pkg==1.0") + version = Version("1.0") + build_env = Mock() + sdist_root_dir = tmp_path / "src" + sdist_file = tmp_context.sdists_builds / "pkg-1.0.tar.gz" + sdist_file.write_text("fake sdist") + override = Mock() if has_override else None + + with ( + patch.object( + packagesettings.PackageBuildInfo, + "source_resolver", + new_callable=PropertyMock, + return_value=source_resolver, + ), + patch("fromager.packagesettings.get_extra_environ", return_value={}), + patch( + "fromager.sources.overrides.find_and_invoke", return_value=sdist_file + ) as find_and_invoke, + patch("fromager.sources.pep517_build_sdist") as pep517, + patch( + "fromager.sources.default_build_sdist", return_value=sdist_file + ) as tarball, + patch( + "fromager.sources.overrides.find_override_method", return_value=override + ) as find_override, + ): + result = sources.build_sdist( + ctx=tmp_context, + req=req, + version=version, + sdist_root_dir=sdist_root_dir, + build_env=build_env, + ) + + assert result == sdist_file.resolve() + build_kwargs = dict( + ctx=tmp_context, + extra_environ={}, + req=req, + version=version, + sdist_root_dir=sdist_root_dir, + build_dir=sdist_root_dir, + build_env=build_env, + ) + if source_resolver is None: + find_and_invoke.assert_called_once_with( + req.name, "build_sdist", tarball, **build_kwargs + ) + tarball.assert_not_called() + find_override.assert_not_called() + else: + tarball.assert_called_once_with(**build_kwargs) + find_and_invoke.assert_not_called() + find_override.assert_called_once_with(req.name, "build_sdist") + if override is not None: + override.assert_not_called() + warning = "ignoring build_sdist override because source.build_sdist is tarball" + assert (warning in caplog.text) is (source_resolver is not None and has_override) + pep517.assert_not_called() + + +@pytest.mark.parametrize("strategy", [None, "unsupported"]) +def test_build_sdist_rejects_unsupported_configured_strategy( + strategy: str | None, + tmp_context: context.WorkContext, + tmp_path: pathlib.Path, +) -> None: + """Reject unsupported configured strategies without calling a builder or hook.""" + source_resolver = Mock(build_sdist=strategy) + with ( + patch.object( + packagesettings.PackageBuildInfo, + "source_resolver", + new_callable=PropertyMock, + return_value=source_resolver, + ), + patch("fromager.packagesettings.get_extra_environ", return_value={}), + patch("fromager.sources.overrides.find_override_method", return_value=None), + patch("fromager.sources.overrides.find_and_invoke") as find_and_invoke, + patch("fromager.sources.pep517_build_sdist") as pep517, + patch("fromager.sources.default_build_sdist") as tarball, + ): + with pytest.raises( + ValueError, match=r"unsupported source\.build_sdist strategy" + ): + sources.build_sdist( + ctx=tmp_context, + req=Requirement("pkg==1.0"), + version=Version("1.0"), + sdist_root_dir=tmp_path / "src", + build_env=Mock(), + ) + + find_and_invoke.assert_not_called() + pep517.assert_not_called() + tarball.assert_not_called() + + @patch("fromager.overrides.find_and_invoke") @patch("fromager.packagesettings.get_extra_environ", return_value={}) def test_build_sdist_raises_file_not_found(