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

Fix a bug where pip-sync would unexpectedly uninstall some packages #1919

Merged
merged 3 commits into from
Jul 18, 2023
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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions piptools/sync.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
direct_url_as_pep440_direct_reference,
direct_url_from_link,
)
from pip._vendor.packaging.utils import canonicalize_name

from ._compat import Distribution, get_dev_pkgs
from .exceptions import IncompatibleRequirements
Expand Down Expand Up @@ -59,7 +60,7 @@ def dependency_tree(

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have to worry about root_key and the installed_keys keys potentially being differently formatted at the time of the if root_key in installed_keys check?

Copy link
Member Author

Choose a reason for hiding this comment

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

Valid point. Thanks! This is why we should use NormalizedName type instead of str for package names. I'm going to address that in a follow-up PR as commented here.

while queue:
v = queue.popleft()
key = v.key
key = str(canonicalize_name(v.key))
Copy link
Member Author

@atugushev atugushev Jul 17, 2023

Choose a reason for hiding this comment

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

FTR: I'd prefer an explicit NormalizedName type instead of str conversion, so I'll refactor the types in a follow-up PR.

if key in dependencies:
continue

Expand All @@ -85,7 +86,7 @@ def get_dists_to_ignore(installed: Iterable[Distribution]) -> list[str]:
locally, click should also be installed/uninstalled depending on the given
requirements.
"""
installed_keys = {r.key: r for r in installed}
installed_keys = {str(canonicalize_name(r.key)): r for r in installed}
return list(
flat_map(lambda req: dependency_tree(installed_keys, req), PACKAGES_TO_IGNORE)
)
Expand Down Expand Up @@ -143,7 +144,7 @@ def diff_key_from_ireq(ireq: InstallRequirement) -> str:

def diff_key_from_req(req: Distribution) -> str:
"""Get a unique key for the requirement."""
key = req.key
key = str(canonicalize_name(req.key))
if (
req.direct_url
and isinstance(req.direct_url.info, ArchiveInfo)
Expand Down
2 changes: 1 addition & 1 deletion tests/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ def _fake_dist(line, deps=None):
if deps is None:
deps = []
req = Requirement.parse(line)
key = req.key
key = req.name
if "==" in line:
version = line.split("==")[1]
else:
Expand Down
27 changes: 25 additions & 2 deletions tests/test_sync.py
Original file line number Diff line number Diff line change
Expand Up @@ -226,16 +226,18 @@ def test_diff_leave_packaging_packages_alone(fake_dist, from_line):
assert to_uninstall == {"first"}


def test_diff_leave_piptools_alone(fake_dist, from_line):
def test_diff_leave_piptools_and_its_dependencies_alone(fake_dist, from_line):
# Suppose an env contains Django, and pip-tools itself (including all of
# its dependencies)
installed = [
fake_dist("django==1.7"),
fake_dist("first==2.0.1"),
fake_dist("pip-tools==1.1.1", ["click>=4", "first", "six"]),
fake_dist("pip-tools==1.1.1", ["click>=4", "first", "six", "build"]),
fake_dist("six==1.9.0"),
fake_dist("click==4.1"),
fake_dist("foobar==0.3.6"),
fake_dist("build==0.10.0", ["pyproject_hooks"]),
fake_dist("pyproject_hooks==1.0.0"),
]

# Then this Django-only requirement should keep pip around (i.e. NOT
Expand Down Expand Up @@ -274,6 +276,27 @@ def test_diff_with_matching_url_hash(fake_dist, from_line):
assert to_uninstall == set()


@pytest.mark.parametrize(
("installed_dist", "compiled_req"),
(
pytest.param("Django==1.7", "django==1.7", id="case insensitive"),
pytest.param(
"jaraco.classes==3.2",
"jaraco-classes==3.2",
id="different namespace notation",
),
),
)
def test_diff_respects_canonical_package_names(
fake_dist, from_line, installed_dist, compiled_req
):
installed = [fake_dist(installed_dist)]
reqs = [from_line(compiled_req)]
to_install, to_uninstall = diff(reqs, installed)
assert to_install == set()
assert to_uninstall == set()


def test_diff_with_no_url_hash(fake_dist, from_line):
# if URL hash is not provided, assume the contents have
# changed and reinstall
Expand Down