Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PEP-517 implementation for pip #5407

Closed
gaborbernat opened this issue May 14, 2018 · 52 comments
Closed

PEP-517 implementation for pip #5407

gaborbernat opened this issue May 14, 2018 · 52 comments
Assignees
Labels
auto-locked Outdated issues that have been locked by automation PEP implementation Involves some PEP state: needs discussion This needs some more discussion type: enhancement Improvements to functionality

Comments

@gaborbernat
Copy link

gaborbernat commented May 14, 2018

This is an issue ticket to track discussion and planning on adding PEP-517 support. Pip 10 does support PEP-518 (with the limitation that the build tool must be a wheel - this avoids the issue of circular dependencies). The next step now is to support PEP-517. All I describe here is the result of my discussion with @dstufft at PyCon 2018 sprints.

pip for PEP-518 does (for a given package to build):

  • if an sdist is acquired extract it to a tree source,
  • get the build tool and build requirements from pyproject.toml (must contain setuptools and wheel),
  • for the current build environment (which is a temporary directory) install the build tool and requirements (this happens by invoking pip via a subprocess),
  • invoke the build command by using this temporary folder to build a wheel,
  • install then the wheel.

Phase 1: pip install in a PEP-517 and 518 would do:

  • if an sdist is acquired extract it to a tree source,
  • get the build tool pyproject.toml,
  • for the current build environment (which is a temporary directory) install the build tool (this happens by invoking pip via a subprocess),
  • use get_requires_for_build_sdist (or it's wheel counterpart) to get the build requirements (this happens via invoke python within a subprocess of the build environment),
  • for the current build environment (which is a temporary directory) install the build requirements (this happens by invoking pip via a subprocess),
  • invoke the build command by using this temporary environment to build a wheel (build_wheel).
  • install then the wheel.

Phase 2: allow pip to build packages for distribution - pip build

  • this follows the same paths as above with the sole difference that allows for the user to select either wheel or sdist, and that it's invoked from the cli (e.g. pip build . --sdist.

For phase 1 most of the things are already implemented in https://github.com/pypa/pep517 by @takluyver. It even follows the method pip uses to implement PEP-518. I suggest we just take that package as a vendored package within pip, and maybe make slight alternation to it where required. Once that's done we can go onto phase 2.

Please show your take on this if you have any obligations, otherwise I'll try to create a PR for this.

@pradyunsg pradyunsg added the S: needs triage Issues/PRs that need to be triaged label May 16, 2018
@gaborbernat
Copy link
Author

@pradyunsg so how's triaging works?

@pradyunsg
Copy link
Member

@gaborbernat Hey! So, there's of a lot of triage to do -- using that label to signal issues/PRs that need a maintainer to respond to. :)


Sure! Let me think out loud how things should work... I feel like I have so much here.

Phase 1

  • sdist -> wheel -> installed when given an sdist is obviously what pip should do.
    • If we have an sdist, we should unpack it to a temporary directory and proceed.
  • local-dir -> ? -> installed is more interesting. :)

Now, I do think having a discussion early about how things should be structured/implemented would be nice, before a PR. Mostly since I've spent a bit of my time thinking about #5051 (although that's a blue whale issue). Basically, I would prefer a clean separation between the current flow and a PEP 517/518 flow.

  • What about non-PEP 517/518 source trees/sdists?
    • Their process/building shouldn't be affected. That is important for backward compatibility.
    • This can be tricky to verify -- Refactor InstallRequirement #5051 would be useful here but hey, that's a blue whale.
  • Should pip try to do local-dir -> sdist -> wheel unless build_sdist screeches in pain?
    • Yes.
      • I have a preference for the 2 step approach so as to minimize the number of surprises for people. Additionally, both steps should have separate environments for building (or the same one "cleared" and reused).
      • It ensures that what's in the sdist, is enough to build a wheel -- integrity of sdists is "verified" in a way. (could have used better words?)
    • But we get dependency metadata only when building wheels. :(
      • I'm gonna shelve this thought and make a lot of noise when the time comes for this. :P

Phase 2

I'm noting these here now but think these should be discussed after Phase 1 is done.

  • Does pip build mean we deprecate pip wheel?
    • Yes. I like the spelling of pip build. :)
  • How does pip build work?
    • local-dir -> sdist -> wheel falling back to local-dir -> wheel
      • Yes.
        • The regular flow (as per above), which should be the default
      • pip build .
    • sdist -> wheel
      • Yes.
      • pip build foo-1.0.tar.gz
    • local-dir -> sdist
      • Yes.
      • pip build --sdist-only .
    • local-dir -> wheel
      • Yes.
        • Useful for build backends to test themselves (to actually verify that going directly to wheel gives the same wheel as going via sdist).
      • pip build --wheel-only .

