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

Pick up hashes from contraints files and intersect #8839

Merged
merged 4 commits into from
Sep 17, 2020
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions news/8792.bugfix
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
New resolver: Pick up hash declarations in constraints files and use them to
filter available distributions.
3 changes: 3 additions & 0 deletions news/8839.bugfix
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
New resolver: If a package appears multiple times in user specification with
different ``--hash`` options, only hashes that present in all specifications
should be allowed.
37 changes: 36 additions & 1 deletion src/pip/_internal/resolution/resolvelib/base.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
from pip._vendor.packaging.specifiers import SpecifierSet
from pip._vendor.packaging.utils import canonicalize_name

from pip._internal.req.req_install import InstallRequirement
from pip._internal.utils.hashes import Hashes
from pip._internal.utils.typing import MYPY_CHECK_RUNNING

if MYPY_CHECK_RUNNING:
Expand All @@ -8,7 +11,6 @@
from pip._vendor.packaging.version import _BaseVersion

from pip._internal.models.link import Link
from pip._internal.req.req_install import InstallRequirement

CandidateLookup = Tuple[
Optional["Candidate"],
Expand All @@ -24,6 +26,39 @@ def format_name(project, extras):
return "{}[{}]".format(project, ",".join(canonical_extras))


class Constraint(object):
def __init__(self, specifier, hashes):
# type: (SpecifierSet, Hashes) -> None
self.specifier = specifier
self.hashes = hashes

@classmethod
def empty(cls):
# type: () -> Constraint
return Constraint(SpecifierSet(), Hashes())

@classmethod
def from_ireq(cls, ireq):
# type: (InstallRequirement) -> Constraint
return Constraint(ireq.specifier, ireq.hashes(trust_internet=False))

def __nonzero__(self):
# type: () -> bool
return bool(self.specifier) or bool(self.hashes)

def __bool__(self):
# type: () -> bool
return self.__nonzero__()

def __and__(self, other):
# type: (InstallRequirement) -> Constraint
if not isinstance(other, InstallRequirement):
return NotImplemented
specifier = self.specifier & other.specifier
hashes = self.hashes & other.hashes(trust_internet=False)
return Constraint(specifier, hashes)


class Requirement(object):
@property
def name(self):
Expand Down
13 changes: 9 additions & 4 deletions src/pip/_internal/resolution/resolvelib/factory.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
from pip._internal.utils.typing import MYPY_CHECK_RUNNING
from pip._internal.utils.virtualenv import running_under_virtualenv

from .base import Constraint
from .candidates import (
AlreadyInstalledCandidate,
EditableCandidate,
Expand Down Expand Up @@ -152,6 +153,7 @@ def _iter_found_candidates(
self,
ireqs, # type: Sequence[InstallRequirement]
specifier, # type: SpecifierSet
hashes, # type: Hashes
):
# type: (...) -> Iterable[Candidate]
if not ireqs:
Expand All @@ -164,11 +166,10 @@ def _iter_found_candidates(
template = ireqs[0]
name = canonicalize_name(template.req.name)

hashes = Hashes()
extras = frozenset() # type: FrozenSet[str]
for ireq in ireqs:
specifier &= ireq.req.specifier
hashes |= ireq.hashes(trust_internet=False)
hashes &= ireq.hashes(trust_internet=False)
extras |= frozenset(ireq.extras)

# We use this to ensure that we only yield a single candidate for
Expand Down Expand Up @@ -218,7 +219,7 @@ def _iter_found_candidates(
return six.itervalues(candidates)

def find_candidates(self, requirements, constraint):
# type: (Sequence[Requirement], SpecifierSet) -> Iterable[Candidate]
# type: (Sequence[Requirement], Constraint) -> Iterable[Candidate]
explicit_candidates = set() # type: Set[Candidate]
ireqs = [] # type: List[InstallRequirement]
for req in requirements:
Expand All @@ -231,7 +232,11 @@ def find_candidates(self, requirements, constraint):
# If none of the requirements want an explicit candidate, we can ask
# the finder for candidates.
if not explicit_candidates:
return self._iter_found_candidates(ireqs, constraint)
return self._iter_found_candidates(
ireqs,
constraint.specifier,
constraint.hashes,
)

if constraint:
name = explicit_candidates.pop().name
Expand Down
7 changes: 4 additions & 3 deletions src/pip/_internal/resolution/resolvelib/provider.py
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
from pip._vendor.packaging.specifiers import SpecifierSet
from pip._vendor.resolvelib.providers import AbstractProvider

from pip._internal.utils.typing import MYPY_CHECK_RUNNING

from .base import Constraint

if MYPY_CHECK_RUNNING:
from typing import (
Any,
Expand Down Expand Up @@ -41,7 +42,7 @@ class PipProvider(AbstractProvider):
def __init__(
self,
factory, # type: Factory
constraints, # type: Dict[str, SpecifierSet]
constraints, # type: Dict[str, Constraint]
ignore_dependencies, # type: bool
upgrade_strategy, # type: str
user_requested, # type: Set[str]
Expand Down Expand Up @@ -134,7 +135,7 @@ def find_matches(self, requirements):
if not requirements:
return []
constraint = self._constraints.get(
requirements[0].name, SpecifierSet(),
requirements[0].name, Constraint.empty(),
)
candidates = self._factory.find_candidates(requirements, constraint)
return reversed(self._sort_matches(candidates))
Expand Down
8 changes: 4 additions & 4 deletions src/pip/_internal/resolution/resolvelib/resolver.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,12 @@
from pip._internal.utils.misc import dist_is_editable
from pip._internal.utils.typing import MYPY_CHECK_RUNNING

from .base import Constraint
from .factory import Factory

if MYPY_CHECK_RUNNING:
from typing import Dict, List, Optional, Set, Tuple

from pip._vendor.packaging.specifiers import SpecifierSet
from pip._vendor.resolvelib.resolvers import Result
from pip._vendor.resolvelib.structs import Graph

Expand Down Expand Up @@ -71,7 +71,7 @@ def __init__(
def resolve(self, root_reqs, check_supported_wheels):
# type: (List[InstallRequirement], bool) -> RequirementSet

constraints = {} # type: Dict[str, SpecifierSet]
constraints = {} # type: Dict[str, Constraint]
user_requested = set() # type: Set[str]
requirements = []
for req in root_reqs:
Expand All @@ -84,9 +84,9 @@ def resolve(self, root_reqs, check_supported_wheels):
continue
name = canonicalize_name(req.name)
if name in constraints:
constraints[name] = constraints[name] & req.specifier
constraints[name] &= req
else:
constraints[name] = req.specifier
constraints[name] = Constraint.from_ireq(req)
else:
if req.user_supplied and req.name:
user_requested.add(canonicalize_name(req.name))
Expand Down
20 changes: 14 additions & 6 deletions src/pip/_internal/utils/hashes.py
Original file line number Diff line number Diff line change
Expand Up @@ -46,16 +46,24 @@ def __init__(self, hashes=None):
"""
self._allowed = {} if hashes is None else hashes

def __or__(self, other):
def __and__(self, other):
# type: (Hashes) -> Hashes
if not isinstance(other, Hashes):
return NotImplemented
new = self._allowed.copy()

# If either of the Hashes object is entirely empty (i.e. no hash
# specified at all), all hashes from the other object are allowed.
if not other:
return self
if not self:
return other

# Otherwise only hashes that present in both objects are allowed.
new = {}
for alg, values in iteritems(other._allowed):
try:
new[alg] += values
except KeyError:
new[alg] = values
if alg not in self._allowed:
continue
new[alg] = [v for v in values if v in self._allowed[alg]]
return Hashes(new)

@property
Expand Down
125 changes: 125 additions & 0 deletions tests/functional/test_new_resolver_hashes.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,125 @@
import collections
import hashlib

import pytest

from pip._internal.utils.urls import path_to_url
from tests.lib import (
create_basic_sdist_for_package,
create_basic_wheel_for_package,
)

_FindLinks = collections.namedtuple(
"_FindLinks", "index_html sdist_hash wheel_hash",
)


def _create_find_links(script):
sdist_path = create_basic_sdist_for_package(script, "base", "0.1.0")
wheel_path = create_basic_wheel_for_package(script, "base", "0.1.0")

sdist_hash = hashlib.sha256(sdist_path.read_bytes()).hexdigest()
wheel_hash = hashlib.sha256(wheel_path.read_bytes()).hexdigest()

index_html = script.scratch_path / "index.html"
index_html.write_text(
"""
<a href="{sdist_url}#sha256={sdist_hash}">{sdist_path.stem}</a>
<a href="{wheel_url}#sha256={wheel_hash}">{wheel_path.stem}</a>
""".format(
sdist_url=path_to_url(sdist_path),
sdist_hash=sdist_hash,
sdist_path=sdist_path,
wheel_url=path_to_url(wheel_path),
wheel_hash=wheel_hash,
wheel_path=wheel_path,
)
)

return _FindLinks(index_html, sdist_hash, wheel_hash)


@pytest.mark.parametrize(
"requirements_template, message",
[
(
"""
base==0.1.0 --hash=sha256:{sdist_hash} --hash=sha256:{wheel_hash}
base==0.1.0 --hash=sha256:{sdist_hash} --hash=sha256:{wheel_hash}
""",
"Checked 2 links for project {name!r} against 2 hashes "
"(2 matches, 0 no digest): discarding no candidates",
),
(
# Different hash lists are intersected.
"""
base==0.1.0 --hash=sha256:{sdist_hash} --hash=sha256:{wheel_hash}
base==0.1.0 --hash=sha256:{sdist_hash}
""",
"Checked 2 links for project {name!r} against 1 hashes "
"(1 matches, 0 no digest): discarding 1 non-matches",
),
],
ids=["identical", "intersect"],
Comment on lines +53 to +63
Copy link
Member

Choose a reason for hiding this comment

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

Can we add a case of the intersection being empty, to ensure we test the error message as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you mean a test for something like this?

base==0.1.0
base==0.1.0 --hash=... --hash=...

The error you get here is unrelated to the intersection logic. This input is rejected straight on parsing since the requirements parser checks every requirement line in the file contains at least one hash. This behaviour definitely needs to be tested (I’m not sure if it is currently), but not here IMO. It is a part of the requirements file parser, not the resolver.

Copy link
Member

Choose a reason for hiding this comment

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

I meant something like

base == 1.0.0 --hash={sdist_hash}
base == 1.0.0 --hash={wheel_hash}

The intersection of these hashes would be empty, which is what I'm suggesting it might be worthwhile to test.

Copy link
Member Author

Choose a reason for hiding this comment

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

I see, that makes sense. Will update soon.

Copy link
Member Author

@uranusjr uranusjr Sep 11, 2020

Choose a reason for hiding this comment

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

Hmm, so pip install fails as expected, but the error message seems suboptimal. Since the two lines are two separate requirements, the error users see is

ERROR: THESE PACKAGES DO NOT MATCH THE HASHES FROM THE REQUIREMENTS FILE.
If you have updated the package versions, please update the hashes. Otherwise,
examine the package contents carefully; someone may have tampered with them.
     base==0.1.0 from ...:
         Expected sha256 {wheel_hash}
              Got        {sdist_hash}

(The hashes in Expected and Got flip if you flip the requirement lines.)

But improving the error message seems out of scope of this fix (and I’m not even sure how to improve it). I will just test the errors as-is for now and open a new issue.

)
def test_new_resolver_hash_intersect(script, requirements_template, message):
find_links = _create_find_links(script)

requirements_txt = script.scratch_path / "requirements.txt"
requirements_txt.write_text(
requirements_template.format(
sdist_hash=find_links.sdist_hash,
wheel_hash=find_links.wheel_hash,
),
)

result = script.pip(
"install",
"--use-feature=2020-resolver",
"--no-cache-dir",
"--no-deps",
"--no-index",
"--find-links", find_links.index_html,
"--verbose",
"--requirement", requirements_txt,
)

assert message.format(name=u"base") in result.stdout, str(result)


def test_new_resolver_hash_intersect_from_constraint(script):
find_links = _create_find_links(script)

constraints_txt = script.scratch_path / "constraints.txt"
constraints_txt.write_text(
"base==0.1.0 --hash=sha256:{sdist_hash}".format(
sdist_hash=find_links.sdist_hash,
),
)
requirements_txt = script.scratch_path / "requirements.txt"
requirements_txt.write_text(
"""
base==0.1.0 --hash=sha256:{sdist_hash} --hash=sha256:{wheel_hash}
""".format(
sdist_hash=find_links.sdist_hash,
wheel_hash=find_links.wheel_hash,
),
)

result = script.pip(
"install",
"--use-feature=2020-resolver",
"--no-cache-dir",
"--no-deps",
"--no-index",
"--find-links", find_links.index_html,
"--verbose",
"--constraint", constraints_txt,
"--requirement", requirements_txt,
)

message = (
"Checked 2 links for project {name!r} against 1 hashes "
"(1 matches, 0 no digest): discarding 1 non-matches"
).format(name=u"base")
assert message in result.stdout, str(result)
7 changes: 3 additions & 4 deletions tests/unit/resolution_resolvelib/test_requirement.py
Original file line number Diff line number Diff line change
@@ -1,8 +1,7 @@
import pytest
from pip._vendor.packaging.specifiers import SpecifierSet
from pip._vendor.resolvelib import BaseReporter, Resolver

from pip._internal.resolution.resolvelib.base import Candidate
from pip._internal.resolution.resolvelib.base import Candidate, Constraint
from pip._internal.utils.urls import path_to_url

# NOTE: All tests are prefixed `test_rlr` (for "test resolvelib resolver").
Expand Down Expand Up @@ -59,7 +58,7 @@ def test_new_resolver_correct_number_of_matches(test_cases, factory):
"""Requirements should return the correct number of candidates"""
for spec, _, match_count in test_cases:
req = factory.make_requirement_from_spec(spec, comes_from=None)
matches = factory.find_candidates([req], SpecifierSet())
matches = factory.find_candidates([req], Constraint.empty())
assert len(list(matches)) == match_count


Expand All @@ -68,7 +67,7 @@ def test_new_resolver_candidates_match_requirement(test_cases, factory):
"""
for spec, _, _ in test_cases:
req = factory.make_requirement_from_spec(spec, comes_from=None)
for c in factory.find_candidates([req], SpecifierSet()):
for c in factory.find_candidates([req], Constraint.empty()):
assert isinstance(c, Candidate)
assert req.is_satisfied_by(c)

Expand Down