diff --git a/docs/concepts/hooks-and-overrides.rst b/docs/concepts/hooks-and-overrides.rst index e030fbf9..e878c790 100644 --- a/docs/concepts/hooks-and-overrides.rst +++ b/docs/concepts/hooks-and-overrides.rst @@ -66,6 +66,16 @@ arguments. artifact kind. Package overrides remain active. See :ref:`prepare_source_hook` for their arguments and the public Git checkout helper. + With ``source:`` configured, set ``build_sdist: hook`` to use the package's + custom sdist builder. See :ref:`build_sdist_hook` for details. + +.. versionchanged:: 0.104.0 + + ``source.build_sdist: hook`` requires a callable package override. Fromager + rejects a missing or non-callable hook before resolving or downloading the + package. Hooks that delegate to ``default_build_sdist()`` exclude VCS + metadata by default when ``source:`` is configured. + Global Hooks ------------ diff --git a/docs/concepts/package-settings.rst b/docs/concepts/package-settings.rst index 6d0890e4..6b9bb424 100644 --- a/docs/concepts/package-settings.rst +++ b/docs/concepts/package-settings.rst @@ -35,6 +35,11 @@ Settings Loading Flow ``PackageBuildInfo`` objects are created lazily the first time a package is queried and cached for the rest of the run. +.. versionchanged:: 0.104.0 + With ``source.build_sdist: hook``, Fromager checks that the selected + variant's hook exists and can be called before resolving or downloading + the package. Hooks used only by other variants are not required. + ``PackageSettings`` validates source settings when it loads each package. A package-level ``source`` cannot be combined with an explicitly set variant ``pre_built`` or ``wheel_server_url``. A variant-level ``source`` diff --git a/docs/concepts/resolver-architecture.rst b/docs/concepts/resolver-architecture.rst index 1d592332..9f473124 100644 --- a/docs/concepts/resolver-architecture.rst +++ b/docs/concepts/resolver-architecture.rst @@ -212,17 +212,32 @@ Building the Sdist Git checkouts can be prepared directly. Configured source resolvers also select the sdist build method. -For source builds, the configured resolver selects one of two methods: +For source builds, ``source.build_sdist`` selects the build method: - ``pep517`` calls the package's build backend. Git checkout resolvers - default to it. + default to it. Archive-based PyPI resolvers can select it explicitly. - ``tarball`` repacks the prepared source without VCS metadata such as - ``.git``. PyPI sdist resolvers default to it. + ``.git``. Archive-based PyPI resolvers default to it. +- ``hook`` calls the package's custom ``build_sdist`` hook. It can generate + files or choose which directory to pack. A missing or non-callable hook is + rejected before resolution or download. -``hook-sdist`` requires an explicit ``build_sdist: pep517`` or -``build_sdist: tarball`` setting. A configured method takes precedence -over a ``build_sdist`` override and logs a warning when the override is -ignored; packages without ``source:`` keep using the override. +``hook-sdist`` requires an explicit build method. Other profiles that build +from source can also select ``hook``. The ``pep517`` and ``tarball`` methods +skip installed build hooks with a warning. Without ``source:``, Fromager +uses the hook when present, or the default otherwise. + +Hooks that delegate to ``default_build_sdist()`` inherit its default VCS +exclusion for configured source profiles. An explicit ``exclude_vcs`` value +overrides this policy. Hooks that create archives directly choose their contents. + +See :ref:`build_sdist_hook` for the arguments, archive rules, and an example. + +.. versionchanged:: 0.104.0 + + ``source.build_sdist: hook`` enables package overrides for configured + source profiles. ``default_build_sdist()`` excludes VCS metadata by default + for those profiles. Replaying a Build Order ----------------------- diff --git a/docs/proposals/new-resolver-config.md b/docs/proposals/new-resolver-config.md index 3895f808..6e216cfd 100644 --- a/docs/proposals/new-resolver-config.md +++ b/docs/proposals/new-resolver-config.md @@ -174,6 +174,11 @@ hook and `source` settings, then at least one `source` setting (top-level or variant) must use `provider: hook-sdist` or `provider: hook-prebuilt`. The rule ensures that the hooks are used. +Set `build_sdist: hook` under `source` to use the package's custom sdist +builder with any profile that builds from source. A missing hook is an error. +The `pep517` and `tarball` methods skip installed build hooks with a warning. +Packages without `source` keep their existing build-hook behavior. + ### git clone Git cloning uses the more efficient `git_clone_fast` function. Like pip's diff --git a/docs/reference/config-reference.rst b/docs/reference/config-reference.rst index 4d305de8..3e1655ff 100644 --- a/docs/reference/config-reference.rst +++ b/docs/reference/config-reference.rst @@ -76,6 +76,19 @@ Source Resolver .. autoattribute:: pep517 .. autoattribute:: tarball + .. autoattribute:: hook + +.. versionadded:: 0.104.0 + + Set ``build_sdist: hook`` to use the package's custom sdist builder. + The hook must be installed and callable. This option is only available for + source builds. See :ref:`build_sdist_hook` for details and an example. + +.. versionchanged:: 0.104.0 + + ``pypi-sdist`` and ``pypi-download`` accept an explicit + ``build_sdist: pep517``. Their default remains ``tarball`` because some + build backends require VCS metadata that source archives do not contain. .. autoclass:: DownloadedSource :members: path, kind diff --git a/docs/reference/hooks.rst b/docs/reference/hooks.rst index f9883045..a52e100a 100644 --- a/docs/reference/hooks.rst +++ b/docs/reference/hooks.rst @@ -361,17 +361,55 @@ Source hooks .. versionchanged:: 0.103.0 Passing ``DownloadKind.git_checkout`` prepares a clean Git checkout. +.. _build_sdist_hook: + .. autofromagerhook:: default_build_sdist - The ``build_sdist()`` function is responsible for creating a new source - distribution from the prepared source tree and placing it in - ``ctx.sdists_build``. The dist name and version of the sdist file must - match the ``Requirement`` and ``Version``. + The ``build_sdist()`` hook packs prepared sources into an sdist and returns + its ``Path``. The file must exist in ``ctx.sdists_builds`` and match the + requested package name and version. + + The hook receives ``ctx``, ``extra_environ``, ``req``, ``version``, + ``sdist_root_dir``, ``build_env``, and ``build_dir``. Build dependencies are + installed when the build pipeline prepares them. The standalone + ``step build-sdist`` command does not install dependencies; run + ``step prepare-build`` first. Use the hook to generate files or pack a + directory other than the wheel build directory. + + .. versionchanged:: 0.104.0 + + With ``source:``, set ``build_sdist: hook`` to call this hook. + A missing or non-callable hook is rejected before resolution or + download. The ``pep517`` and ``tarball`` methods + skip installed hooks with a warning. Without ``source:``, Fromager + uses the hook when present, or the default otherwise. + + For a package downloaded as a tarball: + + .. code-block:: yaml + + build_dir: python + source: + provider: pypi-download + download_url: https://sources.example/project-{version}.tar.gz + download_kind: tarball + build_sdist: hook + + Any profile that builds from source can use this option. The + ``hook-sdist`` profile still needs a resolver or download hook. + + .. versionchanged:: 0.104.0 + + ``default_build_sdist()`` now excludes VCS metadata such as ``.git`` + by default when the effective package or variant has ``source:`` + configured, including when called by a hook. Pass ``exclude_vcs=False`` + to retain the previous behavior. - The arguments are the ``WorkContext``, the ``Requirement`` being evaluated, and the - `Path` to the root of the source tree. + Without ``source:``, the helper retains VCS metadata by default; pass + ``exclude_vcs=True`` to omit it. - The return value is the ``Path`` to the newly created source distribution. + Hooks that create archives directly choose their contents and must omit + VCS metadata themselves. Source helper functions ~~~~~~~~~~~~~~~~~~~~~~~ diff --git a/src/fromager/packagesettings/_pbi.py b/src/fromager/packagesettings/_pbi.py index 5b3f60fb..7f9a71cc 100644 --- a/src/fromager/packagesettings/_pbi.py +++ b/src/fromager/packagesettings/_pbi.py @@ -21,6 +21,7 @@ PurlConfig, VariantInfo, ) +from ._resolver import BuildSDist from ._templates import _resolve_template, substitute_template from ._typedefs import Annotations, PackageVersion, PatchMap, Template, Variant @@ -52,6 +53,22 @@ def __init__(self, settings: Settings, ps: PackageSettings) -> None: self._plugin_module: types.ModuleType | typing.Literal[False] | None = False self._patches: PatchMap | None = None self._annotations: Annotations | None = None + self._validate_build_sdist_hook() + + def _validate_build_sdist_hook(self) -> None: + """Check the selected variant's hook before resolution or download.""" + source_resolver = self.source_resolver + if ( + source_resolver is None + or source_resolver.build_sdist is not BuildSDist.hook + ): + return + hook = overrides.find_override_method(self.package, "build_sdist") + if not callable(hook): + raise ValueError( + f"{self.package}: source.build_sdist='hook' requires a callable " + "build_sdist override hook" + ) @property def package(self) -> NormalizedName: diff --git a/src/fromager/packagesettings/_resolver.py b/src/fromager/packagesettings/_resolver.py index 6a05a5d8..d23adf2a 100644 --- a/src/fromager/packagesettings/_resolver.py +++ b/src/fromager/packagesettings/_resolver.py @@ -28,8 +28,15 @@ class BuildSDist(enum.StrEnum): + """Source distribution build method.""" + pep517 = "pep517" tarball = "tarball" + hook = "hook" + """Call the package's ``build_sdist`` override hook. + + .. versionadded:: 0.104.0 + """ class DownloadKind(enum.StrEnum): @@ -71,10 +78,14 @@ class AbstractResolver(pydantic.BaseModel): provider: str build_sdist: BuildSDist | None - """Sdist build strategy; None for resolvers that do not build from source.""" + """Sdist build strategy; None for resolvers that do not build from source. + + ``hook`` requires a package ``build_sdist`` override independently of + the profile's resolver and download hooks. + """ supports_override_hooks: typing.ClassVar[bool] = False - """Does resolver support override hooks?""" + """Does resolver support resolution and download override hooks?""" download_kinds: typing.ClassVar[DownloadKindSet] = frozenset() """Set of download kinds this resolver can return.""" @@ -248,9 +259,11 @@ class PyPISDistResolver(AbstractPyPIResolver): provider: typing.Literal["pypi-sdist"] - # 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.Literal[BuildSDist.tarball] = BuildSDist.tarball + # Tarball repacking is the safe default because some PEP 517 backends + # require VCS metadata to generate a correct sdist. + build_sdist: typing.Literal[ + BuildSDist.pep517, BuildSDist.tarball, BuildSDist.hook + ] = BuildSDist.tarball download_kinds: typing.ClassVar[DownloadKindSet] = frozenset({DownloadKind.sdist}) def resolver_provider( @@ -362,7 +375,9 @@ class PyPIDownloadResolver(AbstractPyPIResolver): URL must contain '{version}' template string. """ - build_sdist: typing.Literal[BuildSDist.tarball] = BuildSDist.tarball + build_sdist: typing.Literal[ + BuildSDist.pep517, BuildSDist.tarball, BuildSDist.hook + ] = BuildSDist.tarball download_kinds: typing.ClassVar[DownloadKindSet] = frozenset( {DownloadKind.sdist, DownloadKind.tarball} ) @@ -808,7 +823,7 @@ class AbstractHookResolver(AbstractResolver, CooldownMixin): """Abstract base class for hook-based resolvers""" supports_override_hooks: typing.ClassVar[bool] = True - """Hook resolvers support override hooks.""" + """Hook resolvers support resolution and download override hooks.""" hookless_provider: typing.ClassVar[str] """Profile to suggest for packages without hooks.""" @@ -953,7 +968,8 @@ class HookSDistResolver(AbstractHookResolver): ``Path`` treated as an sdist. Without a download hook, the resolved URL is downloaded as an sdist. Specify ``build_sdist`` explicitly: ``tarball`` to repack an existing - sdist, or ``pep517`` to build from a git checkout. + sdist, ``pep517`` to build from a git checkout, or ``hook`` to call + the package's ``build_sdist`` override. Example:: diff --git a/src/fromager/sources.py b/src/fromager/sources.py index 226542ab..da317e27 100644 --- a/src/fromager/sources.py +++ b/src/fromager/sources.py @@ -624,12 +624,34 @@ def build_sdist( sdist_root_dir: pathlib.Path, build_env: build_environment.BuildEnvironment, ) -> pathlib.Path: - """Build a source distribution using the configured strategy or legacy hook.""" + """Build a source distribution using the configured strategy or legacy hook. + + ``source.build_sdist: hook`` requires a package override. Configured + ``pep517`` and ``tarball`` strategies take precedence over overrides. + + .. versionchanged:: 0.104.0 + Configured source profiles can select the ``build_sdist`` override. + """ pbi = ctx.package_build_info(req) source_resolver = pbi.source_resolver + strategy = source_resolver.build_sdist if source_resolver is not None else None build_dir = pbi.build_dir(sdist_root_dir) logger.info(f"building {ctx.variant} source distribution for {req} in {build_dir}") + if source_resolver is not None: + build_sdist_override = overrides.find_override_method(req.name, "build_sdist") + if strategy is packagesettings.BuildSDist.hook: + if not callable(build_sdist_override): + raise ValueError( + f"{req.name}: source.build_sdist='hook' requires a callable " + "build_sdist override hook" + ) + elif build_sdist_override is not None: + logger.warning( + "%s: ignoring build_sdist override because source.build_sdist is %s", + req.name, + strategy, + ) extra_environ = packagesettings.get_extra_environ( ctx=ctx, req=req, @@ -637,45 +659,30 @@ def build_sdist( sdist_root_dir=sdist_root_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( + if source_resolver is None or strategy is packagesettings.BuildSDist.hook: + 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, ) - elif ( - source_resolver is not None - and source_resolver.build_sdist is packagesettings.BuildSDist.tarball - ): - sdist_file = default_build_sdist( + elif strategy 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_dir=build_dir, build_env=build_env, - # Git checkouts keep their `.git`; keep it out of the sdist. - exclude_vcs=True, ) - elif source_resolver is None: - sdist_file = overrides.find_and_invoke( - req.name, - "build_sdist", - default_build_sdist, + elif strategy is packagesettings.BuildSDist.tarball: + sdist_file = default_build_sdist( ctx=ctx, extra_environ=extra_environ, req=req, @@ -686,8 +693,7 @@ def build_sdist( ) else: raise ValueError( - f"{req.name}: unsupported source.build_sdist strategy " - f"{source_resolver.build_sdist!r}" + f"{req.name}: unsupported source.build_sdist strategy {strategy!r}" ) logger.info(f"built source distribution {sdist_file}") @@ -713,12 +719,20 @@ def default_build_sdist( build_env: build_environment.BuildEnvironment, build_dir: pathlib.Path, *, - exclude_vcs: bool = False, + exclude_vcs: bool | None = None, ) -> pathlib.Path: """Repack *build_dir* as an sdist. - With *exclude_vcs*, VCS metadata such as ``.git`` is left out. + With *exclude_vcs*, VCS metadata such as ``.git`` is left out. By default, + exclude it when ``source:`` is configured, and retain it otherwise. + Explicit booleans override this policy. + + .. versionchanged:: 0.104.0 + With ``exclude_vcs=None``, exclude VCS metadata when the effective + package or variant has ``source:`` configured; retain it otherwise. """ + if exclude_vcs is None: + exclude_vcs = ctx.package_build_info(req).source_resolver is not None # It seems like the "correct" way to do this would be to run the # PEP 517 API in the source tree we have modified. However, quite # a few packages assume their source distribution is being built diff --git a/tests/test_commands.py b/tests/test_commands.py index bb1113bb..f96ef856 100644 --- a/tests/test_commands.py +++ b/tests/test_commands.py @@ -14,7 +14,7 @@ from packaging.requirements import Requirement from packaging.version import Version -from fromager import packagesettings, server, wheels +from fromager import build_environment, packagesettings, server, wheels from fromager.commands import bootstrap, build, download_sequence, step from fromager.context import WorkContext from fromager.log import requirement_ctxvar @@ -1251,6 +1251,54 @@ def test_default_source_kind_keeps_archive_fallback( assert result is packagesettings.DownloadKind.sdist +def test_step_build_sdist_uses_selected_hook( + cli_runner: CliRunner, tmp_context: WorkContext +) -> None: + """Use a configured sdist hook through the step command and source lookup.""" + _configure_package( + tmp_context, "source:\n provider: pypi-sdist\n build_sdist: hook\n" + ) + source_root = tmp_context.work_dir / "pkg-1.0" / "pkg-1.0" + source_root.mkdir(parents=True) + build_env = Mock(spec=build_environment.BuildEnvironment) + hook_environments: list[build_environment.BuildEnvironment] = [] + output = tmp_context.sdists_builds / "pkg-1.0.tar.gz" + + def hook( + ctx: WorkContext, + extra_environ: dict[str, str], + req: Requirement, + version: Version, + sdist_root_dir: pathlib.Path, + build_env: build_environment.BuildEnvironment, + build_dir: pathlib.Path, + ) -> pathlib.Path: + """Record the build environment and return the selected hook's output.""" + assert sdist_root_dir == source_root + assert build_dir == source_root + hook_environments.append(build_env) + output.touch() + return output + + with ( + patch( + "fromager.overrides.find_override_method", + side_effect=lambda name, method: hook if method == "build_sdist" else None, + ), + patch("fromager.packagesettings.get_extra_environ", return_value={}), + patch.object( + step.build_environment, "BuildEnvironment", return_value=build_env + ), + ): + result = cli_runner.invoke( + step.step, ["build-sdist", "pkg", "1.0"], obj=tmp_context + ) + + assert result.exit_code == 0, result.output + assert result.output.strip() == str(output) + assert hook_environments == [build_env] + + def test_step_prepare_source_accepts_explicit_hook_path( cli_runner: CliRunner, tmp_context: WorkContext, tmp_path: pathlib.Path ) -> None: diff --git a/tests/test_packagesettings.py b/tests/test_packagesettings.py index 6a37a826..b5ee12ed 100644 --- a/tests/test_packagesettings.py +++ b/tests/test_packagesettings.py @@ -13,6 +13,7 @@ from fromager.packagesettings import ( Annotations, BuildDirectory, + BuildSDist, EnvVars, ExternalCommands, GitOptions, @@ -1260,6 +1261,85 @@ def test_pbi_source_resolver_variant_overrides_package( assert isinstance(pbi.source_resolver, PyPIPrebuiltResolver) +@pytest.mark.parametrize( + "source_config", + [ + pytest.param( + "source:\n provider: pypi-sdist\n build_sdist: hook\n", + id="package-hook", + ), + pytest.param( + "variants:\n cpu:\n source:\n" + " provider: pypi-sdist\n build_sdist: hook\n", + id="variant-hook", + ), + pytest.param( + "source:\n provider: pypi-sdist\n build_sdist: hook\n" + "variants:\n cpu:\n env:\n BUILD_VALUE: prepared\n", + id="inherited-hook", + ), + ], +) +@pytest.mark.parametrize("hook", [None, "not-callable"]) +def test_pbi_rejects_missing_or_non_callable_build_sdist_hook( + source_config: str, hook: str | None, tmp_path: pathlib.Path +) -> None: + """Reject an unusable selected sdist hook before resolution or download.""" + ps = PackageSettings.from_string("test-pkg", source_config) + settings = Settings( + settings=SettingsFile(), + package_settings=[ps], + variant="cpu", + patches_dir=tmp_path, + max_jobs=1, + ) + + with patch("fromager.overrides.find_override_method", return_value=hook): + with pytest.raises( + ValueError, + match=r"test-pkg: source\.build_sdist='hook' requires a callable", + ): + settings.package_build_info("test-pkg") + + +@pytest.mark.parametrize( + "source_config", + [ + pytest.param( + "source:\n provider: pypi-sdist\n build_sdist: hook\n" + "variants:\n cpu:\n source:\n provider: pypi-sdist\n", + id="variant-replaces-package-hook", + ), + pytest.param( + "variants:\n cuda:\n source:\n" + " provider: pypi-sdist\n build_sdist: hook\n", + id="inactive-variant-hook", + ), + ], +) +def test_pbi_does_not_require_unused_build_sdist_hook( + source_config: str, tmp_path: pathlib.Path +) -> None: + """Do not load a hook unless the effective source settings select it.""" + ps = PackageSettings.from_string("test-pkg", source_config) + settings = Settings( + settings=SettingsFile(), + package_settings=[ps], + variant="cpu", + patches_dir=tmp_path, + max_jobs=1, + ) + + with patch("fromager.overrides.find_override_method") as find_override: + pbi = settings.package_build_info("test-pkg") + + assert ( + pbi.source_resolver is None + or pbi.source_resolver.build_sdist is BuildSDist.tarball + ) + find_override.assert_not_called() + + @pytest.mark.parametrize( ("yaml_str", "expected"), [ diff --git a/tests/test_packagesettings_resolver.py b/tests/test_packagesettings_resolver.py index 5f05ec12..5773c5c0 100644 --- a/tests/test_packagesettings_resolver.py +++ b/tests/test_packagesettings_resolver.py @@ -104,7 +104,7 @@ def test_resolver_requires_build_sdist_strategy() -> None: @pytest.mark.parametrize( "provider", - ["pypi-sdist", "pypi-prebuilt", "hook-prebuilt", "not-available"], + ["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.""" @@ -137,6 +137,18 @@ def test_default_index_url(self) -> None: r = PyPISDistResolver(provider="pypi-sdist") assert str(r.index_url) == "https://pypi.org/simple/" + def test_parse_explicit_pep517(self) -> None: + """Allow opting into PEP 517 for a downloaded sdist.""" + r = _parse( + """\ + source: + provider: pypi-sdist + index_url: https://pypi.test/simple + build_sdist: pep517 + """ + ) + assert r.build_sdist is BuildSDist.pep517 + def test_cooldown_override(self) -> None: r = PyPISDistResolver(provider="pypi-sdist", min_release_age=7) assert r.min_release_age == 7 @@ -240,6 +252,20 @@ def test_parse(self) -> None: assert r.download_kind == DownloadKind.tarball assert r.download_kinds == frozenset({DownloadKind.sdist, DownloadKind.tarball}) + def test_parse_explicit_pep517(self) -> None: + """Allow opting into PEP 517 for a custom source archive.""" + r = _parse( + """\ + source: + provider: pypi-download + index_url: https://pypi.test/simple + download_url: https://download.test/pkg-{version}.tar.gz + download_kind: tarball + build_sdist: pep517 + """ + ) + assert r.build_sdist is BuildSDist.pep517 + def test_download_kind_sdist(self) -> None: r = _parse("""\ source: @@ -1607,6 +1633,57 @@ def hook( # -- Discriminated union validation ------------------------------------------- +@pytest.mark.parametrize( + "profile_yaml", + [ + TestPyPISDistResolver.YAML, + TestPyPIDownloadResolver.YAML, + TestPyPIGitResolver.YAML, + TestGitHubTagDownloadResolver.YAML, + TestGitHubTagCloneResolver.YAML, + TestGitLabTagDownloadResolver.YAML, + TestGitLabTagCloneResolver.YAML, + TestHookSDistResolver.YAML, + ], + ids=[ + "pypi-sdist", + "pypi-download", + "pypi-git", + "github-tag-download", + "github-tag-git", + "gitlab-tag-download", + "gitlab-tag-git", + "hook-sdist", + ], +) +def test_source_profiles_accept_build_sdist_hook(profile_yaml: str) -> None: + """Allow source profiles to select a build hook independently of acquisition.""" + data = yaml.safe_load(textwrap.dedent(profile_yaml)) + data["source"]["build_sdist"] = "hook" + + source = _SourceWrapper.model_validate(data).source + + assert source.build_sdist is BuildSDist.hook + + +@pytest.mark.parametrize( + "profile_yaml", + [ + TestPyPIPrebuiltResolver.YAML, + TestHookPrebuiltResolver.YAML, + TestNotAvailableResolver.YAML, + ], + ids=["pypi-prebuilt", "hook-prebuilt", "not-available"], +) +def test_non_source_profiles_reject_build_sdist_hook(profile_yaml: str) -> None: + """Prebuilt and unavailable profiles cannot select an sdist build hook.""" + data = yaml.safe_load(textwrap.dedent(profile_yaml)) + data["source"]["build_sdist"] = "hook" + + with pytest.raises(pydantic.ValidationError, match="build_sdist"): + _SourceWrapper.model_validate(data) + + def test_invalid_provider() -> None: with pytest.raises(pydantic.ValidationError): _parse("""\ diff --git a/tests/test_sources.py b/tests/test_sources.py index 54732d58..053c0935 100644 --- a/tests/test_sources.py +++ b/tests/test_sources.py @@ -11,7 +11,7 @@ from packaging.requirements import Requirement from packaging.version import Version -from fromager import context, packagesettings, resolver, sources +from fromager import build_environment, context, packagesettings, resolver, sources from fromager.candidate import Cooldown from fromager.requirements_file import RequirementType @@ -371,6 +371,35 @@ def test_resolve_source_uses_configured_source_resolver( ) +def test_resolve_source_rejects_missing_build_sdist_hook_before_provider( + tmp_context: context.WorkContext, +) -> None: + """Reject a missing selected sdist hook before resolver provider creation.""" + ps = packagesettings.PackageSettings.from_string( + "test-pkg", "source:\n provider: pypi-sdist\n build_sdist: hook\n" + ) + tmp_context.settings._package_settings[ps.name] = ps + req = Requirement("test-pkg>=1.0") + + with ( + patch("fromager.overrides.find_override_method", return_value=None), + patch.object( + packagesettings.PyPISDistResolver, "resolver_provider" + ) as resolver_provider, + ): + with pytest.raises( + ValueError, + match=r"test-pkg: source\.build_sdist='hook' requires a callable", + ): + sources.resolve_source( + ctx=tmp_context, + req=req, + sdist_server_url=resolver.PYPI_SERVER_URL, + ) + + resolver_provider.assert_not_called() + + @patch("fromager.resolver.find_all_matching_from_provider") @patch("fromager.downloads.download_sdist") def test_resolve_source_from_settings( @@ -1804,13 +1833,25 @@ def test_prepare_source_plugin_returns_bad_tuple( packagesettings.HookSDistResolver( provider="hook-sdist", build_sdist=packagesettings.BuildSDist.pep517 ), + packagesettings.PyPISDistResolver( + provider="pypi-sdist", build_sdist=packagesettings.BuildSDist.pep517 + ), + packagesettings.PyPIDownloadResolver( + provider="pypi-download", + download_url=pydantic.HttpUrl("https://pkg.test/pkg-{version}.tar.gz"), + download_kind=packagesettings.DownloadKind.tarball, + build_sdist=packagesettings.BuildSDist.pep517, + ), ], - ids=["git-clone", "hook-source"], + ids=["git-clone", "hook-source", "pypi-sdist", "pypi-download"], ) @pytest.mark.parametrize("has_override", [False, True]) def test_build_sdist_uses_pep517_for_configured_source( source_resolver: ( - packagesettings.GitHubTagCloneResolver | packagesettings.HookSDistResolver + packagesettings.GitHubTagCloneResolver + | packagesettings.HookSDistResolver + | packagesettings.PyPISDistResolver + | packagesettings.PyPIDownloadResolver ), has_override: bool, tmp_context: context.WorkContext, @@ -1944,7 +1985,7 @@ def test_build_sdist_uses_configured_tarball_or_legacy_hook( tarball.assert_not_called() find_override.assert_not_called() else: - tarball.assert_called_once_with(**build_kwargs, exclude_vcs=True) + 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: @@ -1954,6 +1995,335 @@ def test_build_sdist_uses_configured_tarball_or_legacy_hook( pep517.assert_not_called() +@pytest.mark.parametrize( + ("source_config", "uses_hook"), + [ + pytest.param("", True, id="legacy"), + pytest.param( + "source:\n provider: pypi-sdist\n", + False, + id="pypi-sdist", + ), + pytest.param( + "source:\n provider: hook-sdist\n build_sdist: tarball\n", + False, + id="hook-sdist", + ), + pytest.param( + "source:\n" + " provider: github-tag-git\n" + " project_url: https://github.test/org/pkg\n" + " build_sdist: tarball\n", + False, + id="git-tarball", + ), + pytest.param( + "source:\n provider: pypi-sdist\n build_sdist: hook\n", + True, + id="pypi-sdist-hook", + ), + pytest.param( + "source:\n" + " provider: pypi-download\n" + " download_url: https://pkg.test/pkg-{version}.tar.gz\n" + " download_kind: tarball\n" + " build_sdist: hook\n", + True, + id="pypi-download-hook", + ), + pytest.param( + "source:\n provider: hook-sdist\n build_sdist: hook\n", + True, + id="hook-sdist-hook", + ), + pytest.param( + "source:\n" + " provider: github-tag-git\n" + " project_url: https://github.test/org/pkg\n" + " build_sdist: hook\n", + True, + id="git-hook", + ), + pytest.param( + "source:\n provider: pypi-sdist\n" + "variants:\n cpu:\n source:\n" + " provider: pypi-sdist\n build_sdist: hook\n", + True, + id="variant-hook", + ), + pytest.param( + "source:\n provider: pypi-sdist\n build_sdist: hook\n" + "variants:\n cpu:\n source:\n provider: pypi-sdist\n", + False, + id="variant-tarball", + ), + ], +) +def test_build_sdist_dispatches_hook_and_warns_when_skipped( + source_config: str, + uses_hook: bool, + tmp_context: context.WorkContext, + caplog: pytest.LogCaptureFixture, +) -> None: + """Dispatch selected hooks and warn when a configured method skips one.""" + # Arrange + ps = packagesettings.PackageSettings.from_string( + "pkg", "build_dir: python\n" + source_config + ) + tmp_context.settings._package_settings[ps.name] = ps + req = Requirement("pkg==1.0") + version = Version("1.0") + build_env = Mock(spec=build_environment.BuildEnvironment) + sdist_file = tmp_context.sdists_builds / "pkg-1.0.tar.gz" + sdist_file.write_text("fake sdist") + override = Mock() + + # Act + with ( + patch("fromager.overrides.find_override_method", return_value=override), + 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.packagesettings.get_extra_environ", + return_value={}, + ), + ): + result = sources.build_sdist( + ctx=tmp_context, + req=req, + version=version, + sdist_root_dir=tmp_context.work_dir, + build_env=build_env, + ) + + # Assert + assert result == sdist_file.resolve() + if uses_hook: + find_and_invoke.assert_called_once() + tarball.assert_not_called() + else: + find_and_invoke.assert_not_called() + tarball.assert_called_once() + pep517.assert_not_called() + warning = "ignoring build_sdist override because source.build_sdist is tarball" + assert (warning in caplog.text) is (not uses_hook) + + +@pytest.mark.parametrize( + "source_config", + [ + pytest.param("", id="legacy"), + pytest.param( + "source:\n provider: pypi-sdist\n build_sdist: hook\n", + id="pypi-sdist-hook", + ), + pytest.param( + "source:\n" + " provider: pypi-download\n" + " download_url: https://pkg.test/pkg-{version}.tar.gz\n" + " download_kind: tarball\n" + " build_sdist: hook\n", + id="pypi-download-hook", + ), + pytest.param( + "source:\n provider: hook-sdist\n build_sdist: hook\n", + id="hook-sdist-hook", + ), + pytest.param( + "source:\n" + " provider: github-tag-git\n" + " project_url: https://github.test/org/pkg\n" + " build_sdist: hook\n", + id="git-hook", + ), + pytest.param( + "source:\n provider: pypi-sdist\n" + "variants:\n cpu:\n source:\n" + " provider: pypi-sdist\n build_sdist: hook\n", + id="variant-hook", + ), + ], +) +def test_build_sdist_hook_can_archive_repository( + source_config: str, + tmp_context: context.WorkContext, + tmp_path: pathlib.Path, +) -> None: + """Selected hooks can archive the whole prepared repository.""" + # Arrange + ps = packagesettings.PackageSettings.from_string( + "pkg", "build_dir: python\n" + source_config + ) + tmp_context.settings._package_settings[ps.name] = ps + req = Requirement("pkg==1.0") + version = Version("1.0") + build_env = Mock(spec=build_environment.BuildEnvironment) + sdist_root_dir = tmp_path / "pkg-1.0" + build_dir = sdist_root_dir / "python" + build_dir.mkdir(parents=True) + (build_dir / "setup.py").write_text("# Wheel build also needs ../native.c.\n") + (sdist_root_dir / "native.c").write_text("int value(void) { return 1; }\n") + hook_calls: list[pathlib.Path] = [] + + def archive_repository( + ctx: context.WorkContext, + extra_environ: dict[str, str], + req: Requirement, + version: Version, + sdist_root_dir: pathlib.Path, + build_env: build_environment.BuildEnvironment, + build_dir: pathlib.Path, + ) -> pathlib.Path: + """Keep the whole repository while the wheel builds from a subdirectory.""" + hook_calls.append(build_dir) + (sdist_root_dir / "generated.txt").write_text(extra_environ["BUILD_VALUE"]) + return sources.default_build_sdist( + ctx=ctx, + extra_environ=extra_environ, + req=req, + version=version, + sdist_root_dir=sdist_root_dir, + build_env=build_env, + build_dir=sdist_root_dir, + ) + + # Act + with ( + patch( + "fromager.overrides.find_override_method", return_value=archive_repository + ), + patch( + "fromager.packagesettings.get_extra_environ", + return_value={"BUILD_VALUE": "prepared"}, + ), + ): + sdist_file = sources.build_sdist( + ctx=tmp_context, + req=req, + version=version, + sdist_root_dir=sdist_root_dir, + build_env=build_env, + ) + + # Assert + assert sdist_file == tmp_context.sdists_builds / "pkg-1.0.tar.gz" + assert hook_calls == [build_dir] + with tarfile.open(sdist_file, "r:gz") as sdist: + names = sdist.getnames() + generated = sdist.extractfile("pkg-1.0/generated.txt") + assert generated is not None + assert generated.read() == b"prepared" + assert "pkg-1.0/PKG-INFO" in names + assert "pkg-1.0/native.c" in names + assert "pkg-1.0/generated.txt" in names + assert "pkg-1.0/python/setup.py" in names + + +@pytest.mark.parametrize("hook", [None, "not-callable"]) +def test_build_sdist_requires_selected_hook( + hook: str | None, + tmp_context: context.WorkContext, + tmp_path: pathlib.Path, +) -> None: + """Recheck cached settings before invoking environment or build hooks.""" + req = Requirement("pkg==1.0") + ps = packagesettings.PackageSettings.from_string( + "pkg", "source:\n provider: pypi-sdist\n build_sdist: hook\n" + ) + tmp_context.settings._package_settings[ps.name] = ps + with patch("fromager.overrides.find_override_method", return_value=Mock()): + tmp_context.package_build_info(req) + with ( + patch("fromager.overrides.find_override_method", return_value=hook), + patch("fromager.packagesettings.get_extra_environ") as extra_environ, + patch("fromager.sources.pep517_build_sdist") as pep517, + patch("fromager.sources.default_build_sdist") as tarball, + ): + with pytest.raises( + ValueError, + match=r"pkg: source\.build_sdist='hook' requires a callable build_sdist override hook", + ): + sources.build_sdist( + ctx=tmp_context, + req=req, + version=Version("1.0"), + sdist_root_dir=tmp_path, + build_env=Mock(spec=build_environment.BuildEnvironment), + ) + + extra_environ.assert_not_called() + pep517.assert_not_called() + tarball.assert_not_called() + + +@pytest.mark.parametrize( + ("output", "error_type", "message"), + [ + ("missing", FileNotFoundError, "pkg-1.0.tar.gz"), + ("outside", ValueError, "is not in sdists build directory"), + ("other-1.0.tar.gz", ValueError, "does not match requirement"), + ("pkg-2.0.tar.gz", ValueError, "does not match public version"), + ("exception", RuntimeError, "sdist generation failed"), + ], +) +def test_build_sdist_validates_selected_hook_result( + output: str, + error_type: type[Exception], + message: str, + tmp_context: context.WorkContext, + tmp_path: pathlib.Path, +) -> None: + """Reject invalid hook output and propagate hook failures without fallback.""" + ps = packagesettings.PackageSettings.from_string( + "pkg", "source:\n provider: pypi-sdist\n build_sdist: hook\n" + ) + tmp_context.settings._package_settings[ps.name] = ps + if output == "outside": + sdist_file = tmp_path / "pkg-1.0.tar.gz" + else: + filename = "pkg-1.0.tar.gz" if output == "missing" else output + sdist_file = tmp_context.sdists_builds / filename + if output not in {"missing", "exception"}: + sdist_file.touch() + + def hook( + ctx: context.WorkContext, + extra_environ: dict[str, str], + req: Requirement, + version: Version, + sdist_root_dir: pathlib.Path, + build_env: build_environment.BuildEnvironment, + build_dir: pathlib.Path, + ) -> pathlib.Path: + """Return invalid output or raise a generation error.""" + if output == "exception": + raise RuntimeError("sdist generation failed") + return sdist_file + + with ( + patch("fromager.overrides.find_override_method", return_value=hook), + patch("fromager.packagesettings.get_extra_environ", return_value={}), + patch("fromager.sources.pep517_build_sdist") as pep517, + patch("fromager.sources.default_build_sdist") as tarball, + ): + with pytest.raises(error_type, match=message): + sources.build_sdist( + ctx=tmp_context, + req=Requirement("pkg==1.0"), + version=Version("1.0"), + sdist_root_dir=tmp_path, + build_env=Mock(spec=build_environment.BuildEnvironment), + ) + + pep517.assert_not_called() + tarball.assert_not_called() + + @pytest.mark.parametrize("strategy", [None, "unsupported"]) def test_build_sdist_rejects_unsupported_configured_strategy( strategy: str | None, @@ -2120,13 +2490,29 @@ def test_default_build_sdist_normalizes_name_and_root( assert "foo_bar-1.0/setup.py" in names -@pytest.mark.parametrize("exclude_vcs", [False, True]) +@pytest.mark.parametrize( + "source_config", + [ + pytest.param("", id="legacy"), + pytest.param("source:\n provider: pypi-sdist\n", id="package-source"), + pytest.param( + "variants:\n cpu:\n source:\n" + " provider: github-tag-git\n" + " project_url: https://github.test/org/pkg\n", + id="variant-source", + ), + ], +) +@pytest.mark.parametrize("exclude_vcs", [None, False, True]) def test_default_build_sdist_exclude_vcs( - exclude_vcs: bool, + source_config: str, + exclude_vcs: bool | None, tmp_context: context.WorkContext, tmp_path: pathlib.Path, ) -> None: - """Leave VCS metadata out of the sdist only when asked.""" + """Use the source profile's VCS policy unless explicitly overridden.""" + ps = packagesettings.PackageSettings.from_string("pkg", source_config) + tmp_context.settings._package_settings[ps.name] = ps build_dir = tmp_path / "pkg-1.0" (build_dir / ".git" / "objects").mkdir(parents=True) (build_dir / ".git" / "HEAD").write_text("ref: refs/heads/main\n") @@ -2149,6 +2535,7 @@ def test_default_build_sdist_exclude_vcs( names = tar.getnames() assert "pkg-1.0/setup.py" in names has_vcs = {"pkg-1.0/.git/HEAD", "pkg-1.0/sub/.git"} <= set(names) - assert has_vcs is not exclude_vcs - if exclude_vcs: - assert not any("/.git" in name for name in names) + should_exclude = bool(source_config) if exclude_vcs is None else exclude_vcs + assert has_vcs is not should_exclude + if should_exclude: + assert not any(".git" in pathlib.PurePosixPath(name).parts for name in names)