From 232ba32031d0d1d6acd6f3cb3a596789dd006a33 Mon Sep 17 00:00:00 2001 From: James Hilliard Date: Sun, 12 Dec 2021 07:46:59 -0700 Subject: [PATCH] build: add circular dependency checker for build requirements Implement a basic build requirement cycle detector per PEP-517: - Project build requirements will define a directed graph of requirements (project A needs B to build, B needs C and D, etc.) This graph MUST NOT contain cycles. If (due to lack of co-ordination between projects, for example) a cycle is present, front ends MAY refuse to build the project. - Front ends SHOULD check explicitly for requirement cycles, and terminate the build with an informative message if one is found. See: https://www.python.org/dev/peps/pep-0517/#build-requirements --- src/build/__init__.py | 85 +++++++++++++++++-- src/build/__main__.py | 29 ++++++- .../test-circular-requirements/pyproject.toml | 7 ++ tests/test_projectbuilder.py | 17 +++- 4 files changed, 126 insertions(+), 12 deletions(-) create mode 100644 tests/packages/test-circular-requirements/pyproject.toml diff --git a/src/build/__init__.py b/src/build/__init__.py index 41adff723..adb656c1c 100644 --- a/src/build/__init__.py +++ b/src/build/__init__.py @@ -54,6 +54,9 @@ _ExcInfoType = Union[Tuple[Type[BaseException], BaseException, types.TracebackType], Tuple[None, None, None]] +_SDIST_NAME_REGEX = re.compile(r'(?P.+)-(?P.+)\.tar.gz') + + _WHEEL_NAME_REGEX = re.compile( r'(?P.+)-(?P.+)' r'(-(?P.+))?-(?P.+)' @@ -104,6 +107,19 @@ def __str__(self) -> str: return f'Failed to validate `build-system` in pyproject.toml: {self.args[0]}' +class CircularBuildSystemDependencyError(BuildException): + """ + Exception raised when a ``[build-system]`` requirement in pyproject.toml is circular. + """ + + def __str__(self) -> str: + cycle_deps = self.args[0] + cycle_err_str = f'`{cycle_deps[0]}`' + for dep in cycle_deps[1:]: + cycle_err_str += f' -> `{dep}`' + return f'Failed to validate `build-system` in pyproject.toml, dependency cycle detected: {cycle_err_str}' + + class TypoWarning(Warning): """ Warning raised when a potential typo is found @@ -131,8 +147,16 @@ def _validate_source_directory(srcdir: PathType) -> None: raise BuildException(f'Source {srcdir} does not appear to be a Python project: no pyproject.toml or setup.py') +# https://www.python.org/dev/peps/pep-0503/#normalized-names +def _normalize(name: str) -> str: + return re.sub(r'[-_.]+', '-', name).lower() + + def check_dependency( - req_string: str, ancestral_req_strings: Tuple[str, ...] = (), parent_extras: AbstractSet[str] = frozenset() + req_string: str, + ancestral_req_strings: Tuple[str, ...] = (), + parent_extras: AbstractSet[str] = frozenset(), + project_name: Optional[str] = None, ) -> Iterator[Tuple[str, ...]]: """ Verify that a dependency and all of its dependencies are met. @@ -159,6 +183,12 @@ def check_dependency( # dependency is satisfied. return + # Front ends SHOULD check explicitly for requirement cycles, and + # terminate the build with an informative message if one is found. + # https://www.python.org/dev/peps/pep-0517/#build-requirements + if project_name is not None and _normalize(req.name) == _normalize(project_name): + raise CircularBuildSystemDependencyError((project_name,) + ancestral_req_strings + (req_string,)) + try: dist = importlib_metadata.distribution(req.name) # type: ignore[no-untyped-call] except importlib_metadata.PackageNotFoundError: @@ -171,7 +201,7 @@ def check_dependency( elif dist.requires: for other_req_string in dist.requires: # yields transitive dependencies that are not satisfied. - yield from check_dependency(other_req_string, ancestral_req_strings + (req_string,), req.extras) + yield from check_dependency(other_req_string, ancestral_req_strings + (req_string,), req.extras, project_name) def _find_typo(dictionary: Mapping[str, str], expected: str) -> None: @@ -222,6 +252,23 @@ def _parse_build_system_table(pyproject_toml: Mapping[str, Any]) -> Dict[str, An return build_system_table +def _parse_project_name(pyproject_toml: Mapping[str, Any]) -> Optional[str]: + if 'project' not in pyproject_toml: + return None + + project_table = dict(pyproject_toml['project']) + + # If [project] is present, it must have a ``name`` field (per PEP 621) + if 'name' not in project_table: + return None + + project_name = project_table['name'] + if not isinstance(project_name, str): + return None + + return project_name + + class ProjectBuilder: """ The PEP 517 consumer API. @@ -267,10 +314,14 @@ def __init__( except TOMLDecodeError as e: raise BuildException(f'Failed to parse {spec_file}: {e} ') + self.project_name: Optional[str] = _parse_project_name(spec) self._build_system = _parse_build_system_table(spec) self._backend = self._build_system['build-backend'] self._scripts_dir = scripts_dir self._hook_runner = runner + self._in_tree_build = False + if 'backend-path' in self._build_system: + self._in_tree_build = True self._hook = pep517.wrappers.Pep517HookCaller( self.srcdir, self._backend, @@ -341,6 +392,17 @@ def get_requires_for_build(self, distribution: str, config_settings: Optional[Co with self._handle_backend(hook_name): return set(get_requires(config_settings)) + def check_build_dependencies(self) -> Set[Tuple[str, ...]]: + """ + Return the dependencies which are not satisfied from + :attr:`build_system_requires` + + :returns: Set of variable-length unmet dependency tuples + """ + if self._in_tree_build: + return set() + return {u for d in self.build_system_requires for u in check_dependency(d, project_name=self.project_name)} + def check_dependencies( self, distribution: str, config_settings: Optional[ConfigSettingsType] = None ) -> Set[Tuple[str, ...]]: @@ -353,8 +415,9 @@ def check_dependencies( :param config_settings: Config settings for the build backend :returns: Set of variable-length unmet dependency tuples """ - dependencies = self.get_requires_for_build(distribution, config_settings).union(self.build_system_requires) - return {u for d in dependencies for u in check_dependency(d)} + build_system_dependencies = self.check_build_dependencies() + dependencies = {u for d in self.get_requires_for_build(distribution, config_settings) for u in check_dependency(d)} + return dependencies.union(build_system_dependencies) def prepare( self, distribution: str, output_directory: PathType, config_settings: Optional[ConfigSettingsType] = None @@ -399,7 +462,15 @@ def build( """ self.log(f'Building {distribution}...') kwargs = {} if metadata_directory is None else {'metadata_directory': metadata_directory} - return self._call_backend(f'build_{distribution}', output_directory, config_settings, **kwargs) + basename = self._call_backend(f'build_{distribution}', output_directory, config_settings, **kwargs) + match = None + if distribution == 'wheel': + match = _WHEEL_NAME_REGEX.match(os.path.basename(basename)) + elif distribution == 'sdist': + match = _SDIST_NAME_REGEX.match(os.path.basename(basename)) + if match: + self.project_name = match['distribution'] + return basename def metadata_path(self, output_directory: PathType) -> str: """ @@ -413,6 +484,9 @@ def metadata_path(self, output_directory: PathType) -> str: # prepare_metadata hook metadata = self.prepare('wheel', output_directory) if metadata is not None: + match = _WHEEL_NAME_REGEX.match(os.path.basename(metadata)) + if match: + self.project_name = match['distribution'] return metadata # fallback to build_wheel hook @@ -420,6 +494,7 @@ def metadata_path(self, output_directory: PathType) -> str: match = _WHEEL_NAME_REGEX.match(os.path.basename(wheel)) if not match: raise ValueError('Invalid wheel') + self.project_name = match['distribution'] distinfo = f"{match['distribution']}-{match['version']}.dist-info" member_prefix = f'{distinfo}/' with zipfile.ZipFile(wheel) as w: diff --git a/src/build/__main__.py b/src/build/__main__.py index d5685cd9d..4db735b69 100644 --- a/src/build/__main__.py +++ b/src/build/__main__.py @@ -99,16 +99,29 @@ def _format_dep_chain(dep_chain: Sequence[str]) -> str: def _build_in_isolated_env( - builder: ProjectBuilder, outdir: PathType, distribution: str, config_settings: Optional[ConfigSettingsType] + builder: ProjectBuilder, + outdir: PathType, + distribution: str, + config_settings: Optional[ConfigSettingsType], + skip_dependency_check: bool = False, ) -> str: with _IsolatedEnvBuilder() as env: builder.python_executable = env.executable builder.scripts_dir = env.scripts_dir # first install the build dependencies env.install(builder.build_system_requires) + # validate build system dependencies + revalidate = False + if not skip_dependency_check: + builder.check_build_dependencies() + if builder.project_name is None: + revalidate = True # then get the extra required dependencies from the backend (which was installed in the call above :P) env.install(builder.get_requires_for_build(distribution)) - return builder.build(distribution, outdir, config_settings or {}) + build_result = builder.build(distribution, outdir, config_settings or {}) + if revalidate and builder.project_name is not None: + builder.check_build_dependencies() + return build_result def _build_in_current_env( @@ -118,14 +131,22 @@ def _build_in_current_env( config_settings: Optional[ConfigSettingsType], skip_dependency_check: bool = False, ) -> str: + revalidate = False if not skip_dependency_check: missing = builder.check_dependencies(distribution) if missing: dependencies = ''.join('\n\t' + dep for deps in missing for dep in (deps[0], _format_dep_chain(deps[1:])) if dep) print() _error(f'Missing dependencies:{dependencies}') + elif builder.project_name is None: + revalidate = True + + build_result = builder.build(distribution, outdir, config_settings or {}) + + if revalidate and builder.project_name is not None: + builder.check_build_dependencies() - return builder.build(distribution, outdir, config_settings or {}) + return build_result def _build( @@ -137,7 +158,7 @@ def _build( skip_dependency_check: bool, ) -> str: if isolation: - return _build_in_isolated_env(builder, outdir, distribution, config_settings) + return _build_in_isolated_env(builder, outdir, distribution, config_settings, skip_dependency_check) else: return _build_in_current_env(builder, outdir, distribution, config_settings, skip_dependency_check) diff --git a/tests/packages/test-circular-requirements/pyproject.toml b/tests/packages/test-circular-requirements/pyproject.toml new file mode 100644 index 000000000..7853dd236 --- /dev/null +++ b/tests/packages/test-circular-requirements/pyproject.toml @@ -0,0 +1,7 @@ +[build-system] +requires = ["recursive_dep"] + +[project] +name = "recursive_unmet_dep" +version = "1.0.0" +description = "circular project" diff --git a/tests/test_projectbuilder.py b/tests/test_projectbuilder.py index 98687f88d..a0b52beda 100644 --- a/tests/test_projectbuilder.py +++ b/tests/test_projectbuilder.py @@ -5,6 +5,7 @@ import importlib import logging import os +import pathlib import sys import textwrap @@ -19,8 +20,6 @@ else: # pragma: no cover import importlib_metadata -import pathlib - build_open_owner = 'builtins' @@ -133,6 +132,18 @@ def test_check_dependency(monkeypatch, requirement_string, expected): assert next(build.check_dependency(requirement_string), None) == expected +@pytest.mark.parametrize('distribution', ['wheel', 'sdist']) +def test_build_no_isolation_circular_requirements(monkeypatch, package_test_circular_requirements, distribution): + monkeypatch.setattr(importlib_metadata, 'Distribution', MockDistribution) + msg = ( + 'Failed to validate `build-system` in pyproject.toml, dependency cycle detected: `recursive_unmet_dep` -> ' + '`recursive_dep` -> `recursive_unmet_dep`' + ) + builder = build.ProjectBuilder(package_test_circular_requirements) + with pytest.raises(build.CircularBuildSystemDependencyError, match=msg): + builder.check_build_dependencies() + + def test_bad_project(package_test_no_project): # Passing a nonexistent project directory with pytest.raises(build.BuildException): @@ -570,7 +581,7 @@ def test_log(mocker, caplog, package_test_flit): ('INFO', 'something'), ] if sys.version_info >= (3, 8): # stacklevel - assert caplog.records[-1].lineno == 562 + assert caplog.records[-1].lineno == test_log.__code__.co_firstlineno + 11 @pytest.mark.parametrize(