Partially implement PEP 517 #4589

ghost · 2017-07-01T17:39:12Z

This change accomplishes two principle objectives:

Moving the build isolation environment to be persistent throughout the build process. Because egg_info is invoked outside the wheel builder, the scope of the build environment should be (and is now here) the scope of req_install.
Moving setup.py interactions into a new class called build backend. The goal of this is to make all of the build backend functions pure except for self.setup_py_dir. We can then simply copy the build backend directly into setuptools, but it's faster to develop here first so that we can stay in a single project.

pypa-bot · 2017-07-01T17:39:15Z

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!

ghost · 2017-07-01T17:51:27Z

related: pypa/setuptools#1039

pradyunsg · 2017-07-01T18:50:47Z

@xoviat The build failed for some reason... I restarted it.

ghost · 2017-07-01T20:20:16Z

@pradyunsg What happened there?

pradyunsg · 2017-07-01T20:27:35Z

I'm not very sure, to be honest. We might have hit #4588. :/

pradyunsg · 2017-07-01T20:39:21Z

This might have happened because of some change you've made, because IIRC tox uses the local pip because of how the import system works. I think so because even AppVeyor is being held up by this change.

PS: It's 2am and I'm guessing.

pradyunsg · 2017-07-01T20:42:01Z

It would be nice if you revert the readability/:art: changes as they have bloated the diff making it difficult to see what's actually changed.

ghost · 2017-07-01T21:37:23Z

704-740 are the only changes. Everything that I work on is run through yapf.

ghost · 2017-07-01T21:38:06Z

Just look at ce0a49740a0126485b5c346746d00ce971e046d3

ghost · 2017-07-01T21:42:32Z

pip/wheel.py

+    def __build_one_inside_subprocess(self, wheel_args, cwd, env):
+        os.chdir(cwd)
+        os.environ = env
+        config_settings = {"--build-option": wheel_args}


One thing I'm not completely sure about is whether environ is checked as part of sys.path. Or does that need to be updated as well to env['PYTHONPATH']

pradyunsg · 2017-07-02T10:25:17Z

Everything that I work on is run through yapf.

The thing is YAPF is formatting in a manner that is not consistent with the rest of the codebase and currently, these changes only add noise to the diff; no functionality.

Just look at ce0a497

That won't work out when you make another commit.

To look at everything that changed over the course of this PR using the web UI, it is difficult to figure out what changed due to the noise from the style changes.

FWIW, my reasoning - the style changes don't help the PR get reviewed quicker or add any functionality. To avoid getting into a code-style related debate and making it easier to review, I'm requesting you to revert those changes.

pfmoore · 2017-07-02T10:52:50Z

+1 Please only make functional changes as part of the PR.

ghost · 2017-07-02T16:07:07Z

Done.

pradyunsg · 2017-07-02T19:10:33Z

Done.

Thanks!

pradyunsg · 2017-07-02T20:05:25Z

@xoviat I checked this out locally and reproduced the hold up on the CI. Could you look into it?

ghost · 2017-07-03T17:49:13Z

pip/compat.py

+    "get_path_uid", "stdlib_pkgs", "WINDOWS", "samefile",
+    "ThreadPoolExecutor", "ProcessPoolExecutor",
+]
+
 try:


@pradyunsg Why was this not a valid idea? Is there a circular import or something?

Not really sure...

alex · 2017-07-04T14:52:52Z

If I'm reading this code correct, it's using a ProcessPool in order to run the setuptools build_wheel code because that relies on mutating the environment in various ways:

Is there a way we could get setuptools to make this function take arguments for the directory to use and the path, that way we don't need to fork off a new process?
Even without that, I don't think concurrent.futures is required, multiprocessing.Process should be sufficient.

pradyunsg · 2017-07-04T14:56:15Z

pip/wheel.py

@@ -37,6 +37,12 @@
 from pip.utils.temp_dir import TempDirectory
 from pip.utils.ui import open_spinner

+try:
+    from concurrent.futures.process import ProcessPoolExecutor


Don't try importing the unvendored version, it could break compatibility. :)

The vendored version could fail on unreleased newer python versions. Trying to maintain python core libraries is just duplicating efforts.

I mean technically the core library could be changed, but isn't that subject to a backward compatibility guarantee?

The vendored version could fail on unreleased newer python versions.

Which would warrant a new release of pip, which is fine. You can't support something that doesn't exist when you built it. No laptop supported USB-C before it was standardised, even though OEMs knew how it would almost be.

OTOH, If the user install a release of concurrent.futures that pip has not been tested with and pip would not work - it is pip's fault and if you think about it, it'll be impossible to use pip if it has such a try-except to recover from a situation.

Trying to maintain python core libraries is just duplicating efforts.

pip simply vendors versions of these libraries that it knows it works with. They are all still separate projects in their own right and they just happen to be used by pip and because pip is special (for various reasons that I won't go into), it vendors its dependencies.

Point being - any third party library that pip imports must be a vendored within pip and only that vendored library should be used.

What if the backport only works on Python 2.7 and the core library only works on Python 3.2 and above? Then developers will need to spend additional time to update the vendored version.

OTOH, If the user install a release of concurrent.futures that pip has not been tested with and pip would not work - it is pip's fault and if you think about it, it'll be impossible to use pip if it has such a try-except to recover from a situation.

Ordinarily I would agree, but concurrent.futures is a core python library. It's included in the Python distribution. Using this logic, pip would have to account for every case where users patch core python functions such as shutil.copy.

Could you change this into a conditional on the Python version? That'll address my reservations.

That's fine with me.

pradyunsg · 2017-07-04T15:04:07Z

NEWS.rst

@@ -19,7 +19,6 @@
 - Fix an SyntaxError in an unused module of a vendored dependency. (#4059)
 - Fix UNC paths on Windows. (#4064)

-


This is unnecessary.

pradyunsg · 2017-07-04T15:04:51Z

I'm not too keen on having pip import setuptools for anything since that is exactly what we're trying to move away from in PEP518 - the difficulty of using a build toolchain other than setuptools with pip today.

ghost · 2017-07-04T18:01:37Z

Is there a way we could get setuptools to make this function take arguments for the directory to use and the path, that way we don't need to fork off a new process?

The setuptools function is PEP 517 compliant. Your suggestion would not be PEP 517 compliant.

I'm not too keen on having pip import setuptools for anything since that is exactly what we're trying to move away from in PEP518 - the difficulty of using a build toolchain other than setuptools with pip today.

This is incrementally moving pip away from depending on setuptools. In the future, line 493 could be changed to the build system specified in pyproject.toml.

ghost · 2017-07-04T18:03:34Z

@pradyunsg The other thing we need to replace after this is merged is attempting to use the setuptools.pep517.get_setup_requires function before calling setup.py egg_info

ghost · 2017-11-01T04:46:27Z

Closing as PEP 518 seems to be on hold.

astrojuanlu · 2017-11-01T08:33:37Z

What do you mean? PEP 518 was accepted in May and 517 provisionally accepted in September.

pfmoore · 2017-11-01T08:35:43Z

PEP 518 implementation discussion is ongoing. This particular implementation approach may not be the one chosen (that's part of the discussion) but PEP 518 is still very much in the plans (as is PEP 517, although we're not as far on with that one).

ghost · 2017-11-01T16:18:29Z

I currently don't have time to implement it. Obviously someone needs to and I'm willing to do what I can to help that is reasonably time-efficient. But I don't know how this PR could be merged given that it removes legacy behavior and relies on isolated environments to function. The simplest path forward is probably to wait until build isolation is enabled by default and hope that req_install hasn't changed too much by then.

pypa-bot · 2018-03-04T03:47:01Z

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!

ghost · 2018-03-04T03:56:37Z

cc @pradyunsg Upon resolving recent conflicts, the diff appears to be ~300 lines. Is that okay, or do we need to break out the changes further?

pradyunsg · 2018-03-04T16:35:09Z

Sounds fine to me.

pradyunsg · 2018-03-04T16:35:52Z

Please do open a new PR, rebased on master.

pradyunsg · 2018-03-04T20:02:07Z

Umm... Could you please open a new PR for this, instead of working on this one? Even closing this and opening a new PR from the same branch would be fine.

It's just, cleaner.

ghost · 2018-03-04T20:05:04Z

@pradyunsg If you're interested, gh-4997 needs to be addressed before this can proceed.

lock · 2019-06-02T15:05:55Z

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.

pypa-bot added needs rebase or merge PR has conflicts with current master and removed needs rebase or merge PR has conflicts with current master labels Jul 1, 2017

ghost commented Jul 1, 2017

View reviewed changes

ghost closed this Jul 2, 2017

ghost reopened this Jul 2, 2017

ghost closed this Jul 2, 2017

pradyunsg mentioned this pull request Jul 3, 2017

appveyor: skip older pr builds #4591

Closed

ghost reopened this Jul 3, 2017

ghost commented Jul 3, 2017

View reviewed changes

pradyunsg mentioned this pull request Jul 4, 2017

[WIP] AppVeyor: Timeout tests after 5 minutes #4595

Closed

pradyunsg reviewed Jul 4, 2017

View reviewed changes

ghost mentioned this pull request Oct 24, 2017

Install build requirements before calling setup.py #4799

Merged

ghost closed this Nov 1, 2017

ghost deleted the pep517 branch November 1, 2017 04:46

ghost mentioned this pull request Jan 24, 2018

Plan for 10.0 #4981

Closed

ghost restored the pep517 branch March 4, 2018 03:46

ghost reopened this Mar 4, 2018

pypa-bot added the needs rebase or merge PR has conflicts with current master label Mar 4, 2018

Merge branch 'master' into pep517

cbca754

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

ghost closed this Mar 4, 2018

ghost changed the title ~~Implement PEP 518 and partially implement PEP 517~~ Partially implement PEP 517 Mar 4, 2018

ghost reopened this Mar 4, 2018

xoviat added 3 commits March 4, 2018 13:41

prepare: revert changes

8cda666

Create metadata directory.

6ebfcd0

Fix import

a884794

ghost closed this Mar 4, 2018

gaborbernat mentioned this pull request May 12, 2018

Add support for PEP-517 tox-dev/tox#573

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

This pull request was closed.

		@@ -19,7 +19,6 @@
		- Fix an SyntaxError in an unused module of a vendored dependency. (#4059)
		- Fix UNC paths on Windows. (#4064)

Partially implement PEP 517 #4589

Partially implement PEP 517 #4589

Conversation

ghost commented Jul 1, 2017 • edited by ghost Loading

pypa-bot commented Jul 1, 2017

ghost commented Jul 1, 2017

pradyunsg commented Jul 1, 2017

ghost commented Jul 1, 2017

pradyunsg commented Jul 1, 2017

pradyunsg commented Jul 1, 2017

pradyunsg commented Jul 1, 2017

ghost commented Jul 1, 2017

ghost commented Jul 1, 2017

Choose a reason for hiding this comment

pradyunsg commented Jul 2, 2017

pfmoore commented Jul 2, 2017

ghost commented Jul 2, 2017

pradyunsg commented Jul 2, 2017

pradyunsg commented Jul 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alex commented Jul 4, 2017

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

ghost Jul 6, 2017 • edited by ghost Loading

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 commented Jul 4, 2017

ghost commented Jul 4, 2017 • edited by ghost Loading

ghost commented Jul 4, 2017

ghost commented Nov 1, 2017

astrojuanlu commented Nov 1, 2017

pfmoore commented Nov 1, 2017

ghost commented Nov 1, 2017

pypa-bot commented Mar 4, 2018

ghost commented Mar 4, 2018

pradyunsg commented Mar 4, 2018

pradyunsg commented Mar 4, 2018

pradyunsg commented Mar 4, 2018

ghost commented Mar 4, 2018

lock bot commented Jun 2, 2019

ghost commented Jul 1, 2017 •

edited by ghost

Loading

ghost Jul 6, 2017 •

edited by ghost

Loading

ghost commented Jul 4, 2017 •

edited by ghost

Loading