Don't follow symlinks when uninstalling files #2552
Conversation
|
Test failure appears to be an unrelated download failure, if someone could kick that build to restart. :-) |
|
are these links installed as part of a project using pip? or no? btw, as it is, I'm not aware that it's possible to install symlinks under normal circumstances, but maybe I'm wrong |
|
The symlinks are installed by my tool for package developers. When the
package is uploaded to pypi, downstream users install it using pip. This
problem arises if you symlink a package using flit and then try to remove
it or install over it with pip.
|
|
More broadly, though, there are various ways that someone could end up with symlinks present - the situation described in #40 was caused by something completely different, and I have created symlinks other ways as well. It would be nice if pip could avoid messing them up. |
| @@ -47,7 +47,7 @@ def _can_uninstall(self): | |||
| return True | |||
|
|
|||
| def add(self, path): | |||
| path = normalize_path(path) | |||
| path = os.path.normcase(os.path.expanduser(path)) | |||
There was a problem hiding this comment.
how about a "follow_symlink" kwarg to normalize_path
add a simple unit test for normalize_path and UninstallPathSet.add. we have no tests currently for UninstallPathSet
- for normalize_path: tests/unit/test_utils.py
- for add:
tests/unit/test_req_uninstall.py(new module)
There was a problem hiding this comment.
or maybe a "realpath" kwarg (default=True)
There was a problem hiding this comment.
I called it resolve_symlinks. I considered realpath, but it seemed confusing to have parameters called 'path' and 'realpath'. I'm happy to change this to another name if you prefer.
|
see comment above about tests, but I agree it seems pip is wrong here. |
And call abspath() when not following symlinks
Still not very happy
| os.symlink(f, 'file_link') | ||
|
|
||
| assert normalize_path('dir_link/file1', resolve_symlinks=True) \ | ||
| == os.path.join(self.tempdir, f) |
There was a problem hiding this comment.
The Travis pep8 tests are failing on this line (and the similar lines below), with:
E127 continuation line over-indented for visual indent
I can't find an indentation that makes it happy - one extra space of indentation is still 'over-indented', and zero spaces gives another warning about 'missing indentation'. What is the desired formatting for a statement like this?
There was a problem hiding this comment.
assert normalize_path(
"dir_link/file1", resolve_symlinks=True,
) == os.path.join(self.tempdir, f)Will probably work.
|
OK, tests added, passing and pep8 compliant. :-) |
| if running_under_virtualenv(): | ||
| # Construct tempdir in sys.prefix, otherwise UninstallPathSet | ||
| # will reject paths. | ||
| self.tempdir = tempfile.mkdtemp(prefix=sys.prefix) |
There was a problem hiding this comment.
I'd rather not have the pip test suite creating temp in sys.prefix.
how about a monkeypatch for is_local instead
|
overall, this lgtm if you can fix those few things. |
|
Thanks, fixed. The remaining test failure looks like it's unrelated: |
|
ok, lgtm. merging. |
Don't follow symlinks when uninstalling files
|
Thanks
|
lock
bot
added
the
auto-locked
I'm developing a packaging tool that, among other things, symlinks things into site-packages for development installs (i.e. similar to
pip install -e). When pip tries to uninstall those things, it erroneously removes the target of the symlink instead of the symlink itself. This has been tracked for some time as issue #40, and the comments on that issue seemed to agree that resolving the symlink when uninstalling was plain wrong.I could have fixed this in
normalize_path()itself, but it may be desirable in some other places to resolve the symlink, and in most cases it will at least not be harmful. Since that function is only one line, I opted to copy its contents, minusrealpath(), to where it's used.Fixes gh-40
Crosslink pypa/flit#2.