Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enhance check_version #183

Merged
merged 4 commits into from
Nov 14, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
67 changes: 32 additions & 35 deletions src/build/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
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
Expand Down Expand Up @@ -50,16 +50,14 @@ class TypoWarning(Warning):
"""


class IncompleteCheckWarning(Warning):
def check_dependency(req_string, ancestral_req_strings=(), parent_extras=frozenset()):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be better to put effort in upstreaming this into pypa/packaging first, as proposed in pypa/packaging#317.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The impression I got was that they were reluctant to add anything which manipulates distributions. distlib would probably be a good fit but everybody seems to be moving away from it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my experience there's a long cycle with adding stuff to distlib... so if we want it soon we better add it here. We can always remove it if we do manage to upstream it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am in no hurry to merge this as it only affects people using --no-isolation when trying to verify recursive extras (extras that depend on extras), which is mostly distro packagers, for whom this is only a nice to have. I would rather put the effort into merging somewhere this could live long term rather than here. Before merging it here, we should figure out what would it take to put it somewhere else and then make the call.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am in no hurry to merge this as it only affects people using --no-isolation

As a (new) contributor to this project, this sentence on its own would be enough for me to abandon spending any more effort (on this PR).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as it only affects people ... trying to verify recursive extras

check_version would not verify transitive dependencies at all, whether or not they came from extras.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes sorry, my bad.

# 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

Expand All @@ -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:
gaborbernat marked this conversation as resolved.
Show resolved Hide resolved
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
Expand Down Expand Up @@ -208,18 +206,17 @@ def get_dependencies(self, distribution): # type: (str) -> Set[str]
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
"""
Expand Down
11 changes: 9 additions & 2 deletions src/build/__main__.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
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

Expand Down Expand Up @@ -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:
Expand All @@ -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)

Expand Down
14 changes: 10 additions & 4 deletions tests/test_main.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down