Merge pull request #7970 from pfmoore/sdist_prepare

Avoid preparing candidates when all we need is name or version

src/pip/_internal/resolution/resolvelib/candidates.py

@@ -68,13 +68,20 @@ def make_install_req_from_dist(dist, parent):
 
 
 class LinkCandidate(Candidate):
-    def __init__(self, link, parent, factory):
-        # type: (Link, InstallRequirement, Factory) -> None
+    def __init__(
+        self,
+        link,          # type: Link
+        parent,        # type: InstallRequirement
+        factory,       # type: Factory
+        name=None,     # type: Optional[str]
+        version=None,  # type: Optional[_BaseVersion]
+    ):
+        # type: (...) -> None
         self.link = link
         self._factory = factory
         self._ireq = make_install_req_from_link(link, parent)
-        self._name = None  # type: Optional[str]
-        self._version = None  # type: Optional[_BaseVersion]
+        self._name = name
+        self._version = version
         self._dist = None  # type: Optional[Distribution]
 
     def __eq__(self, other):
@@ -113,6 +120,11 @@ class LinkCandidate(Candidate):
             self._dist = abstract_dist.get_pkg_resources_distribution()
             # TODO: Only InstalledDistribution can return None here :-(
             assert self._dist is not None
+            # TODO: Abort cleanly here, as the resolution has been
+            #       based on the wrong name/version until now, and
+            #       so is wrong.
+            # TODO: (Longer term) Rather than abort, reject this candidate
+            #       and backtrack. This would need resolvelib support.
             # These should be "proper" errors, not just asserts, as they
             # can result from user errors like a requirement "foo @ URL"
             # when the project at URL has a name of "bar" in its metadata.

src/pip/_internal/resolution/resolvelib/factory.py

@@ -22,6 +22,7 @@ if MYPY_CHECK_RUNNING:
     from typing import Dict, Optional, Set, Tuple
 
     from pip._vendor.packaging.specifiers import SpecifierSet
+    from pip._vendor.packaging.version import _BaseVersion
     from pip._vendor.pkg_resources import Distribution
 
     from pip._internal.index.package_finder import PackageFinder
@@ -67,14 +68,16 @@ class Factory(object):
 
     def _make_candidate_from_link(
         self,
-        link,    # type: Link
-        extras,  # type: Set[str]
-        parent,  # type: InstallRequirement
+        link,          # type: Link
+        extras,        # type: Set[str]
+        parent,        # type: InstallRequirement
+        name=None,     # type: Optional[str]
+        version=None,  # type: Optional[_BaseVersion]
     ):
         # type: (...) -> Candidate
         if link not in self._link_candidate_cache:
             self._link_candidate_cache[link] = LinkCandidate(
-                link, parent, factory=self,
+                link, parent, factory=self, name=name, version=version,
             )
         base = self._link_candidate_cache[link]
         if extras:
@@ -105,6 +108,8 @@ class Factory(object):
                 link=ican.link,
                 extras=extras,
                 parent=parent,
+                name=ican.name,
+                version=ican.version,
             )
         return self._make_candidate_from_dist(
             dist=dist,
@@ -115,6 +120,9 @@ class Factory(object):
     def make_requirement_from_install_req(self, ireq):
         # type: (InstallRequirement) -> Requirement
         if ireq.link:
+            # TODO: Get name and version from ireq, if possible?
+            #       Specifically, this might be needed in "name @ URL"
+            #       syntax - need to check where that syntax is handled.
             cand = self._make_candidate_from_link(
                 ireq.link, extras=set(), parent=ireq,
             )

tests/functional/test_new_resolver.py

@@ -2,7 +2,10 @@ import json
 
 import pytest
 
-from tests.lib import create_basic_wheel_for_package
+from tests.lib import (
+    create_basic_sdist_for_package,
+    create_basic_wheel_for_package,
+)
 
 
 def assert_installed(script, **kwargs):
@@ -253,3 +256,41 @@ def test_new_resolver_ignore_installed(script):
     assert script.site_packages / "base" in result.files_updated, (
         "base 0.1.0 not reinstalled"
     )
+
+
+def test_new_resolver_only_builds_sdists_when_needed(script):
+    create_basic_wheel_for_package(
+        script,
+        "base",
+        "0.1.0",
+        depends=["dep"],
+    )
+    create_basic_sdist_for_package(
+        script,
+        "dep",
+        "0.1.0",
+        # Replace setup.py with something that fails
+        extra_files={"setup.py": "assert False"},
+    )
+    create_basic_sdist_for_package(
+        script,
+        "dep",
+        "0.2.0",
+    )
+    # We only ever need to check dep 0.2.0 as it's the latest version
+    script.pip(
+        "install", "--unstable-feature=resolver",
+        "--no-cache-dir", "--no-index",
+        "--find-links", script.scratch_path,
+        "base"
+    )
+    assert_installed(script, base="0.1.0", dep="0.2.0")
+
+    # We merge criteria here, as we have two "dep" requirements
+    script.pip(
+        "install", "--unstable-feature=resolver",
+        "--no-cache-dir", "--no-index",
+        "--find-links", script.scratch_path,
+        "base", "dep"
+    )
+    assert_installed(script, base="0.1.0", dep="0.2.0")

tests/lib/__init__.py

@@ -15,7 +15,7 @@ from textwrap import dedent
 from zipfile import ZipFile
 
 import pytest
-from pip._vendor.six import PY2
+from pip._vendor.six import PY2, ensure_binary, text_type
 from scripttest import FoundDir, TestFileEnvironment
 
 from pip._internal.index.collector import LinkCollector
@@ -1036,6 +1036,56 @@ def create_basic_wheel_for_package(
     return archive_path
 
 
+def create_basic_sdist_for_package(
+    script, name, version, extra_files=None
+):
+    files = {
+        "setup.py": """
+            from setuptools import find_packages, setup
+            setup(name={name!r}, version={version!r})
+        """,
+    }
+
+    # Some useful shorthands
+    archive_name = "{name}-{version}.tar.gz".format(
+        name=name, version=version
+    )
+
+    # Replace key-values with formatted values
+    for key, value in list(files.items()):
+        del files[key]
+        key = key.format(name=name)
+        files[key] = textwrap.dedent(value).format(
+            name=name, version=version
+        ).strip()
+
+    # Add new files after formatting
+    if extra_files:
+        files.update(extra_files)
+
+    for fname in files:
+        path = script.temp_path / fname
+        path.parent.mkdir(exist_ok=True, parents=True)
+        path.write_bytes(ensure_binary(files[fname]))
+
+    # The base_dir cast is required to make `shutil.make_archive()` use
+    # Unicode paths on Python 2, making it able to properly archive
+    # files with non-ASCII names.
+    retval = script.scratch_path / archive_name
+    generated = shutil.make_archive(
+        retval,
+        'gztar',
+        root_dir=script.temp_path,
+        base_dir=text_type(os.curdir),
+    )
+    shutil.move(generated, retval)
+
+    shutil.rmtree(script.temp_path)
+    script.temp_path.mkdir()
+
+    return retval
+
+
 def need_executable(name, check_cmd):
     def wrapper(fn):
         try: