improve handling of PEP 518 build requirements #5286

benoit-pierre · 2018-04-18T10:26:16Z

Offload more work to the underlying pip command used to install the build requirements, so there's no need to duplicate code to handle environment markers/extras. This is done by setting the correct options from the finder passed in argument to _install_build_reqs.

pradyunsg

Thanks for this! ^>^

LGTM, barring a question regarding options passed and some test style-related stuff.

pradyunsg · 2018-04-19T15:47:20Z

tests/functional/test_install.py

+        if sys.version_info[:2] == (3, 3):
+            # Ignore Python 3.3 deprecation warning...
+            kwargs['expect_stderr'] = True
+        script.run(


Is there some reason you're not using script.pip_install_local here?

I think you can pass -f data.find_links4 to that too and it'd simplify the loop.

I'll simplify this.

pradyunsg · 2018-04-19T15:55:05Z

tests/functional/test_wheel.py

@@ -215,7 +215,7 @@ def test_pip_wheel_with_pep518_build_reqs(script, data):

 @pytest.mark.network
 def test_pip_wheel_with_pep518_build_reqs_no_isolation(script, data):
-    script.pip('install', 'wheel')
+    script.pip('install', '-f', data.find_links, 'wheel', 'simplewheel==2.0')


This can be split into 2 -- use of common_wheels to install wheel and pip_install_local for installing simplewheel.

pradyunsg · 2018-04-19T15:57:05Z

tests/lib/__init__.py

@@ -100,6 +100,10 @@ def packages2(self):
    def packages3(self):
        return self.root.join("packages3")

+    @property
+    def packages4(self):


This feels unnecessary -- why has a simple package been added under a new, different directory; am I missing something?

Some tests expect the source distribution of simple to be used. Once PEP 518 support using source distributions for the build requirements package4 can be removed.

Ah, Okay. :)

pradyunsg · 2018-04-19T15:58:22Z

src/pip/_internal/build_env.py

@@ -90,3 +113,6 @@ def __exit__(self, exc_type, exc_val, exc_tb):

    def cleanup(self):
        pass
+
+    def install_requirements(self, finder, requirements, message):
+        raise NotImplementedError


nit: NotImplementedError()

Will amend.

tests/lib/__init__.py

@@ -100,6 +100,10 @@ def packages2(self):
    def packages3(self):
        return self.root.join("packages3")

+    @property
+    def packages4(self):


pradyunsg · 2018-04-19T16:00:36Z

src/pip/_internal/build_env.py

@@ -74,12 +72,37 @@ def restore_var(varname, old_value):
    def cleanup(self):
        self._temp_dir.cleanup()

+    def install_requirements(self, finder, requirements, message):
+        args = [
+            sys.executable, '-m', 'pip', 'install', '--ignore-installed',


What's the reason for adding --ignore-installed here?

This call is not run with build isolation.

Aha.

Isolating this run would make pip not visible -> python -m pip won't work, and a temporary prefix for the installation of packages makes this non env-breaking.

pradyunsg · 2018-04-19T16:02:58Z

src/pip/_internal/req/req_install.py

@@ -127,7 +127,7 @@ def __init__(self, req, comes_from, source_dir=None, editable=False,
        self.is_direct = False

        self.isolated = isolated
-        self.build_env = BuildEnvironment(no_clean=True)
+        self.build_env = NoOpBuildEnvironment()


Nice idea. :)

pradyunsg

LGTM, barring one nit.

pradyunsg · 2018-04-20T10:37:29Z

tests/functional/test_install.py

+        kwargs['expect_stderr'] = True
+    script.run(
+        "python", "-c",
+        "import pip._internal; pip._internal.main(['wheel', '-f', %r, %r])"


You've removed a test-run for "install" command here.

Yes, as noted in the commit message the goal of this test is to check the command to install build dependencies can be run successfully, there's really no need to check with both install and wheel commands.

Fair enough. :)

pradyunsg · 2018-04-20T10:38:35Z

tests/lib/__init__.py

@@ -100,6 +100,10 @@ def packages2(self):
    def packages3(self):
        return self.root.join("packages3")

+    @property
+    def packages4(self):


Ah, Okay. :)

benoit-pierre · 2018-04-21T18:16:48Z

I further tweaked some tests to avoid hitting PyPI, and simplify some of them to use script.pip(..., use_module=True).

pradyunsg · 2018-04-21T18:26:51Z

tests/functional/test_install.py

-        )
+        raise ValueError(variant)
+    kwargs = {'use_module': True}
+    if sys.version_info[:2] == (3, 3):


This conditional is no longer needed.

Indeed! Amended.

pradyunsg · 2018-04-21T18:27:14Z

tests/functional/test_install.py

+
+def test_pep518_with_extra_and_markers(script, data, common_wheels):
+    kwargs = {}
+    if sys.version_info[:2] == (3, 3):


No longer needed.

benoit-pierre · 2018-04-23T17:07:39Z

Should the verbose flag be forwarded to the command used to install build dependencies? IMHO this would make debugging easier:

diff --git a/src/pip/_internal/build_env.py b/src/pip/_internal/build_env.py
index 37f8ab80..13bbb687 100644
--- a/src/pip/_internal/build_env.py
+++ b/src/pip/_internal/build_env.py
@@ -1,6 +1,7 @@
 """Build Environment used for isolation during sdist building
 """
 
+import logging
 import os
 import sys
 from distutils.sysconfig import get_python_lib
@@ -11,6 +12,9 @@ from pip._internal.utils.temp_dir import TempDirectory
 from pip._internal.utils.ui import open_spinner
 
 
+logger = logging.getLogger(__name__)
+
+
 class BuildEnvironment(object):
     """Creates and manages an isolated environment to install build deps
     """
@@ -77,6 +81,8 @@ class BuildEnvironment(object):
             sys.executable, '-m', 'pip', 'install', '--ignore-installed',
             '--no-user', '--prefix', self.path, '--no-warn-script-location',
         ]
+        if logger.getEffectiveLevel() <= logging.DEBUG:
+            args.append('-v')
         for format_control in ('no_binary', 'only_binary'):
             formats = getattr(finder.format_control, format_control)
             args.extend(('--' + format_control.replace('_', '-'),

pradyunsg · 2018-04-23T23:57:06Z

Yeps. Sounds like a good idea. :)

benoit-pierre · 2018-05-02T11:51:28Z

@pradyunsg: I believe I have addressed all your change requests.

benoit-pierre · 2018-05-07T11:14:10Z

Fixed stupid typo: in tests/data/src/pep518_with_extra_and_markers-1.0, renamed pep548_with_extra_and_markers.py to pep518_with_extra_and_markers.py.

benoit-pierre · 2018-05-07T12:52:30Z

Rebased on master to fix mypy job.

pradyunsg · 2018-05-07T19:14:02Z

I'm happy with this PR as is. If someone else has the time to look at this, please do. I'll merge in a few days otherwise. :)

pfmoore · 2018-05-11T09:46:59Z

I've just had a couple of minutes to look at the docs, and your doc changes look good to me.

benoit-pierre · 2018-05-11T09:52:22Z

On the subject of documentation: I think the "future development" section should be dropped or changed: it mentions PEP 426, and build hooks, but this PEP has been withdrawn and this is part is superseded by PEP 517.

pfmoore · 2018-05-11T09:56:23Z

Agreed.

pradyunsg · 2018-05-11T11:44:12Z

docs/reference/pip.rst

+  installation of build dependencies from source has been disabled until a safe
+  resolution of this issue is found.
+
+* ``pip<18.7`` does not support the use of environment markers and extras, only


It'd be 18.0

Right, amended and squashed.

pradyunsg · 2018-05-12T05:28:32Z

I'm happy with this PR as it stands. If someone has any concerns with this, please speak up now.

pfmoore · 2018-05-12T09:08:16Z

I've only done a high level review, but this looks good to me.

BrownTruck · 2018-05-18T17:30:05Z

Hello!

I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the master branch into this pull request or rebase this pull request against master then it will eligible for code review and hopefully merging!

- change module name to prevent conflicts with other distributions - add a `__version__` field for version checks

- avoid hitting the index: use `common_wheels`/`script.pip_install_local` - use `script.pip(..., use_module=True)` to simplify some tests - improve `test_pep518_uses_build_env` parametrization - simplify `test_pep518_with_user_pip`: we only need to check build requirements can be installed, so no need for testing with both `install` and `wheel` command - fix `test_pip_wheel_with_pep518_build_reqs_no_isolation`: building pep518-3.0 without isolation should fail if the build requirements are not installed.

Offload more work to the underlying pip command used to install the build requirements, so there's no need to duplicate code to handle environment markers/extras. This is done by setting the correct options from the finder passed in argument to `_install_build_reqs`.

- remove unused `BuildEnvironment` parameter: `no_clean` - move helper to install build requirement to the `BuildEnvironment` class - simplify a little the 2 different code paths (with and without build isolation)

Suppress warnings about out-of-PATH script installs.

pradyunsg · 2018-05-19T10:03:21Z

Thanks for the rebase @benoit-pierre! I'm merging this now.

lock · 2019-06-02T11:05:54Z

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.

benoit-pierre mentioned this pull request Apr 18, 2018

Fix pep518 support: support markers in build requirements #5245

Closed

benoit-pierre force-pushed the improve_pep518_build_requirements_handling branch 4 times, most recently from a55fd21 to fbbac93 Compare April 19, 2018 08:14

This was referenced Apr 19, 2018

pip-10.0.0 misinterprets version restrictions in pyproject.toml #5298

Closed

pip 10 breaks pandas on alpine linux #5299

Closed

pradyunsg reviewed Apr 19, 2018

View reviewed changes

benoit-pierre force-pushed the improve_pep518_build_requirements_handling branch 2 times, most recently from ddd6193 to d91b60d Compare April 19, 2018 21:50

pradyunsg reviewed Apr 20, 2018

View reviewed changes

pradyunsg approved these changes Apr 20, 2018

View reviewed changes

pradyunsg mentioned this pull request Apr 20, 2018

pip should respect package index option when installing build requirements #5315

Closed

benoit-pierre force-pushed the improve_pep518_build_requirements_handling branch from d91b60d to ec68159 Compare April 21, 2018 18:09

pradyunsg requested changes Apr 21, 2018

View reviewed changes

benoit-pierre force-pushed the improve_pep518_build_requirements_handling branch from ec68159 to 09d840d Compare April 21, 2018 18:36

benoit-pierre mentioned this pull request Apr 23, 2018

--find-links doesn't find setuptools package as depenency #5268

Closed

benoit-pierre mentioned this pull request Apr 26, 2018

PEP 518: enable source installs for build dependencies #5336

Merged

pradyunsg approved these changes May 3, 2018

View reviewed changes

benoit-pierre force-pushed the improve_pep518_build_requirements_handling branch from 02dcb9f to 93a98ee Compare May 7, 2018 11:11

benoit-pierre force-pushed the improve_pep518_build_requirements_handling branch from 93a98ee to b794939 Compare May 7, 2018 12:51

pradyunsg added the type: enhancement Improvements to functionality label May 7, 2018

pradyunsg requested a review from a team May 7, 2018 19:13

pradyunsg approved these changes May 11, 2018

View reviewed changes

pradyunsg reviewed May 11, 2018

View reviewed changes

benoit-pierre force-pushed the improve_pep518_build_requirements_handling branch from c3e8470 to 2f295ad Compare May 11, 2018 21:25

benoit-pierre force-pushed the improve_pep518_build_requirements_handling branch from 2f295ad to 9ee13ad Compare May 12, 2018 18:57

pradyunsg mentioned this pull request May 16, 2018

PEP-517 implementation for pip #5407

Closed

BrownTruck added the needs rebase or merge PR has conflicts with current master label May 18, 2018

benoit-pierre added 7 commits May 18, 2018 23:40

tests: tweak simplewheel test distribution

3de9bdb

- change module name to prevent conflicts with other distributions - add a `__version__` field for version checks

cleanup PEP 518 implementation

7b4790d

- remove unused `BuildEnvironment` parameter: `no_clean` - move helper to install build requirement to the `BuildEnvironment` class - simplify a little the 2 different code paths (with and without build isolation)

tweak output when installing PEP 518 build dependencies

1ac5f09

Suppress warnings about out-of-PATH script installs.

forward verbose flag to build dependencies installs

77f6ac6

minor cleanup

92e6e19

benoit-pierre force-pushed the improve_pep518_build_requirements_handling branch from 9ee13ad to 92e6e19 Compare May 18, 2018 21:47

pypa-bot removed the needs rebase or merge PR has conflicts with current master label May 18, 2018

pradyunsg merged commit 6fdcf23 into pypa:master May 19, 2018

pradyunsg added the PEP implementation Involves some PEP label May 19, 2018

rgommers mentioned this pull request May 19, 2018

Enable pip build isolation scipy/scipy#8734

Closed

2 tasks

benoit-pierre deleted the improve_pep518_build_requirements_handling branch May 30, 2018 17:18

pradyunsg mentioned this pull request Oct 24, 2018

[INTERNAL] BuildEnvironment.install_requirements cannot be called twice #5904

Closed

lock bot added the auto-locked Outdated issues that have been locked by automation label Jun 2, 2019

lock bot locked as resolved and limited conversation to collaborators Jun 2, 2019

improve handling of PEP 518 build requirements #5286

improve handling of PEP 518 build requirements #5286

Conversation

benoit-pierre commented Apr 18, 2018 • edited by pradyunsg Loading

pradyunsg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pradyunsg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benoit-pierre commented Apr 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benoit-pierre commented Apr 23, 2018

pradyunsg commented Apr 23, 2018

benoit-pierre commented May 2, 2018

benoit-pierre commented May 7, 2018

benoit-pierre commented May 7, 2018

pradyunsg commented May 7, 2018

pfmoore commented May 11, 2018

benoit-pierre commented May 11, 2018

pfmoore commented May 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pradyunsg commented May 12, 2018

pfmoore commented May 12, 2018

BrownTruck commented May 18, 2018

pradyunsg commented May 19, 2018

lock bot commented Jun 2, 2019

benoit-pierre commented Apr 18, 2018 •

edited by pradyunsg

Loading