-
-
Notifications
You must be signed in to change notification settings - Fork 258
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
Use pip for resolving and building distributions. #788
Conversation
08af512
to
c69ff6e
Compare
We used to optionally support requests and CacheControl extras and we tested some of the combinations in CI. This is all handled by / bundled with pip now, so kill.
Reviewers - thanks in advance for taking a look at this large change. Commits are reviewable independently. Of these the 2nd from top - ffd5d75 - was automatically generated via The last two commits add on feature kills proposed on #781 and since signed-off by interested parties fwict on Slack and in that issue itself. There is ongoing discussion about breaking pex up into components including a resolve component, but I think that discussion is mainly orthoganal to this change. The resolve interface is tweaked in this change, but in a way that is much more neutral; ie less pex types like Fetcher, etc and more generic resolve configuration primitives that should be easily translateable to any given resolver should we move to add plug-in resolver support. |
pex/pip.py
Outdated
# with. | ||
|
||
platform = platform_info.platform | ||
# TODO(John Sirois): XXX: File issue to kill; this can be done up in pants or in a pants |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NB: Fixed in commit caaf93d.
if resolvable not in seen: | ||
seen.add(resolvable) | ||
yield resolvable | ||
for kwargs in iter_kwargs(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NB: The old cross-product loop was very broken. When a platform is specified, the resolve is foreign and we should definitley not try to use one or more local interpreters to resolve the foreign platform wheels. One bit of brokenness: In the past for a resolve with a local python2.7 interpreter and python3.6 interpreter across (inexact) platforms of linux-x86_64 and macosx-10.13-x86_64 we'd accept any abi for the foreign platform per distribution which was broken in general and only worked when the indexes and find_links passed to resolve were carefully controlled to have uniform wheel platforms.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Epic.
I hadn't pitched in on #781, but agree that this is a big win for the project. Beyond all the fixes this brings, the change has an added benefit of lowering the barrier to entry for new contributors.
--
Because this is a disruptive change, it would help to document the end-user API changes in the PR or some other documentation. I'm envisioning something like:
Removes these flags:
* --cache-ttl, Pex no longer does its resolves
* --manylinux, this was never safe. Instead, explicitly specify the platform.
Will this PR coincide with Pex 2.0?
assert arches == ['manylinux2014_x86_64', | ||
'manylinux2010_x86_64', | ||
'manylinux1_x86_64', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pretty cool John, thanks for doing this. I'd totally forgotten about the longstanding pex caching bug(s) this will also solve, so that's exciting as well.
I took a cursory look and kicked tires and things look good at a high level to me - but I'll try to carve out some more time to dig deeper if time permits. it would be good to have someone from the Build team vet this e2e with pants as well - as I'm long removed from the authoritative chain there.
btw, this may be significant enough to finally cut a 2.0.0?
Yes.
And yes. I'll be adding a few sentences in the release notes, but also added the major changes to the PR description. |
PR description looks great. Very informative. Thank you for adding that! |
It's an "extra". It's a standard part of the requirement spec but irrelevant to |
Thanks folks, I'm going to merge this and prepare a 2.0.0 release. |
This changes Pex to vendor pip and defer all resolution and building to it.
As part of this change some ambiguous and / or broken features were changed and APIs adjusted:
--interpreter-cache-dir
CLI option was removed.--cache-ttl
CLI option andcache_ttl
resolver API argument were removed.fetchers
with a list ofindexes
and a list offind_links
repos.context
which is now automatically handled.precedence
which is now pip default precedence - wheels when available and not ruled out via the--no-wheel
CLI option oruse_wheel=False
API argument.--platform
CLI option andplatform
resolver API argument now must be full platform strings that include platform, implementation, version and abi; e.g.:--platform=macosx-10.13-x86_64-cp-36-m
.--manylinux
CLI option anduse_manylinux
resolver API argument were removed. Instead, to resolve manylinux wheels for a foreign platform, specify the manylinux platform to use with an explicit--platform
CLI flag orplatform
resolver API argument; e.g.:--platform=manylinux2010-x86_64-cp-36-m
.Fixes #781
Additionally:
Fixes #771
Fixes #763
Fixes #761
Fixes #735
Fixes #694
Fixes #660
Fixes #658
Fixes #642
Fixes #641
Fixes #628
Fixes #620
Fixes #614
Fixes #611
Fixes #608
Fixes #439
Fixes #415
Fixes #387
Fixes #315