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

when uninstalling, look for the case of paths containing symlinked directories #3154

Merged
merged 4 commits into from Oct 3, 2015

Conversation

Projects
None yet
5 participants
@qwcode
Contributor

qwcode commented Sep 30, 2015

here's the fix I'd prefer for #3141 (as opposed to #3142)

It updates UninstallPathSet.add to deal with the case of paths containing symlinked directories.

this doesn't undo #2552.

I'll admit some possible regrets on merging #2552 which caused this, but I still logically think it's the right thing.

@qwcode

This comment has been minimized.

Show comment
Hide comment
@qwcode

qwcode Sep 30, 2015

Contributor

@pfmoore any advice on backporting os.path.samefile for windows <3.2 (see the description)

Contributor

qwcode commented Sep 30, 2015

@pfmoore any advice on backporting os.path.samefile for windows <3.2 (see the description)

@qwcode

This comment has been minimized.

Show comment
Hide comment
@qwcode

qwcode Oct 2, 2015

Contributor

@AvdN do you mind confirming this fix works for you as well? much appreciated if you can.

Contributor

qwcode commented Oct 2, 2015

@AvdN do you mind confirming this fix works for you as well? much appreciated if you can.

@AvdN

This comment has been minimized.

Show comment
Hide comment
@AvdN

AvdN Oct 3, 2015

@qwcode I can confirm this works for me as well.

In the end (after more than 1.5 hrs) I ended up cloning your repo, and switching to the branch issue_3011 to test this. Gathering the information from this page ( starting with the proper git remote add, seems impossible for someone with a limited VCS/DVCS experience (30/10y respectively))

AvdN commented Oct 3, 2015

@qwcode I can confirm this works for me as well.

In the end (after more than 1.5 hrs) I ended up cloning your repo, and switching to the branch issue_3011 to test this. Gathering the information from this page ( starting with the proper git remote add, seems impossible for someone with a limited VCS/DVCS experience (30/10y respectively))

@pfmoore

This comment has been minimized.

Show comment
Hide comment
@pfmoore

pfmoore Oct 3, 2015

Member

@qwcode well, all os.path.samefile does in 3.4 is compare st.ino and st.dev from the stat structure. It's os.stat that has been updated to give useful ino/dev values in Python 3.4+

From 3.2 to 3.4, os.path.samefile just checked the filename (using _getfinalpathname, which may do a few extra things like resolve symlinks, I haven't checked). But honestly, I think that testing if the (normalised absolute) pathname matches is as good as you're going to get on <3.2 (short of ctypes or C code).

Member

pfmoore commented Oct 3, 2015

@qwcode well, all os.path.samefile does in 3.4 is compare st.ino and st.dev from the stat structure. It's os.stat that has been updated to give useful ino/dev values in Python 3.4+

From 3.2 to 3.4, os.path.samefile just checked the filename (using _getfinalpathname, which may do a few extra things like resolve symlinks, I haven't checked). But honestly, I think that testing if the (normalised absolute) pathname matches is as good as you're going to get on <3.2 (short of ctypes or C code).

Show outdated Hide outdated pip/req/req_uninstall.py Outdated
@qwcode

This comment has been minimized.

Show comment
Hide comment
@qwcode

qwcode Oct 3, 2015

Contributor

sorry, @AvdN , I should have pasted some quick instructions for testing it : )

Contributor

qwcode commented Oct 3, 2015

sorry, @AvdN , I should have pasted some quick instructions for testing it : )

@qwcode qwcode changed the title from when compacting the uninstall set, detect when 2 paths are the same file to when compacting the uninstall set, look for the case of paths containing symlinked directories Oct 3, 2015

@qwcode

This comment has been minimized.

Show comment
Hide comment
@qwcode

qwcode Oct 3, 2015

Contributor

ok, I've updated this again with new logic based on this comment #3154 (comment)

see the new description.

I've tested locally (in addition to the automated tests) for the known case (#3011 (comment)), but if people want to test, then here's the commands to install this branch

virtualenv  mytestVE
source mytestVE/bin/activate
pip install -e git+https://github.com/qwcode/pip@issue_3011#egg=pip
Contributor

qwcode commented Oct 3, 2015

ok, I've updated this again with new logic based on this comment #3154 (comment)

see the new description.

I've tested locally (in addition to the automated tests) for the known case (#3011 (comment)), but if people want to test, then here's the commands to install this branch

virtualenv  mytestVE
source mytestVE/bin/activate
pip install -e git+https://github.com/qwcode/pip@issue_3011#egg=pip
@qwcode

This comment has been minimized.

Show comment
Hide comment
@qwcode

qwcode Oct 3, 2015

Contributor

cc @takluyver since this relates to your #2552.

Contributor

qwcode commented Oct 3, 2015

cc @takluyver since this relates to your #2552.

@takluyver

This comment has been minimized.

Show comment
Hide comment
@takluyver

takluyver Oct 3, 2015

Member

@qwcode Thanks for ccing me.

I'm having trouble trying to follow the logic in this PR, but from the description in #3141, I think it should be sufficient to normalise paths by resolving links in all but the last component:

join(realpath(dirname(p)), basename(p))

Since #2552 was only concerned with the complete path referring to a symlink, this shouldn't break it.

I think the code in this PR could fall down if you have a path where the whole path is a symlink, and a prefix of it is also a symlink:

/blah/blah/symlink_to_dir/blah/blah/symlink_to_file

Since islink() will be True on the whole path, you'll never normalise the earlier symlink. But I may be missing something.

Member

takluyver commented Oct 3, 2015

@qwcode Thanks for ccing me.

I'm having trouble trying to follow the logic in this PR, but from the description in #3141, I think it should be sufficient to normalise paths by resolving links in all but the last component:

join(realpath(dirname(p)), basename(p))

Since #2552 was only concerned with the complete path referring to a symlink, this shouldn't break it.

I think the code in this PR could fall down if you have a path where the whole path is a symlink, and a prefix of it is also a symlink:

/blah/blah/symlink_to_dir/blah/blah/symlink_to_file

Since islink() will be True on the whole path, you'll never normalise the earlier symlink. But I may be missing something.

@qwcode

This comment has been minimized.

Show comment
Hide comment
@qwcode

qwcode Oct 3, 2015

Contributor

thanks @takluyver . I think you're right about how this can break. I'll make another update.

Contributor

qwcode commented Oct 3, 2015

thanks @takluyver . I think you're right about how this can break. I'll make another update.

@qwcode

This comment has been minimized.

Show comment
Hide comment
@qwcode

qwcode Oct 3, 2015

Contributor

@takluyver , how about now?

Contributor

qwcode commented Oct 3, 2015

@takluyver , how about now?

@@ -41,3 +41,31 @@ def test_add_symlink(self, tmpdir, monkeypatch):
ups = UninstallPathSet(dist=Mock())
ups.add(l)
assert ups.paths == set([l])
def test_compact_shorter_path(self, monkeypatch):

This comment has been minimized.

@qwcode

qwcode Oct 3, 2015

Contributor

this is secondary, I just added this, because we had no unit test for this

@qwcode

qwcode Oct 3, 2015

Contributor

this is secondary, I just added this, because we had no unit test for this

@qwcode qwcode changed the title from when compacting the uninstall set, look for the case of paths containing symlinked directories to when uninstalling, look for the case of paths containing symlinked directories Oct 3, 2015

@takluyver

This comment has been minimized.

Show comment
Hide comment
@takluyver

takluyver Oct 3, 2015

Member

That definitely looks easier to understand. A couple of thoughts:

  • You probably want to pass tail through normcase(), since it's now bypassing normalize_path(), to ensure equivalent filenames on case-insensitive filesystems match.
  • I added the resolve_symlinks parameter to normalize_path in #2552. If no other uses have been added, you may want to remove it again now that it's not called here. On the other hand, it's not much extra complexity, and it's conceivable that it may one day be useful again.
Member

takluyver commented Oct 3, 2015

That definitely looks easier to understand. A couple of thoughts:

  • You probably want to pass tail through normcase(), since it's now bypassing normalize_path(), to ensure equivalent filenames on case-insensitive filesystems match.
  • I added the resolve_symlinks parameter to normalize_path in #2552. If no other uses have been added, you may want to remove it again now that it's not called here. On the other hand, it's not much extra complexity, and it's conceivable that it may one day be useful again.
@qwcode

This comment has been minimized.

Show comment
Hide comment
@qwcode

qwcode Oct 3, 2015

Contributor

ok, normcase added. good point.

as for the extra kwarg, yea, it's ok to leave IMO. might be used later.

Contributor

qwcode commented Oct 3, 2015

ok, normcase added. good point.

as for the extra kwarg, yea, it's ok to leave IMO. might be used later.

@takluyver

This comment has been minimized.

Show comment
Hide comment
@takluyver

takluyver Oct 3, 2015

Member

👍 from me

Member

takluyver commented Oct 3, 2015

👍 from me

qwcode added a commit that referenced this pull request Oct 3, 2015

Merge pull request #3154 from qwcode/issue_3011
when uninstalling, look for the case of paths containing symlinked directories

@qwcode qwcode merged commit 55a3ea8 into pypa:develop Oct 3, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@mathieulongtin

This comment has been minimized.

Show comment
Hide comment
@mathieulongtin

mathieulongtin Oct 5, 2015

Works on my end. Thanks!

mathieulongtin commented Oct 5, 2015

Works on my end. Thanks!

@pomegranited pomegranited referenced this pull request Apr 6, 2016

Closed

Fix reinstall pip dependencies #228

0 of 3 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment