From cab701cc9a749cdc6f87cc5285e51c28f976c360 Mon Sep 17 00:00:00 2001 From: Shubh Bapna Date: Wed, 7 Aug 2024 14:41:01 -0400 Subject: [PATCH] add new build_dir field in settings --- docs/customization.md | 5 +++- pyproject.toml | 9 ++++--- src/fromager/dependencies.py | 16 ++++++++---- src/fromager/settings.py | 14 ++++++++++ src/fromager/sources.py | 12 ++++++--- src/fromager/wheels.py | 8 +++--- tests/test_settings.py | 50 ++++++++++++++++++++++++++++++++++++ 7 files changed, 97 insertions(+), 17 deletions(-) diff --git a/docs/customization.md b/docs/customization.md index d3a6bdef..1c944404 100644 --- a/docs/customization.md +++ b/docs/customization.md @@ -455,7 +455,9 @@ To use predefined urls to download sources from, instead of overriding the entir - `version` - it is replaced by the version returned by the resolver - `canonicalized_name` - it is replaced by the canonicalized name of the package specified in the requirement, specifically it applies `canonicalize_name(req.nam)` -Additionally, the source distribution index server used by the package resolver can be overriden for a particular package. The resolver can also be told to whether include wheels or sdist sources while trying to resolve the package. Templating is not supported here. +The source distribution index server used by the package resolver can be overriden for a particular package. The resolver can also be told to whether include wheels or sdist sources while trying to resolve the package. Templating is not supported here. + +A `build_dir` field can also be defined to indicate to fromager where the package should be build relative to the sdist root directory ```yaml packages: @@ -467,4 +469,5 @@ packages: sdist_server_url: "https://pypi.org/simple" include_wheels: true include_sdists: false + build_dir: directory name relative to sdist directory, defaults to an empty string, which means to use the sdist directory ``` diff --git a/pyproject.toml b/pyproject.toml index ae3f6c56..9321bf8b 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -106,10 +106,11 @@ mypy_path = ["src"] # TODO: remove excludes and silent follow follow_imports = "silent" exclude = [ - "^src/fromager/resolver\\.py$", - "^src/fromager/sources\\.py$", - "^src/fromager/sdist\\.py$", - "^src/fromager/commands/build\\.py$", + "^src/fromager/resolver\\.py$", + "^src/fromager/sources\\.py$", + "^src/fromager/sdist\\.py$", + "^src/fromager/commands/build\\.py$", + "^src/fromager/settings\\.py$", ] [[tool.mypy.overrides]] diff --git a/src/fromager/dependencies.py b/src/fromager/dependencies.py index beffb357..d3929159 100644 --- a/src/fromager/dependencies.py +++ b/src/fromager/dependencies.py @@ -39,6 +39,7 @@ def get_build_system_dependencies( ctx=ctx, req=req, sdist_root_dir=sdist_root_dir, + build_dir=ctx.settings.build_dir(req.name, sdist_root_dir), ) deps = _filter_requirements(req, orig_deps) @@ -68,8 +69,9 @@ def default_get_build_system_dependencies( ctx: context.WorkContext, req: Requirement, sdist_root_dir: pathlib.Path, + build_dir: pathlib.Path, ) -> typing.Iterable[str]: - pyproject_toml = get_pyproject_contents(sdist_root_dir) + pyproject_toml = get_pyproject_contents(build_dir) return typing.cast(list[str], get_build_backend(pyproject_toml)["requires"]) @@ -96,6 +98,7 @@ def get_build_backend_dependencies( ctx=ctx, req=req, sdist_root_dir=sdist_root_dir, + build_dir=ctx.settings.build_dir(req.name, sdist_root_dir), ) deps = _filter_requirements(req, orig_deps) @@ -110,11 +113,12 @@ def default_get_build_backend_dependencies( ctx: context.WorkContext, req: Requirement, sdist_root_dir: pathlib.Path, + build_dir: pathlib.Path, ) -> typing.Iterable[str]: - pyproject_toml = get_pyproject_contents(sdist_root_dir) + pyproject_toml = get_pyproject_contents(build_dir) extra_environ = overrides.extra_environ_for_pkg(ctx.envs_dir, req.name, ctx.variant) hook_caller = get_build_backend_hook_caller( - sdist_root_dir, pyproject_toml, override_environ=extra_environ + build_dir, pyproject_toml, override_environ=extra_environ ) return hook_caller.get_requires_for_build_wheel() @@ -142,6 +146,7 @@ def get_build_sdist_dependencies( ctx=ctx, req=req, sdist_root_dir=sdist_root_dir, + build_dir=ctx.settings.build_dir(req.name, sdist_root_dir), ) deps = _filter_requirements(req, orig_deps) @@ -156,11 +161,12 @@ def default_get_build_sdist_dependencies( ctx: context.WorkContext, req: Requirement, sdist_root_dir: pathlib.Path, + build_dir: pathlib.Path, ) -> typing.Iterable[str]: - pyproject_toml = get_pyproject_contents(sdist_root_dir) + pyproject_toml = get_pyproject_contents(build_dir) extra_environ = overrides.extra_environ_for_pkg(ctx.envs_dir, req.name, ctx.variant) hook_caller = get_build_backend_hook_caller( - sdist_root_dir, pyproject_toml, override_environ=extra_environ + build_dir, pyproject_toml, override_environ=extra_environ ) return hook_caller.get_requires_for_build_wheel() diff --git a/src/fromager/settings.py b/src/fromager/settings.py index cceda3fd..c90bc91a 100644 --- a/src/fromager/settings.py +++ b/src/fromager/settings.py @@ -82,6 +82,20 @@ def resolver_include_sdists( resolve_dist.get("include_sdists"), default ) + def build_dir(self, pkg: str, sdist_root_dir: pathlib.Path) -> pathlib.Path: + p = self.get_package_settings(pkg) + if p.get("build_dir"): + input_build_dir = pathlib.Path(p.get("build_dir")) + # hack: make path absolute to ensure that any directory escaping is contained + if ".." in str(input_build_dir): + raise ValueError( + f"{pkg}: build dir {input_build_dir} defined in settings is not relative to sdist root dir" + ) + # ensure that absolute build_dir path from settings is converted to a relative path + relative_build_dir = input_build_dir.relative_to(input_build_dir.anchor) + return sdist_root_dir / relative_build_dir + return sdist_root_dir + def get_package_settings(self, pkg: str) -> dict[str, dict[str, str]]: p = self.packages() return self._return_value_or_default( diff --git a/src/fromager/sources.py b/src/fromager/sources.py index 19113f1b..39d9d92d 100644 --- a/src/fromager/sources.py +++ b/src/fromager/sources.py @@ -404,7 +404,9 @@ def build_sdist( sdist_root_dir: pathlib.Path, build_env: wheels.BuildEnvironment, ) -> pathlib.Path: - logger.info(f"{req.name}: building source distribution in {sdist_root_dir}") + logger.info( + f"{req.name}: building source distribution in {ctx.settings.build_dir(req.name, sdist_root_dir)}" + ) extra_environ = overrides.extra_environ_for_pkg(ctx.envs_dir, req.name, ctx.variant) sdist_filename = overrides.find_and_invoke( req.name, @@ -415,6 +417,7 @@ def build_sdist( req=req, version=version, sdist_root_dir=sdist_root_dir, + build_dir=ctx.settings.build_dir(req.name, sdist_root_dir), build_env=build_env, ) logger.info(f"{req.name}: built source distribution {sdist_filename}") @@ -428,6 +431,7 @@ def default_build_sdist( version: Version, sdist_root_dir: pathlib.Path, build_env: wheels.BuildEnvironment, + build_dir: pathlib.Path, ) -> pathlib.Path: # 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 @@ -439,7 +443,7 @@ def default_build_sdist( # # For cases where the PEP 517 approach works, use # pep517_build_sdist(). - sdist_filename = ctx.sdists_builds / (sdist_root_dir.name + ".tar.gz") + sdist_filename = ctx.sdists_builds / (build_dir.name + ".tar.gz") if sdist_filename.exists(): sdist_filename.unlink() # The format argument is specified based on @@ -447,8 +451,8 @@ def default_build_sdist( with tarfile.open(sdist_filename, "x:gz", format=tarfile.PAX_FORMAT) as sdist: tarballs.tar_reproducible( tar=sdist, - basedir=sdist_root_dir, - prefix=sdist_root_dir.parent, + basedir=build_dir, + prefix=build_dir.parent, ) return sdist_filename diff --git a/src/fromager/wheels.py b/src/fromager/wheels.py index dc2e2a27..a51d5fa6 100644 --- a/src/fromager/wheels.py +++ b/src/fromager/wheels.py @@ -162,6 +162,7 @@ def build_wheel( extra_environ=extra_environ, req=req, sdist_root_dir=sdist_root_dir, + build_dir=ctx.settings.build_dir(req.name, sdist_root_dir), version=version, ) # End the timer @@ -183,8 +184,9 @@ def default_build_wheel( req: Requirement, sdist_root_dir: pathlib.Path, version: Version, + build_dir: pathlib.Path, ) -> None: - logger.debug(f"{req.name}: building wheel in {sdist_root_dir} with {extra_environ}") + logger.debug(f"{req.name}: building wheel in {build_dir} with {extra_environ}") # Activate the virtualenv for the subprocess: # 1. Put the build environment at the front of the PATH to ensure @@ -219,8 +221,8 @@ def default_build_wheel( "--index-url", ctx.wheel_server_url, # probably redundant, but just in case "--log", - os.fspath(sdist_root_dir.parent / "build.log"), - os.fspath(sdist_root_dir), + os.fspath(build_dir.parent / "build.log"), + os.fspath(build_dir), ] external_commands.run( cmd, diff --git a/tests/test_settings.py b/tests/test_settings.py index a8afb27c..79490832 100644 --- a/tests/test_settings.py +++ b/tests/test_settings.py @@ -1,3 +1,4 @@ +import pathlib import textwrap import pytest @@ -91,6 +92,55 @@ def test_no_resolver_dist(): assert s.resolver_sdist_server_url("foo") is None +def test_relative_path_build_dir(): + s = settings._parse( + textwrap.dedent(""" + packages: + foo: + build_dir: "./build" + """) + ) + sdist_root_dir = pathlib.Path("/foo/bar") + assert s.build_dir("foo", sdist_root_dir) == sdist_root_dir / "build" + + +def test_only_name_build_dir(): + s = settings._parse( + textwrap.dedent(""" + packages: + foo: + build_dir: "build" + """) + ) + sdist_root_dir = pathlib.Path("/foo/bar") + assert s.build_dir("foo", sdist_root_dir) == sdist_root_dir / "build" + + +def test_absolute_path_build_dir(): + s = settings._parse( + textwrap.dedent(""" + packages: + foo: + build_dir: "/tmp/build" + """) + ) + sdist_root_dir = pathlib.Path("/foo/bar") + assert s.build_dir("foo", sdist_root_dir) == sdist_root_dir / "tmp" / "build" + + +def test_escape_sdist_root_build_dir(): + s = settings._parse( + textwrap.dedent(""" + packages: + foo: + build_dir: "../tmp/build" + """) + ) + sdist_root_dir = pathlib.Path("/foo/bar") + with pytest.raises(ValueError): + str(s.build_dir("foo", sdist_root_dir)).startswith("/foo/bar") + + def test_resolve_template_with_no_template(): req = Requirement("foo==1.0") assert settings._resolve_template(None, req, "1.0") is None