-
-
Notifications
You must be signed in to change notification settings - Fork 610
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
Vendor pip 9.0.3 #644
Vendor pip 9.0.3 #644
Conversation
I didn't change the test matrix to stop testing on multiple pip version right away. I want to make sure that it still works with earlier pip versions (as I took away some conditional code). If it passes, I could clear the matrix to make CI faster. |
@pradyunsg FYI |
There seems to be some issue when running through |
|
Also re-organized tests parameters a bit.
5c09001
to
2262938
Compare
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.
LGTM
@@ -45,10 +27,10 @@ def key_from_ireq(ireq): | |||
def key_from_req(req): | |||
"""Get an all-lowercase version of the requirement's name.""" | |||
if hasattr(req, 'key'): | |||
# pip 8.1.1 or below, using pkg_resources | |||
# from pkg_resources, such as installed dists for pip-sync |
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.
I believe this will be stable now that we use vendored pip
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.
It thought so too, but unfortunately there's another condition unrelated to the pip version.
When parsing the installed packages (in pip-sync), the returned object by pip.get_installed_distributions
is a DistributionEggInfo
or something alike, which comes from pkg_resources
, and those object do not have a .name
attribute.
The if hasattr(req, 'key')
condition masked that case, making us believe it was only a pip difference.
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.
ok
@@ -221,10 +203,10 @@ def dedup(iterable): | |||
def name_from_req(req): | |||
"""Get the name of the requirement""" | |||
if hasattr(req, 'project_name'): | |||
# pip 8.1.1 or below, using pkg_resources |
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.
same: I believe this will be stable now that we use vendored pip
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.
See comment above: same applies.
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.
ok
For the record: I installed pip 10 (commit https://github.com/pypa/pip/tree/b20235d7c9e6a7ece969567ad0a21b55aea96624), and both pip-compile and pip-sync work fine so far. I went as far as to uninstall pip to test pip-compile, and it keeps working. Look like we'll survive through this! |
Vendoring pip 9.0.3 inside of pip-tools.
This will ensure the dependency resolution code will work even if users install pip 10.
The install/uninstall operations of
pip-sync
as still done through shell out, so they will benefit directly from pip 10, as those command will still be available.Fixes #580
This should be release in a Major 2.0.0 version.
Changelog-friendly one-liner: Vendored pip 9.0.3 to keep compatibility for users with pip 10
Contributor checklist