Skip to content

Always reinstall local distributions provided by the user #9147

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

Merged
merged 8 commits into from
Nov 26, 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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 2 additions & 13 deletions src/pip/_internal/index/collector.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
from pip._internal.models.search_scope import SearchScope
from pip._internal.network.utils import raise_for_status
from pip._internal.utils.compat import lru_cache
from pip._internal.utils.filetypes import ARCHIVE_EXTENSIONS
from pip._internal.utils.filetypes import is_archive_file
from pip._internal.utils.misc import pairwise, redact_auth_from_url
from pip._internal.utils.typing import MYPY_CHECK_RUNNING
from pip._internal.utils.urls import path_to_url, url_to_path
Expand Down Expand Up @@ -65,17 +65,6 @@ def _match_vcs_scheme(url):
return None


def _is_url_like_archive(url):
# type: (str) -> bool
"""Return whether the URL looks like an archive.
"""
filename = Link(url).filename
for bad_ext in ARCHIVE_EXTENSIONS:
if filename.endswith(bad_ext):
return True
return False


class _NotHTML(Exception):
def __init__(self, content_type, request_desc):
# type: (str, str) -> None
Expand Down Expand Up @@ -130,7 +119,7 @@ def _get_html_response(url, session):
3. Check the Content-Type header to make sure we got HTML, and raise
`_NotHTML` otherwise.
"""
if _is_url_like_archive(url):
if is_archive_file(Link(url).filename):
_ensure_html_response(url, session=session)

logger.debug('Getting page %s', redact_auth_from_url(url))
Expand Down
13 changes: 2 additions & 11 deletions src/pip/_internal/req/constructors.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,8 @@
from pip._internal.pyproject import make_pyproject_path
from pip._internal.req.req_install import InstallRequirement
from pip._internal.utils.deprecation import deprecated
from pip._internal.utils.filetypes import ARCHIVE_EXTENSIONS
from pip._internal.utils.misc import is_installable_dir, splitext
from pip._internal.utils.filetypes import is_archive_file
from pip._internal.utils.misc import is_installable_dir
from pip._internal.utils.typing import MYPY_CHECK_RUNNING
from pip._internal.utils.urls import path_to_url
from pip._internal.vcs import is_url, vcs
Expand All @@ -45,15 +45,6 @@
operators = Specifier._operators.keys()


def is_archive_file(name):
# type: (str) -> bool
"""Return True if `name` is a considered as an archive file."""
ext = splitext(name)[1].lower()
if ext in ARCHIVE_EXTENSIONS:
return True
return False


def _strip_extras(path):
# type: (str) -> Tuple[str, Optional[str]]
m = re.match(r'^(.+)(\[[^\]]+\])$', path)
Expand Down
38 changes: 35 additions & 3 deletions src/pip/_internal/resolution/resolvelib/resolver.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@
PipDebuggingReporter,
PipReporter,
)
from pip._internal.utils.deprecation import deprecated
from pip._internal.utils.filetypes import is_archive_file
from pip._internal.utils.misc import dist_is_editable
from pip._internal.utils.typing import MYPY_CHECK_RUNNING

Expand Down Expand Up @@ -132,12 +134,14 @@ def resolve(self, root_reqs, check_supported_wheels):

# Check if there is already an installation under the same name,
# and set a flag for later stages to uninstall it, if needed.
# * There isn't, good -- no uninstalltion needed.
#
# * There is no existing installation. Nothing to uninstall.
# * The --force-reinstall flag is set. Always reinstall.
# * The installation is different in version or editable-ness, so
# we need to uninstall it to install the new distribution.
# * The installed version is the same as the pending distribution.
# Skip this distrubiton altogether to save work.
# * The candidate is a local wheel. Do nothing.
# * The candidate is a local sdist. Print a deprecation warning.
# * The candidate is a local path. Always reinstall.
installed_dist = self.factory.get_dist_to_uninstall(candidate)
if installed_dist is None:
ireq.should_reinstall = False
Expand All @@ -147,6 +151,34 @@ def resolve(self, root_reqs, check_supported_wheels):
ireq.should_reinstall = True
elif dist_is_editable(installed_dist) != candidate.is_editable:
ireq.should_reinstall = True
Copy link
Member

@uranusjr uranusjr Nov 20, 2020

Choose a reason for hiding this comment

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

Not related, but is this a bug? I thought editables are always reinstalled. (i.e. this should be true if dist_is_editable(installed_dist) == candidate.is_editable == True)

Copy link
Member Author

Choose a reason for hiding this comment

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

No one's complained yet. And, we'd only hit this if someone tries to install an editable-over-already-installed-editable.

I'm fine with deferring this until someone complains.

Copy link
Member

Choose a reason for hiding this comment

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

I'd hate to be the one seen as complaining but reinstalling an already installed editable is a very common workflow, to make sure entrypoints are up-to-date. The fix looks easy enough. I'll look at writing a test for this.

Copy link
Member

Choose a reason for hiding this comment

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

#9116 seems to be along the same lines as well.

elif candidate.source_link.is_file:
if candidate.source_link.is_wheel:
logger.info(
"%s is already installed with the same version as the "
"provided wheel. Use --force-reinstall to force an "
"installation of the wheel.",
ireq.name,
)
continue

looks_like_sdist = (
is_archive_file(candidate.source_link.file_path)
and candidate.source_link.ext != ".zip"
)
if looks_like_sdist:
reason = (
"Source distribution is being reinstalled despite an "
"installed package having the same name and version as "
"the installed package."
)
replacement = "use --force-reinstall"
deprecated(
reason=reason,
replacement=replacement,
gone_in="21.1",
issue=8711,
)
ireq.should_reinstall = True
else:
continue

Expand Down
10 changes: 10 additions & 0 deletions src/pip/_internal/utils/filetypes.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
"""Filetype information.
"""
from pip._internal.utils.misc import splitext
from pip._internal.utils.typing import MYPY_CHECK_RUNNING

if MYPY_CHECK_RUNNING:
Expand All @@ -14,3 +15,12 @@
ARCHIVE_EXTENSIONS = (
ZIP_EXTENSIONS + BZ2_EXTENSIONS + TAR_EXTENSIONS + XZ_EXTENSIONS
)


def is_archive_file(name):
# type: (str) -> bool
"""Return True if `name` is a considered as an archive file."""
ext = splitext(name)[1].lower()
if ext in ARCHIVE_EXTENSIONS:
return True
return False
79 changes: 52 additions & 27 deletions tests/functional/test_new_resolver.py
Original file line number Diff line number Diff line change
Expand Up @@ -1152,44 +1152,69 @@ def test_new_resolver_check_wheel_version_normalized(
assert_installed(script, simple="0.1.0+local.1")


def test_new_resolver_contraint_on_dep_with_extra(script):
create_basic_wheel_for_package(
def test_new_resolver_does_reinstall_local_sdists(script):
archive_path = create_basic_sdist_for_package(
script,
name="simple",
version="1",
depends=["dep[x]"],
"pkg",
"1.0",
)
create_basic_wheel_for_package(
script,
name="dep",
version="1",
extras={"x": ["depx==1"]},
script.pip(
"install", "--no-cache-dir", "--no-index",
archive_path,
)
create_basic_wheel_for_package(
script,
name="dep",
version="2",
extras={"x": ["depx==2"]},
assert_installed(script, pkg="1.0")

result = script.pip(
"install", "--no-cache-dir", "--no-index",
archive_path,
expect_stderr=True,
)
create_basic_wheel_for_package(
assert "Installing collected packages: pkg" in result.stdout, str(result)
assert "DEPRECATION" in result.stderr, str(result)
assert_installed(script, pkg="1.0")


def test_new_resolver_does_reinstall_local_paths(script):
pkg = create_test_package_with_setup(
script,
name="depx",
version="1",
name="pkg",
version="1.0"
)
create_basic_wheel_for_package(
script,
name="depx",
version="2",
script.pip(
"install", "--no-cache-dir", "--no-index",
pkg,
)
assert_installed(script, pkg="1.0")

result = script.pip(
"install", "--no-cache-dir", "--no-index",
pkg,
)
assert "Installing collected packages: pkg" in result.stdout, str(result)
assert_installed(script, pkg="1.0")

constraints_txt = script.scratch_path / "constraints.txt"
constraints_txt.write_text("dep==1")

def test_new_resolver_does_not_reinstall_when_from_a_local_index(script):
create_basic_sdist_for_package(
script,
"simple",
"0.1.0",
)
script.pip(
"install",
"--no-cache-dir", "--no-index",
"--find-links", script.scratch_path,
"--constraint", constraints_txt,
"simple",
"simple"
)
assert_installed(script, simple="0.1.0")

result = script.pip(
"install",
"--no-cache-dir", "--no-index",
"--find-links", script.scratch_path,
"simple"
)
assert_installed(script, simple="1", dep="1", depx="1")
# Should not reinstall!
assert "Installing collected packages: simple" not in result.stdout, str(result)
assert "Requirement already satisfied: simple" in result.stdout, str(result)
assert_installed(script, simple="0.1.0")