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

Don't crash on invalid requirements in installed packages #5842

Merged
merged 10 commits into from
Oct 22, 2018

Conversation

takluyver
Copy link
Member

See #5839. This adds exception handling in two places:

  1. Inside the checking code, so that an error message can be logged which describes the package name - the tracebacks at the moment don't make it clear which package has the problem.
  2. Where the pip install machinery calls into the check machinery, because I believe that an uncaught error in checking shouldn't be able to crash the install process.

@benoit-pierre
Copy link
Member

What's the return code of pip check if that happen?

@takluyver
Copy link
Member Author

Whatever it would be if that package wasn't there - it will check the other packages as normal.

I guess pip check should exit with code 1 if there's a problem parsing requirements?

@benoit-pierre
Copy link
Member

I think yes.

@takluyver
Copy link
Member Author

OK, pip check should now exit with code 1 if such a warning is printed. pip install will print the warning but it won't affect the exit code.

@cjerdonek cjerdonek added T: bugfix C: check Checking dependency graph for consistency labels Oct 3, 2018
Copy link
Member

@xavfernandez xavfernandez left a comment

Choose a reason for hiding this comment

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

Any chance we could get a test with it ? :)

@uranusjr uranusjr mentioned this pull request Oct 14, 2018
@takluyver
Copy link
Member Author

I added a test; I'm not working on this further at the moment.

Copy link
Member

@xavfernandez xavfernandez 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 test :)

@xavfernandez xavfernandez merged commit 3b8c076 into pypa:master Oct 22, 2018
@benoit-pierre
Copy link
Member

@xavfernandez: it might be better holding-off merging things until Github is fully operational again, see https://status.github.com/messages.

@xavfernandez
Copy link
Member

I'm optimistic ;)

@jakirkham
Copy link
Contributor

Would it be possible to get a release with this fix?

@pfmoore
Copy link
Member

pfmoore commented Nov 20, 2018

The next release is scheduled for January - see https://pip.pypa.io/en/latest/development/release-process/#release-cadence

@jakirkham
Copy link
Contributor

Would it be reasonable to make a small patch release that contains a backport of this change?

@pradyunsg pradyunsg added this to the 19.0 milestone Jan 3, 2019
@lock
Copy link

lock bot commented May 31, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot added the auto-locked Outdated issues that have been locked by automation label May 31, 2019
@lock lock bot locked as resolved and limited conversation to collaborators May 31, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
auto-locked Outdated issues that have been locked by automation C: check Checking dependency graph for consistency
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants