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.

Sign up for GitHub

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

Jump to bottom

Make failed uninstalls roll back more reliably and better at avoiding naming conflicts #6225

Merged
merged 6 commits into from
Feb 3, 2019
Merged

Make failed uninstalls roll back more reliably and better at avoiding naming conflicts #6225

merged 6 commits into from
Feb 3, 2019

Conversation

zooba
Copy link
Contributor

@zooba zooba commented Jan 30, 2019

Fixes #6194

@zooba zooba mentioned this pull request Jan 30, 2019
Merged
pradyunsg
pradyunsg approved these changes Jan 31, 2019
View reviewed changes
Copy link
Member

@pradyunsg pradyunsg left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM, other than the lack of direct tests, but I'd appreciate another @pypa/pip-committers's review on this.

cjerdonek
cjerdonek approved these changes Jan 31, 2019
View reviewed changes
Copy link
Member

@cjerdonek cjerdonek 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 breaking this fix out separately, @zooba.

Approving this now, but continuing what @pradyunsg said, is there any way a regression test can be added for this case (can also be added as part of PR #6215)?

src/pip/_internal/utils/temp_dir.py Outdated
@@ -98,7 +98,7 @@ class AdjacentTempDirectory(TempDirectory):

"""
# The characters that may be used to name the temp directory
LEADING_CHARS = "-~.+=%0123456789"
LEADING_CHARS = "~.=%0123456789"
Copy link
Member

Choose a reason for hiding this comment

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

Since the reason isn't obvious and for anyone in the future that might want to modify this, I think it would be good to add a comment here recording the lesson of why - isn't in the list (see #6194 (comment)).

It's okay for this to be done in PR #6215 after this is merged.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, will do

@cjerdonek cjerdonek mentioned this pull request Jan 31, 2019
Closed
@cjerdonek cjerdonek added C: uninstall The logic for uninstallation of packages type: bugfix labels Jan 31, 2019
@zooba
Copy link
Contributor Author

zooba commented Jan 31, 2019

Actually, I'm playing around with pkg_resources now and it's looking like the best option is to always have a leading ~ and substitue characters after that. PyPI packages can start with numbers, and while the Python package can't, the dist-info directory will.

The crash was only because pkg_resources checks for an empty directory to skip, and then fails when the dist-info directory has contents that is not valid metadata. I can handle this scenario in freeze, but it probably will show up all over the place.

@zooba
Copy link
Contributor Author

zooba commented Jan 31, 2019

And I'll mention again, that this scenarios only occurs when we've already failed in some other way (incorrectly cleaning up after uninstallation). So it's not a normal or expected scenario by any means, though obviously it's one that a few people have already hit.

@cjerdonek
Copy link
Member

I can handle this scenario in freeze, but it probably will show up all over the place.

This should be discussed separately in a different PR (one fix per PR). If you want to add more commits here, I'd focus on the LEADING_CHARS code comment and regression test @pradyunsg and I mentioned. Otherwise, we said it looked okay with what you had before.

@zooba
Copy link
Contributor Author

zooba commented Jan 31, 2019

Unfortunately, the original problem isn't really LEADING_CHARS. It's that pkg_resources raises a different error for corrupt metadata and an unparseable name than either one but not the other. So everything outside of req_uninstall.py is unrelated, but if I add tests based on the scenario then they will be failing until we add handling for the real bug.

Instead, I'll dig into the rollback tests, which should have caught this in the first place. But that needs to wait for tomorrow - I'm busy rest of today.

@cjerdonek cjerdonek mentioned this pull request Feb 2, 2019
Closed
pradyunsg
pradyunsg approved these changes Feb 2, 2019
View reviewed changes
Copy link
Member

@pradyunsg pradyunsg left a comment

Choose a reason for hiding this comment

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

LGTM.

tests/unit/test_utils.py Outdated Show resolved Hide resolved
@pradyunsg
Copy link
Member

@cjerdonek @zooba Let's get to the lack of relevant tests (and any issues they might identify) in a separate PR?

@pradyunsg
Copy link
Member

@cjerdonek @zooba Let's get to the lack of relevant tests (and any issues they might identify) in a separate PR?

^ ping

@pradyunsg
Copy link
Member

If there's nothing else needed here in your opinion @cjerdonek, let's merge this. :)

@cjerdonek
Copy link
Member

@pradyunsg I had approved this earlier (with the request to handle the new commit separately) and said the regression test could be added / discussed in a later PR.

@cjerdonek
Copy link
Member

I would squash first to cancel the reverted commit first though.

@cjerdonek cjerdonek merged commit b5dd279 into pypa:master Feb 3, 2019
@pradyunsg
Copy link
Member

I had approved this earlier (with the request to handle the new commit separately) and said the regression test could be added / discussed in a later PR.

Yea, I was just being cautious in making sure you're okay with the subsequent commits.

@pradyunsg pradyunsg added this to the 19.0 milestone Feb 3, 2019
@zooba zooba deleted the issue-6194 branch February 4, 2019 19:00
@maynardflies
Copy link

Hi folks, just wanted to check on this issue and ask if there is a plan to make a release with this fix anytime soon. This has some effect on our update script and we want to know how we should proceed. Thanks for the quick turnaround on the fix!

@pradyunsg
Copy link
Member

There is.

This'll be a part of 19.0.2, planned to be released late this week or early next week: #6106.

pradyunsg added a commit that referenced this pull request Feb 8, 2019
@pradyunsg
AdjacentTempDirectory should fail on unwritable directory (#6215)
7eb79b1 
Based on #6225
@lock
Copy link

lock bot commented May 29, 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 29, 2019
@lock lock bot locked as resolved and limited conversation to collaborators May 29, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@cjerdonek cjerdonek cjerdonek approved these changes

@pradyunsg pradyunsg pradyunsg approved these changes

Assignees
No one assigned
Labels
auto-locked Outdated issues that have been locked by automation C: uninstall The logic for uninstallation of packages
Projects
None yet
Milestone
19.0
Development

Successfully merging this pull request may close these issues.

pip19.0.1 list error"AttributeError: _version"
4 participants
@zooba @cjerdonek @pradyunsg @maynardflies