Then, PEP 518 support needs to be improved/expanded and we have #5286 + #5336, which if I understand correctly, would get us there. (thanks to @benoit-pierre)

All I describe here is the result of my discussion with @dstufft at PyCon 2018 sprints.

You got to meet him! :O

maybe make slight alternation to it where required.

I do prefer that the slight alterations happen upstream (not modifying vendored packages unless absolutely needed). :)


self-note: We will need to be building wheels (or just wheel metadata) during resolution. zazo won't care about this -- it's designed to defer this to when integrating into pip; so let's look into this then. :)

local-dir -> sdist/wheel:

  • create a build environment (buildenv)
    • do not copy the local-dir like pip does today
  • install build-system.requires in buildenv
  • "load" build-system.build-backend
  • install build_backend.get_requires_for_build_sdist/wheel in buildenv (optional)
    • subprocess
  • build_backend.build_sdist/wheel
    • subprocess
  • Proceed to with the generated sdist/wheel

@pradyunsg pradyunsg added type: enhancement Improvements to functionality PEP implementation Involves some PEP state: needs discussion This needs some more discussion and removed S: needs triage Issues/PRs that need to be triaged labels May 16, 2018
@gaborbernat
Copy link
Author

@pradysung do we have an official definition of what an sdist is? I am personally not convinced that we do need the sdist step always. Seems something that could potentially slow us down and redundant. Any reason you want to Keep sdist around in all builds?

@pradyunsg
Copy link
Member

@gaborbernat sdist is short for source distribution. The reason to keep it is as an integrity check of the installability of a source distribution. It should be possible to go from a source distribution to an installed package. Ensuring that by default makes sense to me, when installing from local directories.

@gaborbernat
Copy link
Author

@pradyunsg integrity check? That just sounds to me like we're trying to validate that the build backend does its job correctly, or that the user does the packaging correctly. I would argue that this would violate separation of concern. Shouldn't pip take care of the package management itself, and leave the build to the responsibility of the build backend, as outlined in PEP-517?

@pradyunsg
Copy link
Member

sdist is short for source distribution.

This was wrong from my end to think you didn't know that. Apologies. As for the format of sdists, it's mostly a defacto based on what setuptools generates right now. Moving forward, PEP 517 has a specification: https://www.python.org/dev/peps/pep-0517/#source-distributions

integrity check? That just sounds to me like we're trying to validate that the build backend does its job correctly, or that the user does the packaging correctly.

Mostly the latter. That said, I'm not sure how I feel strongly about this. I'm curious what other think about this.

@pfmoore
Copy link
Member

pfmoore commented Jun 11, 2018

From what I recall, this (whether builds should be done "via sdist" or not) was extensively debated in the PEP 517 discussions. I don't recall the answer, but I think it would be worth researching the discussion threads before starting any implementation. I'm not going to try to summarise what I recollect, as I was one of the participants in the thread, so I'm not unbiased, and I honestly don't remember what the ultimate consensus was.

Yes, I know the PEP should probably specify things like this, but there was way too much debate and controversy to realistically expect that in all cases.

One thing I will say is that we do not want to copy the source tree and build from the copy. That's the source of all the "pip takes too long to build from source" issues we have, because we can't know whether we need to copy (e.g.) .git directories, or huge data files. We should either build in place, or build from sdist, or there's an alternative efficient approach in the PEP that I don't recall right now :-)

@RonnyPfannschmidt
Copy link
Contributor

@pfmoore pip could introduce a tool.pip section where projects can specify whether its safe to copy them among other things

@pfmoore
Copy link
Member

pfmoore commented Jun 11, 2018

But the copy is used when building from a user's working directory - the project cannot know what additional stuff the user might have stored there.

I don't think there's anything new going on here (at least it sounds to my recollection like the sort of thing we discussed during the development of PEP 517). I'm not going to rehash that debate, so I'll just say anyone interested should read the mailing list archives for context. (Apologies if you were involved in those discussions at the time @RonnyPfannschmidt - once again I simply don't recall).

@RonnyPfannschmidt
Copy link
Contributor

pip copying breaks stuff for me on regular basis tho ^^

@takluyver
Copy link
Member

From what I recall, this (whether builds should be done "via sdist" or not) was extensively debated in the PEP 517 discussions. I don't recall the answer, but I think it would be worth researching the discussion threads before starting any implementation.

I am certainly not unbiased, but the conclusion as I remember it was that:

  • It's recommended to build via an sdist if possible
  • But frontend tools should be prepared to fall back to building a wheel directly if building an sdist fails - e.g. because flit depends on a VCS to build an sdist, and that may not be available in an install context.

@pfmoore
Copy link
Member

pfmoore commented Jun 11, 2018

@takluyver As one of the PEP authors, your bias is authoritative, IMO :-) But either way, my recollection matches yours.

Actually, looking at the PEP:

To ensure that wheels from different sources are built the same way, frontends may call build_sdist first, and then call build_wheel in the unpacked sdist. But if the backend indicates that it is missing some requirements for creating an sdist (see below), the frontend will fall back to calling build_wheel in the source directory.

And certainly my view remains that pip should choose to call build_sdist first. So recommendation or not, that's what I'd want pip to do. It's not so much about an "integrity check", as it is about ensuring that pip install . produces the same result as building a sdist manually, then pip install <the-sdist>. So the developer doesn't get different results than his end users (who will be getting the sdist, not the developer's source tree).

There was a lot of debate about whether the PEP could (or even should) require that building a wheel directly had to "do the same as" building a sdist and then building a wheel from that sdist. It basically didn't reach any particular conclusion, not least because the question is rather ill-defined. IMO, the point of pip always building a sdist first if it can is precisely to avoid needing to address that question.

@pradyunsg
Copy link
Member

It's not so much about an "integrity check", as it is about ensuring that pip install . produces the same result as building a sdist manually, then pip install .

This is what I meant by "integrity check"; looking back at that comment, I guess I could have worded it better.


ISTM that both @takluyver and @pfmoore have the same position as me on this:

  • try local-dir -> sdist -> wheel unless sdist creation is not possible.

@pradyunsg
Copy link
Member

@gaborbernat This issue has been idle for over a month; do you have any outstanding concerns here or the bandwidth to take this on in the near future?

@pfmoore
Copy link
Member

pfmoore commented Jul 21, 2018

I'd like to pick this up (I'd said back at the release of pip 10 that I was hoping to work on this, but hadn't got around to it so far).

My initial plan is as follows:

  1. Implement a separate "pep517" package that handles the infrastructure of calling PEP 517 hooks.
  2. Vendor this in pip and use it to implement the "build frontend" side of things.

I wanted to put an initial comment here as most of the first stage will be happening independently of pip, and as a result won't be immediately visible here. Commenting here at least means people are aware of this, and don't duplicate work.

@gaborbernat
Copy link
Author

@pfmoore I feel like step 1 us already completed via https://github.com/pypa/pep517 can't you use that?

@takluyver
Copy link
Member

That's what that package was intended for, so if it's not the right shape to be used from pip, feel free to modify it.

@pfmoore
Copy link
Member

pfmoore commented Jul 21, 2018

Ah, I didn't even know that existed - thanks!

@dstufft
Copy link
Member

dstufft commented Jul 25, 2018 via email

@pfmoore
Copy link
Member

pfmoore commented Jul 25, 2018

My inclination was, if there is no backend explicitly specified in pyproject.toml, then we do a check for our "assumed requirements" (wheel plus any setuptools for legacy, wheel plus a sufficiently new setuptools for setuptools-backend) before we start calling hooks. We do that for both isolated environments that we built, and the main environment in the no-isolation case, and we error out if the assumed requirements aren't present. (So yes, a too-old version of setuptools is an error if we're using the setuptools backend).

That would replace the current "we haven't implemented PEP 517 yet" check for setuptools and wheel in the isolation case, as well as tightening up the non-isolation case.

@pfmoore
Copy link
Member

pfmoore commented Jul 27, 2018

One note: when we make --use-pep517 the default everywhere, everyone that uses --no-index will need to ensure that they make setuptools and wheel packages available to pip if they want to do any builds from source. This is basically #5402, but it's potentially going to hit a lot more people (it hits our test suite in a fair number of places, which is how I spotted it 😄).

It's a direct consequence of how isolated environments work, so I'm not suggesting we don't do this, but when we do, we'll need to think carefully about how we communicate the change, and ensure that we warn people in advance, before we make that switch.

It's not a problem for the initial implementation, because at that point we default to --no-use-pep517 for setup.py based builds.

@pfmoore
Copy link
Member

pfmoore commented Jul 27, 2018

I should say, if anyone wants to follow along, my work in progress is at https://github.com/pfmoore/pip/tree/pep517_implementation. I've not made a PR out of it yet because I'm doing a bit of history rewriting to keep things clean. But I'll start splitting out PRs from it soon.

Current status: I've refactored req_install.py to add the "use PEP 517" logic discussed above (nothing actually uses PEP 517 yet, that flag currently just means basically "use isolation"). Next steps are:

  • Expose the --[no-]use-pep517 option. The internal code is there, it's just the option itself to do.
  • Vendor the pep517 library (a bit fiddly as it doesn't have a released version yet).
  • Actually build wheels using the backend.
  • Handle dynamic dependencies declared by the backend.
  • Tests (hardest part here will be writing a set of dummy backends to exercise all the edge cases).
  • Documentation.

Plus plenty of bug fixing and the like.

@dstufft
Copy link
Member

dstufft commented Jul 27, 2018

The PEP 517 flag should be independent of the build isolation flag right? Some day we're going to remove the --pep517 flag, but we're going to keep --no-build-isolation. If we're not isolating the build, then you're just expected to have already installed the build dependencies and we won't call it inside the isolated environment?

@pfmoore
Copy link
Member

pfmoore commented Jul 27, 2018

Sorry, I wasn't clear. Yes, build isolation and PEP 517 are separate. What I've done is refactor the internals of InstallRequirement to read pyproject.toml and decide what to do. There's a new attribute, use_pep517, which encapsulates what we decided (and the --use-pep517 flag will affect this). The build isolation processing remains unchanged, it's just that in the absence of --no-build-isolation, rather than deciding whether to isolate based on "does pyproject.toml exist?", we're doing it based on this new flag (which returns the same answer at the moment, but which will ultimately be affected by the new --use-pep517 flag which will allow users to opt in to isolation in cases where they can't at the moment).

Also, all the tests currently pass, so I've not broken any existing functionality 😉

tl; dr; Yes, I'm keeping the two independent 😄

@dstufft
Copy link
Member

dstufft commented Jul 27, 2018

Ok great!

@pfmoore
Copy link
Member

pfmoore commented Aug 13, 2018

Quick update. I've briefly stalled on the implementation, because of various family and other commitments. I should be getting back to it in a week or two (maybe sooner depending on how things go).

@pfmoore
Copy link
Member

pfmoore commented Nov 23, 2018

Can this now be closed? #5743 is now merged.

@gaborbernat
Copy link
Author

Sure 😁when's next pip release?😁

@pfmoore
Copy link
Member

pfmoore commented Nov 23, 2018

January We don't have anyone who's volunteered to be RM for it yet, though :-)

@pradyunsg
Copy link
Member

Whee!!! If no one beats me to it, I'll go ahead and open an issue for adding a pip build command tomorrow. :)

@pfmoore
Copy link
Member

pfmoore commented Nov 23, 2018

@pradyunsg What's a pip build?

@gaborbernat
Copy link
Author

@pfmoore note phase two of this issue first post. Basically now pip should be able to build both wheel/sdist via PEP-517. So a generalization of pip wheel?

@pfmoore
Copy link
Member

pfmoore commented Nov 23, 2018

Oh yeah, thanks, I'd forgotten about that :-) (Or more accurately, I'd forgotten it was added into this issue as part of PEP 517 - I'd always considered it as a separate thing).

By the way, it's worth noting with regard to this comment from @pradyunsg, in particular

Should pip try to do local-dir -> sdist -> wheel unless build_sdist screeches in pain?
Yes.

that the current implementation of PEP 517 does not do this. I consider the change to always build via sdist to be an independent piece of work - we could have done it ages ago long before PEP 517/518, and it always got caught up in debates about incremental builds, and tools like flit that (at the time) didn't support sdists, etc. I still think this is worth doing, and I don't think the arguments against it are particularly compelling, but I didn't want to block implementing PEP 517 in order to debate that.

So if someone wants to finally switch to build-via-sdist, then I'm all for it, but it would also need a separate issue.

@lock
Copy link

lock bot commented May 31, 2019

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.

@lock lock bot added the auto-locked Outdated issues that have been locked by automation label May 31, 2019
@lock lock bot locked as resolved and limited conversation to collaborators May 31, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
auto-locked Outdated issues that have been locked by automation PEP implementation Involves some PEP state: needs discussion This needs some more discussion type: enhancement Improvements to functionality
Projects
None yet
Development

No branches or pull requests

8 participants