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'
maxkrivich added 2 commits Jun 20, 2018
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

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.

@@ -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')

This comment has been minimized.

@uranusjr

uranusjr Jun 20, 2018
Member

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.

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')

This comment has been minimized.

@uranusjr

uranusjr Jun 20, 2018
Member

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?

This comment has been minimized.

@maxkrivich

maxkrivich Jun 20, 2018
Author Contributor

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

@uranusjr uranusjr added this to In progress in Better user experience via automation Jun 20, 2018
maxkrivich added 2 commits Jun 20, 2018
Quick refactor for improve readability of code
@maxkrivich
Copy link
Contributor Author

@maxkrivich maxkrivich commented Jun 20, 2018

@uranusjr fixes according to your comments.

@uranusjr
Copy link
Member

@uranusjr uranusjr commented Jun 20, 2018

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

@maxkrivich maxkrivich force-pushed the maxkrivich:fix-empty-requires branch from 8118ce1 to 7516ead Jun 20, 2018
@maxkrivich
Copy link
Contributor Author

@maxkrivich maxkrivich commented Jun 20, 2018

@uranusjr CI passed

@uranusjr
Copy link
Member

@uranusjr uranusjr commented Jun 20, 2018

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!

@techalchemy
Copy link
Member

@techalchemy techalchemy commented Jun 22, 2018

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

@uranusjr uranusjr moved this from In progress to Done in Better user experience Jul 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.