PEP-518 support is broken

[benoit-pierre]

• Pip version: 10.0.0b2-17-gc76452ad
• Python version: Python 3.6
• Operating system: Arch Linux

Description:

Steps to reproduce (run from pip' source directory):

> python setup.py -q egg_info
> mkdir test_project
> >test_project/setup.py <<\EOF
from setuptools import setup
setup(name='project', version='0.1')
EOF
> >test_project/pyproject.toml <<\EOF
[build-system]              
requires = ["setuptools>=34.4.0", "wheel"]
EOF
> python -m venv --without-pip venv
> PYTHONPATH=$PWD/src/ ./venv/bin/python -m pip freeze --all
pip==10.0.0b2
> PYTHONPATH=$PWD/src/ ./venv/bin/python -m pip install ./test_project
Processing ./test_project
  None does not include 'setuptools' as a buildtime requirement in its pyproject.toml.
  This version of pip does not implement PEP 517 so it cannot build a wheel without setuptools.
  Installing build dependencies ... done
Installing collected packages: project
Could not import setuptools which is required to install from a source distribution.
Please install setuptools.

Note the 2 additional minor issues:

• None instead of the package name because it's not yet known
• the check for setuptools presence in the build requirements fails

Additionally, now that PEP 518 is implemented, building a wheel should be possible even if the wheel package is not installed (as long as it's in the build requirements):

> PYTHONPATH=$PWD/src/ ./venv/bin/python -m pip wheel ./test_project
ERROR: 'pip wheel' requires the 'wheel' package. To fix this, run: pip install wheel

Tentative patch:

 src/pip/_internal/commands/wheel.py     | 21 -----------------
 src/pip/_internal/operations/prepare.py | 24 ++++++++++---------
 src/pip/_internal/req/req_install.py    | 42 ++++++++++-----------------------
 3 files changed, 26 insertions(+), 61 deletions(-)

diff --git i/src/pip/_internal/commands/wheel.py w/src/pip/_internal/commands/wheel.py
index 8a85d873..a6788e37 100644
--- i/src/pip/_internal/commands/wheel.py
+++ w/src/pip/_internal/commands/wheel.py
@@ -102,28 +102,7 @@ class WheelCommand(RequirementCommand):
         self.parser.insert_option_group(0, index_opts)
         self.parser.insert_option_group(0, cmd_opts)
 
-    def check_required_packages(self):
-        import_or_raise(
-            'wheel.bdist_wheel',
-            CommandError,
-            "'pip wheel' requires the 'wheel' package. To fix this, run: "
-            "pip install wheel"
-        )
-
-        need_setuptools_message = (
-            "'pip wheel' requires setuptools >= 0.8 for dist-info support. "
-            "To fix this, run: pip install --upgrade setuptools>=0.8"
-        )
-        pkg_resources = import_or_raise(
-            'pkg_resources',
-            CommandError,
-            need_setuptools_message
-        )
-        if not hasattr(pkg_resources, 'DistInfoDistribution'):
-            raise CommandError(need_setuptools_message)
-
     def run(self, options, args):
-        self.check_required_packages()
         cmdoptions.check_install_build_global(options)
 
         index_urls = [options.index_url] + options.extra_index_urls
diff --git i/src/pip/_internal/operations/prepare.py w/src/pip/_internal/operations/prepare.py
index 2c4ff94c..c61d8873 100644
--- i/src/pip/_internal/operations/prepare.py
+++ w/src/pip/_internal/operations/prepare.py
@@ -126,25 +126,27 @@ class IsSDist(DistAbstraction):
         build_requirements, isolate = self.req.get_pep_518_info()
         should_isolate = build_isolation and isolate
 
-        if 'setuptools' not in build_requirements:
+        if not {'setuptools', 'wheel'}.issubset(
+            pkg_resources.Requirement(r).key
+            for r in build_requirements
+        ):
             logger.warning(
-                "%s does not include 'setuptools' as a buildtime requirement "
-                "in its pyproject.toml.", self.req.name,
+                "%s does not include 'setuptools' and/or 'wheel' as buildtime "
+                "requirements in its pyproject.toml.", self.req,
             )
             logger.warning(
                 "This version of pip does not implement PEP 517 so it cannot "
-                "build a wheel without setuptools."
+                "build a wheel without those."
             )
 
-        if not should_isolate:
-            self.req.build_env = NoOpBuildEnvironment(no_clean=False)
-
-        with self.req.build_env as prefix:
-            if should_isolate:
+        if should_isolate:
+            with self.req.build_env as prefix:
                 _install_build_reqs(finder, prefix, build_requirements)
+        else:
+            self.req.build_env = NoOpBuildEnvironment(no_clean=False)
 
-            self.req.run_egg_info()
-            self.req.assert_source_matches_version()
+        self.req.run_egg_info()
+        self.req.assert_source_matches_version()
 
 
 class Installed(DistAbstraction):
diff --git i/src/pip/_internal/req/req_install.py w/src/pip/_internal/req/req_install.py
index 04bb3fd6..ddd167c6 100644
--- i/src/pip/_internal/req/req_install.py
+++ w/src/pip/_internal/req/req_install.py
@@ -413,24 +413,6 @@ class InstallRequirement(object):
     @property
     def setup_py(self):
         assert self.source_dir, "No source dir for %s" % self
-        cmd = [sys.executable, '-c', 'import setuptools']
-        output = call_subprocess(
-            cmd,
-            show_stdout=False,
-            command_desc='python -c "import setuptools"',
-            on_returncode='ignore',
-        )
-
-        if output:
-            if get_installed_version('setuptools') is None:
-                add_msg = "Please install setuptools."
-            else:
-                add_msg = output
-            # Setuptools is not available
-            raise InstallationError(
-                "Could not import setuptools which is required to "
-                "install from a source distribution.\n%s" % add_msg
-            )
 
         setup_py = os.path.join(self.setup_py_dir, 'setup.py')
 
@@ -496,11 +478,12 @@ class InstallRequirement(object):
                 egg_info_dir = os.path.join(self.setup_py_dir, 'pip-egg-info')
                 ensure_dir(egg_info_dir)
                 egg_base_option = ['--egg-base', 'pip-egg-info']
-            call_subprocess(
-                egg_info_cmd + egg_base_option,
-                cwd=self.setup_py_dir,
-                show_stdout=False,
-                command_desc='python setup.py egg_info')
+            with self.build_env:
+                call_subprocess(
+                    egg_info_cmd + egg_base_option,
+                    cwd=self.setup_py_dir,
+                    show_stdout=False,
+                    command_desc='python setup.py egg_info')
 
         if not self.req:
             if isinstance(parse_version(self.pkg_info()["Version"]), Version):
@@ -788,12 +771,13 @@ class InstallRequirement(object):
             msg = 'Running setup.py install for %s' % (self.name,)
             with open_spinner(msg) as spinner:
                 with indent_log():
-                    call_subprocess(
-                        install_args + install_options,
-                        cwd=self.setup_py_dir,
-                        show_stdout=False,
-                        spinner=spinner,
-                    )
+                    with self.build_env:
+                        call_subprocess(
+                            install_args + install_options,
+                            cwd=self.setup_py_dir,
+                            show_stdout=False,
+                            spinner=spinner,
+                        )
 
             if not os.path.exists(record_filename):
                 logger.debug('Record file %s not found', record_filename)

[pradyunsg]

Feel free to go ahead and open up a PR for this.

[benoit-pierre]

Sure, but are we in agreement over the changes? Particularly: checking for both setuptools and wheel, since we need both.

I ran the testsuite locally, and 3 tests fail (as expected):

• tests/functional/test_install.py::test_without_setuptools
• tests/functional/test_install.py::test_with_setuptools_and_import_error
• tests/functional/test_wheel.py::test_basic_pip_wheel_fails_without_wheel

IHMO those don't make sense anymore and should just be removed, agreed?

[benoit-pierre]

The testsuite is also clearly missing a test to ensure all commands are run with the build environment enabled.

[pradyunsg]

checking for both setuptools and wheel, since we need both.

Yes. Minor nit about the message -- if only one is missing, the message shouldn't say "those".

should just be removed, agreed?

Yes.

The testsuite is also clearly missing a test to ensure all commands are run with the build environment enabled.

Yes.

---

Thanks for trying out the PEP 518 support -- and actually figuring out it's broken. :)

[benoit-pierre]

How about without both.?

[benoit-pierre]

Or the message could be changed to show the missing requirements.

[pradyunsg]

Showing missing requirements.

[benoit-pierre]

I was under the impression that build isolation could only disabled with --no-build-isolation, but that's not the case... There's a different code path when no pyproject.toml is present...

[pradyunsg]

When there's no pyproject.toml, there's no build isolation. This is so that projects have to opt-in to build isolation, which is a behavior change for a lot of them.

[benoit-pierre]

You're too nice...

[pradyunsg]

Uhm... Okay? That's very vague. I'm not even sure if that's a complement or sarcastic.

…

On Sun, 8 Apr 2018, 11:59 Benoit Pierre, ***@***.***> wrote: You're too nice... — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#5188 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADH7SQo2j_VfxL1akAW63WfkKEUQVjozks5tma4xgaJpZM4TLY9u> .

[benoit-pierre]

It is sarcastic.

[pradyunsg]

Sorry that you felt that way -- I'm being terse because I'm in the middle of something else right now. :/

[benoit-pierre]

The you as in pip's developers.

[pradyunsg]

Oh. Okay. Could you elaborate upon why you feel so?

[benoit-pierre]

Too many codepaths, too much code. There are already several way for users to keep the current behavior: don't update, or use --no-build-isolation.

[pradyunsg]

A package without PEP 518 support, should not have build isolation since that's a (big) change in behavior -- that'll break things in an unexpected way for people. It might happen that someone installs a PEP 518 package and a legacy package in the same pip run and one wants isolation while the other doesn't. This is an important case to have working for an easy transition -- hence we can't have a all-in-or-nothing approach here.

I do agree there's a lot of code paths and honestly, the best way to fix that would be refactors to make life easier like #5051.

[pfmoore]

I'd be in favour of reducing the number of code paths post-10.0, but before we consider that we would have to look at how we communicate such changes. Making everyone with an existing project, with a workflow that involves installing build requirements manually, add --no-build-isolation or work out how to specify their build dependencies in pyproject.toml (not always possible until we support non-binary build requirements) is certainly going to impact a lot of people, and we don't currently have any good way to warn them.

Also, refactorings like #5051 should be much easier post-10.0 when we've gone past the pain of pushing the message that our internals are private.

[pradyunsg]

Closing on account of #5190.

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.