Repository: pypa/build
Pull request #183 (2020-11-08 03:14:17)

Enhance `check_version`

layday: I've made a few changes to return the dependency chain similar to how `pip check` does it.
layday: While we are waiting, would anybody know why the pypy3 tests are failing?
gaborbernat: > While we are waiting, would anybody know why the pypy3 tests are failing?

No, cold you investigate?
layday: It looks like it's flaky - it passed on Ubuntu this time.  Appears to be coverage-related, possibly https://github.com/nedbat/coveragepy/issues/1010, judging from this line:

```
E                   _sqlite3.InterfaceError: Error binding parameter 0 - probably unsupported type.
```

I don't know why it'd appear now, there might be some interaction with the `monkeypatch` fixture which I replaced the global overwrite of `importlib_metadata.Distribution` with?
layday: I dropped the commit which enabled branch coverage and that seems to have fixed it (or perhaps not).  We can add branch cov in a separate commit.
layday: Should we merge this now?

Patch:
diff --git a/src/build/__init__.py b/src/build/__init__.py
index 97af78d..b7473d8 100644
--- a/src/build/__init__.py
+++ b/src/build/__init__.py
@@ -11,7 +11,7 @@ import os
 import sys
 import warnings
 
-from typing import Iterator, Mapping, Optional, Sequence, Set, Text, Union
+from typing import AbstractSet, Iterator, Mapping, Optional, Sequence, Set, Text, Tuple, Union
 
 import pep517.wrappers
 import toml
@@ -50,16 +50,14 @@ class TypoWarning(Warning):
     """
 
 
-class IncompleteCheckWarning(Warning):
+def check_dependency(req_string, ancestral_req_strings=(), parent_extras=frozenset()):
+    # type: (str, Tuple[str, ...], AbstractSet[str]) -> Iterator[Tuple[str, ...]]
     """
-    Warning raised when we have an incomplete check
-    """
-
+    Verify that a dependency and all of its dependencies are met.
 
-def check_version(requirement_string, extra=''):  # type: (str, str) -> bool
-    """
-    :param requirement_string: Requirement string
-    :param extra: Extra (eg. test in myproject[test])
+    :param req_string: Requirement string
+    :param parent_extras: Extras (eg. "test" in myproject[test])
+    :yields: Unmet dependencies
     """
     import packaging.requirements
 
@@ -68,31 +66,31 @@ def check_version(requirement_string, extra=''):  # type: (str, str) -> bool
     else:
         import importlib_metadata
 
-    req = packaging.requirements.Requirement(requirement_string)
-    env = {'extra': extra}
+    req = packaging.requirements.Requirement(req_string)
 
-    if req.marker and not req.marker.evaluate(env):
-        return True
+    if req.marker:
+        extras = frozenset(('',)).union(parent_extras)
+        # a requirement can have multiple extras but ``evaluate`` can
+        # only check one at a time.
+        if all(not req.marker.evaluate(environment={'extra': e}) for e in extras):
+            # if the marker conditions are not met, we pretend that the
+            # dependency is satisfied.
+            return
 
     try:
-        version = importlib_metadata.version(req.name)
-        metadata = importlib_metadata.metadata(req.name)
+        dist = importlib_metadata.distribution(req.name)
     except importlib_metadata.PackageNotFoundError:
-        return False
-
-    for extra in req.extras:
-        if extra not in (metadata.get_all('Provides-Extra') or []):
-            return False
-        warnings.warn(
-            "Verified that the '{}[{}]' extra is present but did not verify that it is active "
-            '(its dependencies are met)'.format(req.name, extra),
-            IncompleteCheckWarning,
-        )
-
-    if req.specifier:
-        return req.specifier.contains(version)
-
-    return True
+        # dependency is not installed in the environment.
+        yield ancestral_req_strings + (req_string,)
+    else:
+        if req.specifier and dist.version not in req.specifier:
+            # the installed version is incompatible.
+            yield ancestral_req_strings + (req_string,)
+        elif dist.requires:
+            for other_req_string in dist.requires:
+                for unmet_req in check_dependency(other_req_string, ancestral_req_strings + (req_string,), req.extras):
+                    # a transitive dependency is not satisfied.
+                    yield unmet_req
 
 
 def _find_typo(dictionary, expected):  # type: (Mapping[str, str], str) -> None
@@ -208,18 +206,17 @@ class ProjectBuilder(object):
         except Exception as e:  # noqa: E722
             raise BuildBackendException('Backend operation failed: {}'.format(e))
 
-    def check_dependencies(self, distribution):  # type: (str) -> Set[str]
+    def check_dependencies(self, distribution):  # type: (str) -> Set[Tuple[str, ...]]
         """
         Return the dependencies which are not satisfied from the combined set of
         :attr:`build_dependencies` and :meth:`get_dependencies` for a given
         distribution.
 
         :param distribution: Distribution to check (``sdist`` or ``wheel``)
