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

Pip18 support #672

Merged
merged 8 commits into from
Aug 29, 2018
Merged

Pip18 support #672

merged 8 commits into from
Aug 29, 2018

Conversation

techalchemy
Copy link
Contributor

@techalchemy techalchemy commented Aug 4, 2018

  • Restructure repository code for better modularity
  • Pip team has been working on the resolver a bit so this will make it easier to modify that code on its own
  • Update tests
  • Fixes Pip 18 support #670

Changelog-friendly one-liner: Updated resolver and compatibility shims to support pip 18.0.

Contributor checklist
  • Provided the tests for the changes (no substantive changes)
  • Requested (or received) a review from another contributor
  • Gave a clear one-line description in the PR (that the maintainers can add to CHANGELOG.md afterwards).

- Restructure repository code for better modularity
- Pip team has been working on the resolver a bit so this will make it
  easier to modify that code on its own
- Update tests

Signed-off-by: Dan Ryan <dan@danryan.co>
- Point at moved `cmdoptions` and `base_command`
- Update import script for better organization and laziness

Signed-off-by: Dan Ryan <dan@danryan.co>
Signed-off-by: Dan Ryan <dan@danryan.co>
Signed-off-by: Dan Ryan <dan@danryan.co>
Signed-off-by: Dan Ryan <dan@danryan.co>
Signed-off-by: Dan Ryan <dan@danryan.co>
Signed-off-by: Dan Ryan <dan@danryan.co>
Signed-off-by: Dan Ryan <dan@danryan.co>
@@ -11,6 +11,7 @@ env:
- PIP=8.1.1
- PIP=9.0.1
- PIP=9.0.3
- PIP=10.0.1
- PIP=master
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also PIP=18.0 should be tested. The master branch is not equal to the 18.0 version anymore, e.g. currently in master the Command class lives in pip._internal.cli.base_command, but in Pip 18.0 it's still in pip._internal.basecommand.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Latest is tested, which is 18. It doesn't particularly matter because the import shims are rewritten to handle both possibilities gracefully

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, I forgot that latest==18 (currently).

@@ -24,7 +28,7 @@ def do_import(module_path, subimport=None, old_path=None):
PackageFinder = do_import('index', 'PackageFinder')
FormatControl = do_import('index', 'FormatControl')
Wheel = do_import('wheel', 'Wheel')
Command = do_import('basecommand', 'Command')
cmdoptions = do_import('cmdoptions')
Command = do_import('cli.base_command', 'Command', old_path='basecommand')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this doesn't work with Pip 18.0.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using the new import shims it tries each possibility and falls back to the next ones

>>> def do_import(module_path, subimport=None, old_path=None):
  2         old_path = old_path or module_path
  3         prefixes = ["pip._internal", "pip"]
  4         paths = [module_path, old_path]
  5         search_order = ["{0}.{1}".format(p, pth) for p in prefixes for pth in paths if pth is not None]
  6         package = subimport if subimport else None
  7         for to_import in search_order:
  8             if not subimport:
  9                 to_import, _, package = to_import.rpartition(".")
 10             print('Import target: %s\tpackage: %s' % (to_import, package))
>>> do_import('cli.base_command', 'Command', old_path='basecommand')
Import target: pip._internal.cli.base_command   package: Command
Import target: pip._internal.basecommand        package: Command
Import target: pip.cli.base_command     package: Command
Import target: pip.basecommand  package: Command

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And in case you need proof:

 ~/g/pip-tools   pip18-support    vf tmp --python=python3.6
Running virtualenv with interpreter /home/hawk/.pyenv/shims/python3.6
Using base prefix '/home/hawk/.pyenv/versions/3.6.5'
New python executable in /home/hawk/.virtualenvs/tempenv-3369325970666/bin/python3.6
Also creating executable in /home/hawk/.virtualenvs/tempenv-3369325970666/bin/python
Installing setuptools, pip, wheel...done.
  ³ tempenv-3369325970666  ~/g/pip-tools   pip18-support    pip install -e .
Obtaining file:///home/hawk/git/pip-tools
[...]
Installing collected packages: click, first, six, pip-tools
  Running setup.py develop for pip-tools
Successfully installed click-6.7 first-2.0.1 pip-tools six-1.11.0
  ³ tempenv-3369325970666  ~/g/pip-tools   pip18-support    pip install ptpython
Collecting ptpython
  Downloading https://files.pythonhosted.org/packages/d0/dd/163a698a86b9de92857f128117034bdb36f5a784d839eea3bc07e2c858ae/ptpython-0.41-py3-none-any.whl (47kB)
    100% |████████████████████████████████| 51kB 1.1MB/s
[...]

  ³ tempenv-3369325970666  ~/g/pip-tools   pip18-support    ptpython
>>> from pip import __version__ as pip_version
>>> print(pip_version)
18.0
>>> from piptools._compat import Command
>>> Command
<class 'pip._internal.basecommand.Command'>
>>> 

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI if you are using the code for importing from the pip_compat file I recommend grabbing the updated version, it borrows some concepts from https://github.com/brettcannon/modutil to do imports a bit more efficiently

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, OK, great that it works. Sorry for the incorrect analysis.

Good work! 👍

Copy link
Contributor

@suutari suutari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Thanks @techalchemy!

@techalchemy
Copy link
Contributor Author

Thanks for posting your fixes by the way, if there's anything else you changed let me know!

@techalchemy techalchemy closed this Aug 5, 2018
@techalchemy techalchemy reopened this Aug 5, 2018
@akaIDIOT
Copy link

Can confirm @techalchemy's fixes work for me, thanks for the effort! 👍

Any ideas on when this will get merged + released to PyPI?

@suutari-ai suutari-ai merged commit 446e56d into jazzband:master Aug 29, 2018
@techalchemy
Copy link
Contributor Author

@suutari FYI I released a new library recently which you might be interested in to clean up some of the pip import code--

https://pypi.org/project/pip-shims/

@vphilippon vphilippon added this to the 3.0.0 milestone Sep 18, 2018
else:
del os.environ['PIP_REQ_TRACKER']
try:
self.wheel_cache.cleanup()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@techalchemy shouldn't there be a wheel_cache.cleanup() instead?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! And how unfortunate that the error is masked by the try-except. Maybe instead of try-except, it would be better to use:

if hasattr(wheel_cache, 'cleanup'):
    wheel_cache.cleanup()

IMHO that's easier to read and less error prone.

Copy link
Member

@atugushev atugushev Oct 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The hasattr() has a quircks on Python 2, see the explanation: https://hynek.me/articles/hasattr/

Take a look at my suggestion in #968, btw.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pip 18 support
6 participants