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

Fix pipenv crashes #2388

Closed
wants to merge 5 commits into from
Closed

Conversation

maxkrivich
Copy link
Contributor

@maxkrivich maxkrivich commented Jun 20, 2018

Add extra checks for CLI params for preventing unexpected crashes and add the more informal message for that exceptions.

To reproduce those bugs type following command in the terminal.

$ pipenv install -e
Traceback (most recent call last):
  File "/usr/local/bin/pipenv", line 11, in <module>
    load_entry_point('pipenv', 'console_scripts', 'pipenv')()
  File "/Users/maxkrivich/Projects/pipenv/pipenv/vendor/click/core.py", line 722, in __call__
    return self.main(*args, **kwargs)
  File "/Users/maxkrivich/Projects/pipenv/pipenv/vendor/click/core.py", line 697, in main
    rv = self.invoke(ctx)
  File "/Users/maxkrivich/Projects/pipenv/pipenv/vendor/click/core.py", line 1066, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
  File "/Users/maxkrivich/Projects/pipenv/pipenv/vendor/click/core.py", line 895, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "/Users/maxkrivich/Projects/pipenv/pipenv/vendor/click/core.py", line 535, in invoke
    return callback(*args, **kwargs)
  File "/Users/maxkrivich/Projects/pipenv/pipenv/cli.py", line 416, in install
    selective_upgrade=selective_upgrade,
  File "/Users/maxkrivich/Projects/pipenv/pipenv/core.py", line 1892, in do_install
    package_name = ' '.join([package_name, more_packages.pop(0)])
IndexError: pop from empty list

$ pipenv install <some_module> -i
/Users/maxkrivich/Projects/pipenv/pipenv/vendor/requirements/parser.py:44: UserWarning: Private repos not supported. Skipping.
  warnings.warn('Private repos not supported. Skipping.')
Traceback (most recent call last):
  File "/usr/local/bin/pipenv", line 11, in <module>
    load_entry_point('pipenv', 'console_scripts', 'pipenv')()
  File "/Users/maxkrivich/Projects/pipenv/pipenv/vendor/click/core.py", line 722, in __call__
    return self.main(*args, **kwargs)
  File "/Users/maxkrivich/Projects/pipenv/pipenv/vendor/click/core.py", line 697, in main
    rv = self.invoke(ctx)
  File "/Users/maxkrivich/Projects/pipenv/pipenv/vendor/click/core.py", line 1066, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
  File "/Users/maxkrivich/Projects/pipenv/pipenv/vendor/click/core.py", line 895, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "/Users/maxkrivich/Projects/pipenv/pipenv/vendor/click/core.py", line 535, in invoke
    return callback(*args, **kwargs)
  File "/Users/maxkrivich/Projects/pipenv/pipenv/cli.py", line 416, in install
    selective_upgrade=selective_upgrade,
  File "/Users/maxkrivich/Projects/pipenv/pipenv/core.py", line 1987, in do_install
    pypi_mirror=pypi_mirror,
  File "/Users/maxkrivich/Projects/pipenv/pipenv/core.py", line 1408, in pip_install
    package_name.split('--hash')[0].split('--trusted-host')[0]
  File "/Users/maxkrivich/Projects/pipenv/pipenv/vendor/requirementslib/models/requirements.py", line 717, in from_line
    r = NamedRequirement.from_line(line)
  File "/Users/maxkrivich/Projects/pipenv/pipenv/vendor/requirementslib/models/requirements.py", line 75, in from_line
    if req.specifier:
AttributeError: 'NoneType' object has no attribute 'specifier'

Fix IndexError exception when `more_packages` is empty and add the more
informal message for argument usage.

To reproduce this issue `pipenv install -e`
Add extra check for -i option for fix `AttributeError: 'NoneType'`

To reproduce this bug `pipenv install <module> -i`
Copy link
Member

@uranusjr uranusjr left a comment

Choose a reason for hiding this comment

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

Thanks for the patch! The logic seems correct to me, but I left some comments about wording and coding styles. The latter is not your fault (the original code is quite confusing to start with), but I feel it is a good chance to refactor it a bit.

This would close #2383.

pipenv/core.py Outdated
@@ -1889,12 +1889,16 @@ def do_install(
# Capture -e argument and assign it to following package_name.
more_packages = list(more_packages)
if package_name == '-e':
if not more_packages:
raise click.BadArgumentUsage('Please provide path to setup.py')
Copy link
Member

Choose a reason for hiding this comment

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

This message could be misleading. We don’t want a path to setup.py, but to the directory containing it. I would use something like “path to editable package (e.g. the directory containing setup.py)” instead.

pipenv/core.py Outdated
package_name = ' '.join([package_name, more_packages.pop(0)])
# capture indexes and extra indexes
line = [package_name] + more_packages
index_indicators = ['-i', '--index', '--extra-index-url']
index, extra_indexes = None, None
if more_packages and any(more_packages[0].startswith(s) for s in index_indicators):
if len(more_packages) < 2:
raise click.BadArgumentUsage('Please provide index value')
Copy link
Member

Choose a reason for hiding this comment

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

I feel the logic in this block can be rewritten a bit to make the checks clearer. Right now the various checks are a bit convoluted, and require a lot of context to be understood. Why does it use more_packages here, but package_names below? How do they relate to line?

Copy link
Contributor Author

@maxkrivich maxkrivich Jun 20, 2018

Choose a reason for hiding this comment

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

@uranusjr I agree with you about some extra context what you should keep in mind. I'll try to rewrite this block.

Quick refactor for improve readability of code
@maxkrivich
Copy link
Contributor Author

@uranusjr fixes according to your comments.

@uranusjr
Copy link
Member

CI is complaining… (the refactor look legit in general to me, just need more careful argument checking)

@maxkrivich
Copy link
Contributor Author

@uranusjr CI passed

@uranusjr
Copy link
Member

Weeeeeee

We’re in code freeze mode right now (release immenent) so this will need to wait a while to be merged, but it will be in the next version!

@uranusjr uranusjr added the PR: awaiting-merge The PR related to this issue has been reviewed and is awaiting merge. label Jun 20, 2018
@techalchemy
Copy link
Member

I pulled this branch and added a test to it and merged it, thanks a bunch for putting this together! Closed via beb6aef

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: awaiting-merge The PR related to this issue has been reviewed and is awaiting merge.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants