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 all 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.
21 changes: 17 additions & 4 deletions src/pip/_internal/resolution/resolvelib/factory.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -273,14 +273,27 @@ 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

pinned = is_pinned(specifier)
Comment on lines +281 to +292
Copy link
Member

Choose a reason for hiding this comment

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

Not for this PR, but we might want to synchronise this with how the other is_pinned works in the codebase.

Copy link
Member

@uranusjr uranusjr Nov 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I believe there’re two other pinned logic that can be unified:

  1. In provider.py, there’s a pinned = any(op[:2] == "==" for op in operators) that can be tightened up. This one just offers a “hint” to the resolver so the current logic is not very careful with the definition of “pinned”, but since we have a proper implementation now it’s a good idea to reuse it.
  2. There’s an is_pinned property in req_install.py. This one is a stright up bug that can only be triggered by the legacy resolver. But again, it’s a good idea to also fix it if it’s not too much effort. We don’t know if the code path will be needed by future code.

Copy link
Member

Choose a reason for hiding this comment

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

If it isn't put in provider.py this PR I'm happy to submit a PR after this lands.

Currently pinned is considered more important than if a requirement is a backtracking cause in backtracking situations (I have a logical argument and tested evidence for this being faster), so being accurate about it probably reduces the chance of heavy backtracking scenarios.


# PackageFinder returns earlier versions first, so we reverse.
for ican in reversed(icans):
if not all_yanked and ican.link.is_yanked:
if not (all_yanked and pinned) and ican.link.is_yanked:
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