+        :returns: Set of variable-length unmet dependency tuples
         """
-        dependencies = self.get_dependencies(distribution)
-        dependencies.update(self.build_dependencies)
-
-        return {dep for dep in dependencies if not check_version(dep)}
+        dependencies = self.get_dependencies(distribution).union(self.build_dependencies)
+        return {u for d in dependencies for u in check_dependency(d)}
 
     def build(self, distribution, outdir):  # type: (str, str) -> None
         """
diff --git a/src/build/__main__.py b/src/build/__main__.py
index c6b0fd6..517aab9 100644
--- a/src/build/__main__.py
+++ b/src/build/__main__.py
@@ -6,7 +6,7 @@ import sys
 import traceback
 import warnings
 
-from typing import Iterable, List, Optional, TextIO, Type, Union
+from typing import Iterable, List, Optional, Sequence, TextIO, Type, Union
 
 import build
 
@@ -42,6 +42,10 @@ def _error(msg, code=1):  # type: (str, int) -> None  # pragma: no cover
     exit(code)
 
 
+def _format_dep_chain(dep_chain):  # type: (Sequence[str]) -> str
+    return ' -> '.join(dep.partition(';')[0].strip() for dep in dep_chain)
+
+
 def _build_in_isolated_env(builder, outdir, distributions):
     # type: (ProjectBuilder, str, List[str]) -> None
     with IsolatedEnvBuilder() as env:
@@ -57,7 +61,10 @@ def _build_in_current_env(builder, outdir, distributions, skip_dependencies=Fals
         if not skip_dependencies:
             missing = builder.check_dependencies(dist)
             if missing:
-                _error('Missing dependencies:' + ''.join(['\n\t' + dep for dep in missing]))
+                _error(
+                    'Missing dependencies:'
+                    + ''.join('\n\t' + dep for deps in missing for dep in (deps[0], _format_dep_chain(deps[1:])) if dep)
+                )
 
         builder.build(dist, outdir)
 


Test patch:
diff --git a/tests/test_main.py b/tests/test_main.py
index 1d4cefe..54e6ac0 100644
--- a/tests/test_main.py
+++ b/tests/test_main.py
@@ -121,16 +121,22 @@ def test_build_no_isolation_check_deps_empty(mocker, test_flit_path):
     build_cmd.assert_called_with('sdist', '.')
 
 
-def test_build_no_isolation_with_check_deps(mocker, test_flit_path):
-    # check_dependencies = ['something']
+@pytest.mark.parametrize(
+    ['missing_deps', 'output'],
+    [
+        ([('foo',)], '\n\tfoo'),
+        ([('foo',), ('bar', 'baz', 'qux')], '\n\tfoo\n\tbar\n\tbaz -> qux'),
+    ],
+)
+def test_build_no_isolation_with_check_deps(mocker, test_flit_path, missing_deps, output):
     error = mocker.patch('build.__main__._error')
     build_cmd = mocker.patch('build.ProjectBuilder.build')
-    mocker.patch('build.ProjectBuilder.check_dependencies', return_value=['something'])
+    mocker.patch('build.ProjectBuilder.check_dependencies', return_value=missing_deps)
 
     build.__main__.build_package(test_flit_path, '.', ['sdist'], isolation=False)
 
     build_cmd.assert_called_with('sdist', '.')
-    error.assert_called_with('Missing dependencies:\n\tsomething')
+    error.assert_called_with('Missing dependencies:' + output)
 
 
 @pytest.mark.isolated
diff --git a/tests/test_projectbuilder.py b/tests/test_projectbuilder.py
index 05e52b1..b354a4d 100644
--- a/tests/test_projectbuilder.py
+++ b/tests/test_projectbuilder.py
@@ -14,16 +14,10 @@ import build
 
 
 if sys.version_info >= (3, 8):  # pragma: no cover
-    import email
-
     from importlib import metadata as importlib_metadata
-
-    email_message_from_string = email.message_from_string
 else:  # pragma: no cover
     import importlib_metadata
 
-    email_message_from_string = importlib_metadata._compat.email_message_from_string
-
 if sys.version_info >= (3,):  # pragma: no cover
     build_open_owner = 'builtins'
 else:  # pragma: no cover
@@ -32,12 +26,6 @@ else:  # pragma: no cover
     PermissionError = OSError
 
 
-DUMMY_METADATA = """
-Version: 1.0.0
-Provides-Extra: some_extra
-""".strip()
-
-
 DEFAULT_BACKEND = {
     'build-backend': 'setuptools.build_meta:__legacy__',
     'requires': ['setuptools >= 40.8.0', 'wheel'],
@@ -45,52 +33,84 @@ DEFAULT_BACKEND = {
 
 
 class MockDistribution(importlib_metadata.Distribution):
-    def read_text(self, filename):
-        return DUMMY_METADATA
-
     def locate_file(self, path):  # pragma: no cover
         return ''
 
     @classmethod
     def from_name(cls, name):
-        if name == 'something':
-            return cls()
+        if name == 'extras_dep':
+            return ExtraMockDistribution()
+        elif name == 'recursive_dep':
+            return RecursiveMockDistribution()
+        elif name == 'requireless_dep':
+            return RequirelessMockDistribution()
         raise importlib_metadata.PackageNotFoundError
 
 
-importlib_metadata.Distribution = MockDistribution
+class ExtraMockDistribution(MockDistribution):
+    def read_text(self, filename):
+        return """
+Version: 1.0.0
+Provides-Extra: extra_without_associated_deps
+Provides-Extra: extra_with_unmet_deps
+Requires-Dist: unmet_dep; extra == 'extra_with_unmet_deps'
+Provides-Extra: extra_with_met_deps
+Requires-Dist: extras_dep; extra == 'extra_with_met_deps'
+Provides-Extra: recursive_extra_with_unmet_deps
+Requires-Dist: recursive_dep; extra == 'recursive_extra_with_unmet_deps'
+""".strip()
+
+
+class RecursiveMockDistribution(MockDistribution):
+    def read_text(self, filename):
+        return """
+Version: 1.0.0
+Requires-Dist: recursive_unmet_dep
+""".strip()
+
+
+class RequirelessMockDistribution(MockDistribution):
+    def read_text(self, filename):
+        return ''
 
 
 @pytest.mark.parametrize(
-    ('requirement_string', 'expected', 'extra_warning'),
+    ('requirement_string', 'expected'),
     [
-        ('something', True, False),
-        ('something_else', False, False),
-        ('something[extra]', False, False),
-        ('something[some_extra]', True, True),
-        ('something_else; python_version>"10"', True, False),
-        ('something_else; python_version<="1"', True, False),
-        ('something_else; python_version>="1"', False, False),
-        ('something == 1.0.0', True, False),
-        ('something == 2.0.0', False, False),
-        ('something[some_extra] == 1.0.0', True, True),
-        ('something[some_extra] == 2.0.0', False, True),
+        ('extras_dep', None),
+        ('missing_dep', ('missing_dep',)),
+        ('requireless_dep', None),
+        ('extras_dep[undefined_extra]', None),
+        # would the wheel builder filter this out?
+        ('extras_dep[extra_without_associated_deps]', None),
+        (
+            'extras_dep[extra_with_unmet_deps]',
+            ('extras_dep[extra_with_unmet_deps]', "unmet_dep; extra == 'extra_with_unmet_deps'"),
+        ),
+        (
+            'extras_dep[recursive_extra_with_unmet_deps]',
+            (
+                'extras_dep[recursive_extra_with_unmet_deps]',
+                "recursive_dep; extra == 'recursive_extra_with_unmet_deps'",
+                'recursive_unmet_dep',
+            ),
+        ),
+        ('extras_dep[extra_with_met_deps]', None),
+        ('missing_dep; python_version>"10"', None),
+        ('missing_dep; python_version<="1"', None),
+        ('missing_dep; python_version>="1"', ('missing_dep; python_version>="1"',)),
+        ('extras_dep == 1.0.0', None),
+        ('extras_dep == 2.0.0', ('extras_dep == 2.0.0',)),
+        ('extras_dep[extra_without_associated_deps] == 1.0.0', None),
+        ('extras_dep[extra_without_associated_deps] == 2.0.0', ('extras_dep[extra_without_associated_deps] == 2.0.0',)),
     ],
 )
-def test_check_version(requirement_string, expected, extra_warning):
-    if extra_warning:
-        with pytest.warns(build.IncompleteCheckWarning):
-            assert build.check_version(requirement_string) == expected
-    else:
-        assert build.check_version(requirement_string) == expected
+def test_check_dependency(monkeypatch, requirement_string, expected):
+    monkeypatch.setattr(importlib_metadata, 'Distribution', MockDistribution)
+    assert next(build.check_dependency(requirement_string), None) == expected
 
 
 def test_init(mocker, test_flit_path, legacy_path, test_no_permission, test_bad_syntax_path):
-    modules = {
-        'flit_core.buildapi': None,
-        'setuptools.build_meta:__legacy__': None,
-    }
-    mocker.patch('importlib.import_module', modules.get)
     mocker.patch('pep517.wrappers.Pep517HookCaller')
 
     # correct flit pyproject.toml
@@ -167,10 +187,8 @@ def test_build_missing_backend(packages_path, distribution, tmpdir):
 
 
 def test_check_dependencies(mocker, test_flit_path):
-    mocker.patch('importlib.import_module', autospec=True)
     mocker.patch('pep517.wrappers.Pep517HookCaller.get_requires_for_build_sdist')
     mocker.patch('pep517.wrappers.Pep517HookCaller.get_requires_for_build_wheel')
-    mocker.patch('build.check_version', autospec=True)
 
     builder = build.ProjectBuilder(test_flit_path)
 
@@ -180,17 +198,16 @@ def test_check_dependencies(mocker, test_flit_path):
         pep517.wrappers.BackendUnavailable,
     ]
 
-    build.check_version.return_value = False
     builder._hook.get_requires_for_build_sdist.side_effect = copy.copy(side_effects)
     builder._hook.get_requires_for_build_wheel.side_effect = copy.copy(side_effects)
 
     # requires = []
-    assert builder.check_dependencies('sdist') == {'flit_core >=2,<3'}
-    assert builder.check_dependencies('wheel') == {'flit_core >=2,<3'}
+    assert builder.check_dependencies('sdist') == {('flit_core >=2,<3',)}
+    assert builder.check_dependencies('wheel') == {('flit_core >=2,<3',)}
 
     # requires = ['something']
-    assert builder.check_dependencies('sdist') == {'flit_core >=2,<3', 'something'}
-    assert builder.check_dependencies('wheel') == {'flit_core >=2,<3', 'something'}
+    assert builder.check_dependencies('sdist') == {('flit_core >=2,<3',), ('something',)}
+    assert builder.check_dependencies('wheel') == {('flit_core >=2,<3',), ('something',)}
 
     # BackendUnavailable
     with pytest.raises(build.BuildBackendException):
@@ -206,7 +223,6 @@ def test_working_directory(tmp_dir):
 
 
 def test_build(mocker, test_flit_path, tmp_dir):
-    mocker.patch('importlib.import_module', autospec=True)
     mocker.patch('pep517.wrappers.Pep517HookCaller', autospec=True)
     mocker.patch('build._working_directory', autospec=True)
 
@@ -233,7 +249,6 @@ def test_build(mocker, test_flit_path, tmp_dir):
 
 
 def test_default_backend(mocker, legacy_path):
-    mocker.patch('importlib.import_module', autospec=True)
     mocker.patch('pep517.wrappers.Pep517HookCaller', autospec=True)
 
     builder = build.ProjectBuilder(legacy_path)
@@ -242,7 +257,6 @@ def test_default_backend(mocker, legacy_path):
 
 
 def test_missing_backend(mocker, test_no_backend_path):
-    mocker.patch('importlib.import_module', autospec=True)
     mocker.patch('pep517.wrappers.Pep517HookCaller', autospec=True)
 
     builder = build.ProjectBuilder(test_no_backend_path)
@@ -251,7 +265,6 @@ def test_missing_backend(mocker, test_no_backend_path):
 
 
 def test_missing_requires(mocker, test_no_requires_path):
-    mocker.patch('importlib.import_module', autospec=True)
     mocker.patch('pep517.wrappers.Pep517HookCaller', autospec=True)
 
     with pytest.raises(build.BuildException):
@@ -259,7 +272,6 @@ def test_missing_requires(mocker, test_no_requires_path):
 
 
 def test_build_system_typo(mocker, test_typo):
-    mocker.patch('importlib.import_module', autospec=True)
     mocker.patch('pep517.wrappers.Pep517HookCaller', autospec=True)
 
     with pytest.warns(build.TypoWarning):
@@ -267,7 +279,6 @@ def test_build_system_typo(mocker, test_typo):
 
 
 def test_missing_outdir(mocker, tmp_dir, test_flit_path):
-    mocker.patch('importlib.import_module', autospec=True)
     mocker.patch('pep517.wrappers.Pep517HookCaller', autospec=True)
 
     builder = build.ProjectBuilder(test_flit_path)
@@ -279,7 +290,6 @@ def test_missing_outdir(mocker, tmp_dir, test_flit_path):
 
 
 def test_relative_outdir(mocker, tmp_dir, test_flit_path):
-    mocker.patch('importlib.import_module', autospec=True)
     mocker.patch('pep517.wrappers.Pep517HookCaller', autospec=True)
 
     builder = build.ProjectBuilder(test_flit_path)
@@ -290,7 +300,6 @@ def test_relative_outdir(mocker, tmp_dir, test_flit_path):
 
 
 def test_not_dir_outdir(mocker, tmp_dir, test_flit_path):
-    mocker.patch('importlib.import_module', autospec=True)
     mocker.patch('pep517.wrappers.Pep517HookCaller', autospec=True)
 
     builder = build.ProjectBuilder(test_flit_path)