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

Add unit tests for easy-install.pth uninstall #6517

Merged
merged 2 commits into from
Aug 7, 2019

Conversation

MHendricks
Copy link
Contributor

Resolves issue #6516.

If uninstalling a editable package in Python 2.7 that was installed using a UNC file path, the path is not removed from the easy-install.pth file.

Change use of os.path.splitdrive to os.path.isabs and add unit tests for .pth file handling.

Copy link
Member

@pfmoore pfmoore left a comment

Choose a reason for hiding this comment

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

One minor comment, but it doesn't need addressing and I'm +1 on this change.

@@ -593,7 +593,7 @@ def add(self, entry):
# backslashes. This is correct for entries that describe absolute
# paths outside of site-packages, but all the others use forward
# slashes.
if WINDOWS and not os.path.splitdrive(entry)[0]:
if WINDOWS and not os.path.isabs(entry):
Copy link
Member

Choose a reason for hiding this comment

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

Technically, this will behave differently on a non-absolute path with a drive letter, like C:foo\bar. To be honest, though, I can't imagine why we'd ever encounter a real life case where that would happen, so I'm inclined to ignore the issue. And as a general point, isabs seems clearer to me.

My only (very slight) reservation is that not using isabs in the first place is non-obvious enough that thee may have been a reason for it. But it seems to have been introduced in 770930d, without comment, so I'm inclined to think there was no particular motive.

@pfmoore
Copy link
Member

pfmoore commented May 20, 2019

Looks like there are some test failures that will need addressing, though :-)

@MHendricks
Copy link
Contributor Author

https://stackoverflow.com/a/42014874 The problem with the test was due to a change in python 2.7.8 where isabs returns False if you don't pass more than just the server\share. os.path.isabs('\\\\server\\share') returns False on 2.7.8+ while os.path.isabs('\\\\server\\share\\') returns True. I was developing on a older version of python and though I had tested on newer python installs, but apparently not.

Testing on 2.7.15, I get the following results:

>>> p = '\\\\example\\share\\case'; print not os.path.isabs(p), not os.path.splitdrive(p)[0], os.path.splitdrive(p)
False False ('\\\\example\\share', '\\case')
>>> p = '\\\\example\\share\\'; print not os.path.isabs(p), not os.path.splitdrive(p)[0], os.path.splitdrive(p)
False False ('\\\\example\\share', '\\')
>>> p = '\\\\example\\share'; print not os.path.isabs(p), not os.path.splitdrive(p)[0], os.path.splitdrive(p)
True False ('\\\\example\\share', '')
>>> p = '\\\\example'; print not os.path.isabs(p), not os.path.splitdrive(p)[0], os.path.splitdrive(p)
False True ('', '\\\\example')
>>> p = 'C:\\foo\\bar'; print not os.path.isabs(p), not os.path.splitdrive(p)[0], os.path.splitdrive(p)
False False ('C:', '\\foo\\bar')
>>> p = 'C:foo\\bar'; print not os.path.isabs(p), not os.path.splitdrive(p)[0], os.path.splitdrive(p)
True False ('C:', 'foo\\bar')
>>> p = 'foo\\bar'; print not os.path.isabs(p), not os.path.splitdrive(p)[0], os.path.splitdrive(p)
True True ('', 'foo\\bar')

It looks like isabs fails if you are using a unc share as the root of your pip package, which I think is unlikely, but would be supported if using the original splitdrive.

The root of the problem was due to me using a old un-supported version of python. If you want I can revert the change to isabs, but leave the unit test changes, or just close the pull request.

@cjerdonek
Copy link
Member

If nothing else, I think it would be worth documenting your findings in a code comment so it won’t need to be rediscovered and because it seems obscure.

@pfmoore
Copy link
Member

pfmoore commented May 21, 2019

Excellent bit of diagnosis! I agree with @cjerdonek that documenting the reason that isabs won't work would be very useful. Personally, I'm not sure that the behaviour of isabs in this edge case makes much sense, but I can see why consistency across os.path APIs is a reasonable justification for picking this particular behaviour. And I'm +1 on keeping the tests, simply because more tests is good.

@MHendricks MHendricks changed the title Use os.path.isabs to check for absolute paths on windows Add unit tests for easy-install.pth uninstall May 21, 2019
@MHendricks
Copy link
Contributor Author

Documentation added to req_uninstall.py and original splitdrive check restored.

@BrownTruck
Copy link
Contributor

Hello!

I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the master branch into this pull request or rebase this pull request against master then it will be eligible for code review and hopefully merging!

@BrownTruck BrownTruck added the needs rebase or merge PR has conflicts with current master label Jul 27, 2019
@pypa-bot pypa-bot removed the needs rebase or merge PR has conflicts with current master label Jul 30, 2019
@chrahunt
Copy link
Member

chrahunt commented Aug 7, 2019

Anything else needed for this?

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.

I have some code style disagreements but I'm okay functionally.

I'll defer to others to merge though.

@pfmoore pfmoore merged commit d44938c into pypa:master Aug 7, 2019
@pfmoore
Copy link
Member

pfmoore commented Aug 7, 2019

Thanks for the contribution!

@lock lock bot added the auto-locked Outdated issues that have been locked by automation label Sep 6, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Sep 6, 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants