Merge pull request #6370 from cjerdonek/editable-and-pep-517-optional

Fix editable-mode logic when pyproject.toml present

news/6370.bugfix

@@ -0,0 +1,3 @@
+Fix the handling of editable mode during installs when ``pyproject.toml`` is
+present but PEP 517 doesn't require the source tree to be treated as
+``pyproject.toml``-style.

src/pip/_internal/operations/prepare.py

@@ -99,21 +99,48 @@ class IsSDist(DistAbstraction):
     def dist(self):
         return self.req.get_dist()
 
+    def _raise_conflicts(self, conflicting_with, conflicting_reqs):
+        conflict_messages = [
+            '%s is incompatible with %s' % (installed, wanted)
+            for installed, wanted in sorted(conflicting_reqs)
+        ]
+        raise InstallationError(
+            "Some build dependencies for %s conflict with %s: %s." % (
+                self.req, conflicting_with, ', '.join(conflict_messages))
+        )
+
+    def install_backend_dependencies(self, finder):
+        # type: (PackageFinder) -> None
+        """
+        Install any extra build dependencies that the backend requests.
+
+        :param finder: a PackageFinder object.
+        """
+        req = self.req
+        with req.build_env:
+            # We need to have the env active when calling the hook.
+            req.spin_message = "Getting requirements to build wheel"
+            reqs = req.pep517_backend.get_requires_for_build_wheel()
+        conflicting, missing = req.build_env.check_requirements(reqs)
+        if conflicting:
+            self._raise_conflicts("the backend dependencies", conflicting)
+        req.build_env.install_requirements(
+            finder, missing, 'normal',
+            "Installing backend dependencies"
+        )
+
     def prep_for_dist(self, finder, build_isolation):
         # type: (PackageFinder, bool) -> None
         # Prepare for building. We need to:
         #   1. Load pyproject.toml (if it exists)
         #   2. Set up the build environment
 
         self.req.load_pyproject_toml()
-        should_isolate = self.req.use_pep517 and build_isolation
 
-        def _raise_conflicts(conflicting_with, conflicting_reqs):
-            raise InstallationError(
-                "Some build dependencies for %s conflict with %s: %s." % (
-                    self.req, conflicting_with, ', '.join(
-                        '%s is incompatible with %s' % (installed, wanted)
-                        for installed, wanted in sorted(conflicting))))
+        should_isolate = (
+            (self.req.use_pep517 or self.req.pyproject_requires) and
+            build_isolation
+        )
 
         if should_isolate:
             # Isolate in a BuildEnvironment and install the build-time
@@ -127,8 +154,8 @@ def _raise_conflicts(conflicting_with, conflicting_reqs):
                 self.req.requirements_to_check
             )
             if conflicting:
-                _raise_conflicts("PEP 517/518 supported requirements",
-                                 conflicting)
+                self._raise_conflicts("PEP 517/518 supported requirements",
+                                      conflicting)
             if missing:
                 logger.warning(
                     "Missing build requirements in pyproject.toml for %s.",
@@ -139,20 +166,13 @@ def _raise_conflicts(conflicting_with, conflicting_reqs):
                     "pip cannot fall back to setuptools without %s.",
                     " and ".join(map(repr, sorted(missing)))
                 )
-            # Install any extra build dependencies that the backend requests.
-            # This must be done in a second pass, as the pyproject.toml
-            # dependencies must be installed before we can call the backend.
-            with self.req.build_env:
-                # We need to have the env active when calling the hook.
-                self.req.spin_message = "Getting requirements to build wheel"
-                reqs = self.req.pep517_backend.get_requires_for_build_wheel()
-            conflicting, missing = self.req.build_env.check_requirements(reqs)
-            if conflicting:
-                _raise_conflicts("the backend dependencies", conflicting)
-            self.req.build_env.install_requirements(
-                finder, missing, 'normal',
-                "Installing backend dependencies"
-            )
+
+            if self.req.use_pep517:
+                # If we're using PEP 517, then install any extra build
+                # dependencies that the backend requested.  This must be
+                # done in a second pass, as the pyproject.toml dependencies
+                # must be installed before we can call the backend.
+                self.install_backend_dependencies(finder=finder)
 
         self.req.prepare_metadata()
         self.req.assert_source_matches_version()

src/pip/_internal/pyproject.py

@@ -12,6 +12,8 @@
 if MYPY_CHECK_RUNNING:
     from typing import Any, Dict, List, Optional, Tuple
 
+    Pep517Data = Tuple[str, List[str]]
+
 
 def _is_list_of_str(obj):
     # type: (Any) -> bool
@@ -64,6 +66,37 @@ def make_editable_error(req_name, reason):
     return InstallationError(message)
 
 
+def get_build_system_requires(build_system, req_name):
+    if build_system is None:
+        return None
+
+    # Ensure that the build-system section in pyproject.toml conforms
+    # to PEP 518.
+    error_template = (
+        "{package} has a pyproject.toml file that does not comply "
+        "with PEP 518: {reason}"
+    )
+
+    # Specifying the build-system table but not the requires key is invalid
+    if "requires" not in build_system:
+        raise InstallationError(
+            error_template.format(package=req_name, reason=(
+                "it has a 'build-system' table but not "
+                "'build-system.requires' which is mandatory in the table"
+            ))
+        )
+
+    # Error out if requires is not a list of strings
+    requires = build_system["requires"]
+    if not _is_list_of_str(requires):
+        raise InstallationError(error_template.format(
+            package=req_name,
+            reason="'build-system.requires' is not a list of strings.",
+        ))
+
+    return requires
+
+
 def resolve_pyproject_toml(
     build_system,  # type: Optional[Dict[str, Any]]
     has_pyproject,  # type: bool
@@ -72,7 +105,7 @@ def resolve_pyproject_toml(
     editable,  # type: bool
     req_name,  # type: str
 ):
-    # type: (...) -> Optional[Tuple[List[str], str, List[str]]]
+    # type: (...) -> Tuple[Optional[List[str]], Optional[Pep517Data]]
     """
     Return how a pyproject.toml file's contents should be interpreted.
 
@@ -86,6 +119,13 @@ def resolve_pyproject_toml(
     :param editable: whether editable mode was requested for the requirement.
     :param req_name: the name of the requirement we're processing (for
         error reporting).
+
+    :return: a tuple (requires, pep517_data), where `requires` is the list
+      of build requirements from pyproject.toml (or else None).  The value
+      `pep517_data` is None if `use_pep517` is False.  Otherwise, it is the
+      tuple (backend, check), where `backend` is the name of the PEP 517
+      backend and `check` is the list of requirements we should check are
+      installed after setting up the build environment.
     """
     # The following cases must use PEP 517
     # We check for use_pep517 being non-None and falsey because that means
@@ -126,19 +166,34 @@ def resolve_pyproject_toml(
                 req_name, 'PEP 517 processing was explicitly requested'
             )
 
-    # If we haven't worked out whether to use PEP 517 yet,
-    # and the user hasn't explicitly stated a preference,
-    # we do so if the project has a pyproject.toml file.
+    # If we haven't worked out whether to use PEP 517 yet, and the user
+    # hasn't explicitly stated a preference, we do so if the project has
+    # a pyproject.toml file (provided editable mode wasn't requested).
     elif use_pep517 is None:
+        if has_pyproject and editable:
+            message = (
+                'Error installing {!r}: editable mode is not supported for '
+                'pyproject.toml-style projects. pip is processing this '
+                'project as pyproject.toml-style because it has a '
+                'pyproject.toml file. Since the project has a setup.py and '
+                'the pyproject.toml has no "build-backend" key for the '
+                '"build_system" value, you may pass --no-use-pep517 to opt '
+                'out of pyproject.toml-style processing. '
+                'See PEP 517 for details on pyproject.toml-style projects.'
+            ).format(req_name)
+            raise InstallationError(message)
+
         use_pep517 = has_pyproject
 
     # At this point, we know whether we're going to use PEP 517.
     assert use_pep517 is not None
 
+    requires = get_build_system_requires(build_system, req_name=req_name)
+
     # If we're using the legacy code path, there is nothing further
     # for us to do here.
     if not use_pep517:
-        return None
+        return (requires, None)
 
     if build_system is None:
         # Either the user has a pyproject.toml with no build-system
@@ -149,8 +204,8 @@ def resolve_pyproject_toml(
         # traditional direct setup.py execution, and require wheel and
         # a version of setuptools that supports that backend.
 
+        requires = ["setuptools>=40.8.0", "wheel"]
         build_system = {
-            "requires": ["setuptools>=40.8.0", "wheel"],
             "build-backend": "setuptools.build_meta:__legacy__",
         }
 
@@ -160,30 +215,6 @@ def resolve_pyproject_toml(
     # specified a backend, though.
     assert build_system is not None
 
-    # Ensure that the build-system section in pyproject.toml conforms
-    # to PEP 518.
-    error_template = (
-        "{package} has a pyproject.toml file that does not comply "
-        "with PEP 518: {reason}"
-    )
-
-    # Specifying the build-system table but not the requires key is invalid
-    if "requires" not in build_system:
-        raise InstallationError(
-            error_template.format(package=req_name, reason=(
-                "it has a 'build-system' table but not "
-                "'build-system.requires' which is mandatory in the table"
-            ))
-        )
-
-    # Error out if requires is not a list of strings
-    requires = build_system["requires"]
-    if not _is_list_of_str(requires):
-        raise InstallationError(error_template.format(
-            package=req_name,
-            reason="'build-system.requires' is not a list of strings.",
-        ))
-
     backend = build_system.get("build-backend")
     check = []  # type: List[str]
     if backend is None:
@@ -202,7 +233,7 @@ def resolve_pyproject_toml(
         backend = "setuptools.build_meta:__legacy__"
         check = ["setuptools>=40.8.0", "wheel"]
 
-    return (requires, backend, check)
+    return (requires, (backend, check))
 
 
 def load_pyproject_toml(
@@ -212,7 +243,7 @@ def load_pyproject_toml(
     setup_py,  # type: str
     req_name  # type: str
 ):
-    # type: (...) -> Optional[Tuple[List[str], str, List[str]]]
+    # type: (...) -> Tuple[Optional[List[str]], Optional[Pep517Data]]
     """Load the pyproject.toml file.
 
     Parameters:
@@ -224,13 +255,13 @@ def load_pyproject_toml(
         req_name - The name of the requirement we're processing (for
                    error reporting)
 
-    Returns:
-        None if we should use the legacy code path, otherwise a tuple
+    Returns: (requires, pep_517_data)
+      requires: requirements from pyproject.toml (can be None).
+      pep_517_data: None if we should use the legacy code path, otherwise:
         (
-            requirements from pyproject.toml,
             name of PEP 517 backend,
-            requirements we should check are installed after setting
-                up the build environment
+            requirements we should check are installed after setting up
+            the build environment
         )
     """
     has_pyproject = os.path.isfile(pyproject_toml)

src/pip/_internal/req/req_install.py

@@ -485,21 +485,22 @@ def load_pyproject_toml(self):
         use_pep517 attribute can be used to determine whether we should
         follow the PEP 517 or legacy (setup.py) code path.
         """
-        pep517_data = load_pyproject_toml(
+        requires, pep517_data = load_pyproject_toml(
             self.use_pep517,
             self.editable,
             self.pyproject_toml,
             self.setup_py,
             str(self)
         )
 
-        if pep517_data is None:
-            self.use_pep517 = False
-        else:
-            self.use_pep517 = True
-            requires, backend, check = pep517_data
+        use_pep517 = bool(pep517_data)
+
+        self.use_pep517 = use_pep517
+        self.pyproject_requires = requires
+
+        if use_pep517:
+            backend, check = pep517_data
             self.requirements_to_check = check
-            self.pyproject_requires = requires
             self.pep517_backend = Pep517HookCaller(self.setup_py_dir, backend)
 
             # Use a custom function to call subprocesses

tests/functional/test_install_reqs.py

@@ -302,8 +302,12 @@ def test_constraints_local_editable_install_pep518(script, data):
     to_install = data.src.join("pep518-3.0")
 
     script.pip('download', 'setuptools', 'wheel', '-d', data.packages)
+    # --no-use-pep517 has to be passed since a pyproject.toml file is
+    # present but PEP 517 doesn't support editable mode.
     script.pip(
-        'install', '--no-index', '-f', data.find_links, '-e', to_install)
+        'install', '--no-use-pep517', '--no-index', '-f', data.find_links,
+        '-e', to_install,
+    )
 
 
 def test_constraints_local_install_causes_error(script, data):