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.

Sign up for GitHub

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

Jump to bottom

Skip yanked releases unless specified #10625

Merged
merged 7 commits into from
Jan 24, 2022
Merged

Skip yanked releases unless specified #10625

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
6b62c28
Skip yanked releases unless specified
albertosottile Oct 30, 2021
99cb1e0
Add test_error_all_yanked_files_and_no_pin test
albertosottile Nov 6, 2021
8293826
Change specifier detection logic to exclude compatible release clauses
albertosottile Nov 6, 2021
2f3983e
Minor fix in news file
albertosottile Nov 6, 2021
51bda96
Lint
albertosottile Nov 6, 2021
d1ba9d5
Fix permissions
albertosottile Nov 7, 2021
7057423
Implement suggested changes
albertosottile Nov 7, 2021
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/10617.bugfix.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Prevent pip from installing yanked releases unless
explicitely pinned via the ``==`` or ``===`` operators.
19 changes: 15 additions & 4 deletions src/pip/_internal/resolution/resolvelib/factory.py
100644 → 100755
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -273,14 +273,25 @@ def iter_index_candidate_infos() -> Iterator[IndexCandidateInfo]:
)
icans = list(result.iter_applicable())

# PEP 592: Yanked releases must be ignored unless only yanked
# releases can satisfy the version range. So if this is false,
# all yanked icans need to be skipped.
# PEP 592: Yanked releases are ignored unless the specifier
# explicitely pins a version (via '==' or '===') that can be
# solely satisfied by a yanked release.
all_yanked = all(ican.link.is_yanked for ican in icans)

def is_pinned(specifier: SpecifierSet) -> bool:
for sp in specifier:
if sp.operator == "===":
return True
if sp.operator != "==":
continue
if sp.version.endswith(".*"):
continue
return True
return False

# PackageFinder returns earlier versions first, so we reverse.
for ican in reversed(icans):
if not all_yanked and ican.link.is_yanked:
if (all_yanked and not is_pinned(specifier)) and ican.link.is_yanked:
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this condition correct? It would seem that if some of the releases are not yanked, then we would want to skip yanked releases. Currently, this turns into condition:

if (False and ...) and ican.link.is_yanked:
    ...

which means that the yanked releases won't get ignored.
If I understand this correctly, it should instead be:

Suggested change
if (all_yanked and not is_pinned(specifier)) and ican.link.is_yanked:
if not (all_yanked and is_pinned(specifier)) and ican.link.is_yanked:

Also I've noticed that is_pinned() is called for each ican even though the specifier argument is always the same. It's probably a premature optimization but it seems that this could be calculated just once.

Copy link
Contributor Author

@albertosottile albertosottile Nov 7, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Is this condition correct? It would seem that if some of the releases are not yanked, then we would want to skip yanked releases.

This is actually a good point, I trusted tests/functional/test_install.py::test_ignore_yanked_file for this but it turns out there might be a bug/undocumented feature in the code.

The test is pass, but only because the icans returned by PackageFinder looks like this:

ican=<InstallationCandidate('simple', <Version('3.0')>, <Link file:///tmp/pytest-of-root/pytest-0/test_ignore_yanked_file0/data/packages/simple-3.0.tar.gz (from file:///tmp/pytest-of-root/pytest-0/test_ignore_yanked_file0/data/indexes/yanked/simple/index.html)>)>
ican=<InstallationCandidate('simple', <Version('1.0')>, <Link file:///tmp/pytest-of-root/pytest-0/test_ignore_yanked_file0/data/packages/simple-1.0.tar.gz (from file:///tmp/pytest-of-root/pytest-0/test_ignore_yanked_file0/data/indexes/yanked/simple/index.html)>)>
ican=<InstallationCandidate('simple', <Version('2.0')>, <Link file:///tmp/pytest-of-root/pytest-0/test_ignore_yanked_file0/data/packages/simple-2.0.tar.gz (from file:///tmp/pytest-of-root/pytest-0/test_ignore_yanked_file0/data/indexes/yanked/simple/index.html)>)>

When reversed later, simple-2.0 becomes the first package, which passes the in-loop condition as it is not yanked and is therefore installed, making the test pass.

Altering the index used by the test and un-yanking simple-3.0 corrects the list order returned by PackageFinder:

ican=<InstallationCandidate('simple', <Version('1.0')>, <Link file:///tmp/pytest-of-root/pytest-0/test_ignore_yanked_file0/data/packages/simple-1.0.tar.gz (from file:///tmp/pytest-of-root/pytest-0/test_ignore_yanked_file0/data/indexes/yanked/simple/index.html)>)>
ican=<InstallationCandidate('simple', <Version('2.0')>, <Link file:///tmp/pytest-of-root/pytest-0/test_ignore_yanked_file0/data/packages/simple-2.0.tar.gz (from file:///tmp/pytest-of-root/pytest-0/test_ignore_yanked_file0/data/indexes/yanked/simple/index.html)>)>
ican=<InstallationCandidate('simple', <Version('3.0')>, <Link file:///tmp/pytest-of-root/pytest-0/test_ignore_yanked_file0/data/packages/simple-3.0.tar.gz (from file:///tmp/pytest-of-root/pytest-0/test_ignore_yanked_file0/data/indexes/yanked/simple/index.html)>)>

Hence, I am not sure on how/whether to fix this. If we can rely on this returned order, then both the code and the test are valid. Otherwise,

if not (all_yanked and is_pinned(specifier)) and ican.link.is_yanked:

This is fine for me as it passes both tests.

Also I've noticed that is_pinned() is called for each ican even though the specifier argument is always the same. It's probably a premature optimization but it seems that this could be calculated just once.

Good idea, I can adapt as proposed.

@uranusjr Should I implement both the proposed changes?

Copy link
Member

@uranusjr uranusjr Nov 7, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Let's fix the is_pinned() thing.

The yanked ordering one is more complicated. PackageFinder ordering yanked versions last is a designed feature, defined here:

pip/src/pip/_internal/index/package_finder.py

Lines 473 to 539 in d81c65a

def _sort_key(self, candidate: InstallationCandidate) -> CandidateSortingKey:
"""
Function to pass as the `key` argument to a call to sorted() to sort
InstallationCandidates by preference.
Returns a tuple such that tuples sorting as greater using Python's
default comparison operator are more preferred.
The preference is as follows:
First and foremost, candidates with allowed (matching) hashes are
always preferred over candidates without matching hashes. This is
because e.g. if the only candidate with an allowed hash is yanked,
we still want to use that candidate.
Second, excepting hash considerations, candidates that have been
yanked (in the sense of PEP 592) are always less preferred than
candidates that haven't been yanked. Then:
If not finding wheels, they are sorted by version only.
If finding wheels, then the sort order is by version, then:
1. existing installs
2. wheels ordered via Wheel.support_index_min(self._supported_tags)
3. source archives
If prefer_binary was set, then all wheels are sorted above sources.
Note: it was considered to embed this logic into the Link
comparison operators, but then different sdist links
with the same version, would have to be considered equal
"""
valid_tags = self._supported_tags
support_num = len(valid_tags)
build_tag: BuildTag = ()
binary_preference = 0
link = candidate.link
if link.is_wheel:
# can raise InvalidWheelFilename
wheel = Wheel(link.filename)
try:
pri = -(
wheel.find_most_preferred_tag(
valid_tags, self._wheel_tag_preferences
)
)
except ValueError:
raise UnsupportedWheel(
"{} is not a supported wheel for this platform. It "
"can't be sorted.".format(wheel.filename)
)
if self._prefer_binary:
binary_preference = 1
if wheel.build_tag is not None:
match = re.match(r"^(\d+)(.*)$", wheel.build_tag)
build_tag_groups = match.groups()
build_tag = (int(build_tag_groups[0]), build_tag_groups[1])
else: # sdist
pri = -(support_num)
has_allowed_hash = int(link.is_hash_allowed(self._hashes))
yank_value = -1 * int(link.is_yanked) # -1 for yanked.
return (
has_allowed_hash,
yank_value,
binary_preference,
candidate.version,
pri,
build_tag,
)

So it's OK to rely on this behaviour. The question is whether it's a good idea to do this when we have another solution. The answer to this is usually no, so let's change it. I actually have a feeling we might change the condition again later here specifically due to the prerelease issue (we'd want PackageFinder to return those, but still only allow them if all stable releases are yanked, and I think that's only doable if we rely on ordering). But let's worry about that later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, done.

continue
func = functools.partial(
self._make_candidate_from_link,
Expand Down
7 changes: 7 additions & 0 deletions tests/data/indexes/yanked_all/simple/index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
<html>
<body>
<a data-yanked="test reason message" href="../../../packages/simple-1.0.tar.gz">simple-1.0.tar.gz</a>
<a data-yanked="test reason message" href="../../../packages/simple-2.0.tar.gz">simple-2.0.tar.gz</a>
<a data-yanked="test reason message" href="../../../packages/simple-3.0.tar.gz">simple-3.0.tar.gz</a>
</body>
</html>
18 changes: 18 additions & 0 deletions tests/functional/test_install.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2030,6 +2030,24 @@ def test_install_yanked_file_and_print_warning(script, data):
assert "Successfully installed simple-3.0\n" in result.stdout, str(result)


def test_error_all_yanked_files_and_no_pin(script, data):
"""
Test raising an error if there are only "yanked" files available and no pin
"""
result = script.pip(
"install",
"simple",
"--index-url",
data.index_url("yanked_all"),
expect_error=True,
)
# Make sure an error is raised
assert (
result.returncode == 1
and "ERROR: No matching distribution found for simple\n" in result.stderr
), str(result)
uranusjr marked this conversation as resolved.
Show resolved Hide resolved


@pytest.mark.parametrize(
"install_args",
[
Expand Down