From f8d58256b74ad1b8909a4efd86bdcc4449f54ef3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20Bidoul=20=28ACSONE=29?= Date: Fri, 23 Aug 2019 23:11:12 +0200 Subject: [PATCH 1/5] Add failing test for symlink uninstall --- tests/functional/test_uninstall.py | 21 +++++++++++++++++++++ tests/lib/path.py | 10 ++++++++++ 2 files changed, 31 insertions(+) diff --git a/tests/functional/test_uninstall.py b/tests/functional/test_uninstall.py index 128d66102..2e7330fee 100644 --- a/tests/functional/test_uninstall.py +++ b/tests/functional/test_uninstall.py @@ -458,6 +458,27 @@ def test_uninstall_wheel(script, data): assert_all_changes(result, result2, []) +@pytest.mark.skipif("sys.platform == 'win32'") +def test_uninstall_with_symlink(script, data, tmpdir): + """ + Test uninstalling a wheel, with an additional symlink + https://github.com/pypa/pip/issues/6892 + """ + package = data.packages.joinpath("simple.dist-0.1-py2.py3-none-any.whl") + script.pip('install', package, '--no-index') + symlink_target = tmpdir / "target" + symlink_target.mkdir() + symlink_source = script.site_packages / "symlink" + (script.base_path / symlink_source).symlink_to(symlink_target) + st_mode = symlink_target.stat().st_mode + distinfo_path = script.site_packages_path / 'simple.dist-0.1.dist-info' + record_path = distinfo_path / 'RECORD' + record_path.append_text("symlink,,\n") + uninstall_result = script.pip('uninstall', 'simple.dist', '-y') + assert symlink_source in uninstall_result.files_deleted + assert symlink_target.stat().st_mode == st_mode + + def test_uninstall_setuptools_develop_install(script, data): """Try uninstall after setup.py develop followed of setup.py install""" pkg_path = data.packages.joinpath("FSPkg") diff --git a/tests/lib/path.py b/tests/lib/path.py index cb2e6bda7..d54146868 100644 --- a/tests/lib/path.py +++ b/tests/lib/path.py @@ -203,9 +203,19 @@ class Path(_base): with open(self, "w") as fp: fp.write(content) + def append_text(self, content): + with open(self, "a") as fp: + fp.write(content) + def touch(self): with open(self, "a") as fp: path = fp.fileno() if os.utime in supports_fd else self os.utime(path, None) # times is not optional on Python 2.7 + def symlink_to(self, target): + os.symlink(target, self) + + def stat(self): + return os.stat(self) + curdir = Path(os.path.curdir) From 885fdc375446258cbc66fde576118c1a3defddc1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20Bidoul=20=28ACSONE=29?= Date: Sun, 18 Aug 2019 12:46:11 +0200 Subject: [PATCH 2/5] Correctly uninstall symlinks --- news/6892.bugfix | 2 ++ src/pip/_internal/utils/misc.py | 16 ++++++++-------- tests/unit/test_req_uninstall.py | 4 ++-- 3 files changed, 12 insertions(+), 10 deletions(-) create mode 100644 news/6892.bugfix diff --git a/news/6892.bugfix b/news/6892.bugfix new file mode 100644 index 000000000..763f2520b --- /dev/null +++ b/news/6892.bugfix @@ -0,0 +1,2 @@ +Correctly uninstall symlinks that were installed in a virtualenv, +by tools such as ``flit install --symlink``. \ No newline at end of file diff --git a/src/pip/_internal/utils/misc.py b/src/pip/_internal/utils/misc.py index 399de7116..e59aba534 100644 --- a/src/pip/_internal/utils/misc.py +++ b/src/pip/_internal/utils/misc.py @@ -427,10 +427,12 @@ def is_local(path): If we're not in a virtualenv, all paths are considered "local." + Caution: this function assumes the head of path has been normalized + with normalize_path. """ if not running_under_virtualenv(): return True - return normalize_path(path).startswith(normalize_path(sys.prefix)) + return path.startswith(normalize_path(sys.prefix)) def dist_is_local(dist): @@ -450,8 +452,7 @@ def dist_in_usersite(dist): """ Return True if given Distribution is installed in user site. """ - norm_path = normalize_path(dist_location(dist)) - return norm_path.startswith(normalize_path(user_site)) + return dist_location(dist).startswith(normalize_path(user_site)) def dist_in_site_packages(dist): @@ -460,9 +461,7 @@ def dist_in_site_packages(dist): Return True if given Distribution is installed in sysconfig.get_python_lib(). """ - return normalize_path( - dist_location(dist) - ).startswith(normalize_path(site_packages)) + return dist_location(dist).startswith(normalize_path(site_packages)) def dist_is_editable(dist): @@ -593,11 +592,12 @@ def dist_location(dist): packages, where dist.location is the source code location, and we want to know where the egg-link file is. + The returned location is normalized (in particular, with symlinks removed). """ egg_link = egg_link_path(dist) if egg_link: - return egg_link - return dist.location + return normalize_path(egg_link) + return normalize_path(dist.location) def current_umask(): diff --git a/tests/unit/test_req_uninstall.py b/tests/unit/test_req_uninstall.py index 69dbeebfe..ce5940d75 100644 --- a/tests/unit/test_req_uninstall.py +++ b/tests/unit/test_req_uninstall.py @@ -183,7 +183,7 @@ class TestUninstallPathSet(object): def test_compact_shorter_path(self, monkeypatch): monkeypatch.setattr(pip._internal.req.req_uninstall, 'is_local', - lambda p: True) + mock_is_local) monkeypatch.setattr('os.path.exists', lambda p: True) # This deals with nt/posix path differences short_path = os.path.normcase(os.path.abspath( @@ -196,7 +196,7 @@ class TestUninstallPathSet(object): @pytest.mark.skipif("sys.platform == 'win32'") def test_detect_symlink_dirs(self, monkeypatch, tmpdir): monkeypatch.setattr(pip._internal.req.req_uninstall, 'is_local', - lambda p: True) + mock_is_local) # construct 2 paths: # tmpdir/dir/file From 75a12ff42359316374365656400319589666d3ac Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20Bidoul=20=28ACSONE=29?= Date: Sat, 24 Aug 2019 23:27:56 +0200 Subject: [PATCH 3/5] Add failing test for StashedUninstallPathSet symlink --- tests/unit/test_req_uninstall.py | 64 ++++++++++++++++++++++++++++++++ 1 file changed, 64 insertions(+) diff --git a/tests/unit/test_req_uninstall.py b/tests/unit/test_req_uninstall.py index ce5940d75..d4d707e60 100644 --- a/tests/unit/test_req_uninstall.py +++ b/tests/unit/test_req_uninstall.py @@ -306,3 +306,67 @@ class TestStashedUninstallPathSet(object): for old_path, new_path in stashed_paths: assert os.path.exists(old_path) assert not os.path.exists(new_path) + + @pytest.mark.skipif("sys.platform == 'win32'") + def test_commit_symlinks(self, tmpdir): + adir = tmpdir / "dir" + adir.mkdir() + dirlink = tmpdir / "dirlink" + dirlink.symlink_to(adir) + afile = tmpdir / "file" + afile.write_text("...") + filelink = tmpdir / "filelink" + filelink.symlink_to(afile) + + pathset = StashedUninstallPathSet() + stashed_paths = [] + stashed_paths.append(pathset.stash(dirlink)) + stashed_paths.append(pathset.stash(filelink)) + for stashed_path in stashed_paths: + assert os.path.lexists(stashed_path) + assert not os.path.exists(dirlink) + assert not os.path.exists(filelink) + + pathset.commit() + + # stash removed, links removed + for stashed_path in stashed_paths: + assert not os.path.lexists(stashed_path) + assert not os.path.lexists(dirlink) and not os.path.isdir(dirlink) + assert not os.path.lexists(filelink) and not os.path.isfile(filelink) + + # link targets untouched + assert os.path.isdir(adir) + assert os.path.isfile(afile) + + @pytest.mark.skipif("sys.platform == 'win32'") + def test_rollback_symlinks(self, tmpdir): + adir = tmpdir / "dir" + adir.mkdir() + dirlink = tmpdir / "dirlink" + dirlink.symlink_to(adir) + afile = tmpdir / "file" + afile.write_text("...") + filelink = tmpdir / "filelink" + filelink.symlink_to(afile) + + pathset = StashedUninstallPathSet() + stashed_paths = [] + stashed_paths.append(pathset.stash(dirlink)) + stashed_paths.append(pathset.stash(filelink)) + for stashed_path in stashed_paths: + assert os.path.lexists(stashed_path) + assert not os.path.lexists(dirlink) + assert not os.path.lexists(filelink) + + pathset.rollback() + + # stash removed, links restored + for stashed_path in stashed_paths: + assert not os.path.lexists(stashed_path) + assert os.path.lexists(dirlink) and os.path.isdir(dirlink) + assert os.path.lexists(filelink) and os.path.isfile(filelink) + + # link targets untouched + assert os.path.isdir(adir) + assert os.path.isfile(afile) From db3320e4629ae4bf0bea4539a0a3a8b073f5ecec Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20Bidoul=20=28ACSONE=29?= Date: Sat, 24 Aug 2019 23:04:01 +0200 Subject: [PATCH 4/5] Add symlink support to StashedUninstallPathSet --- src/pip/_internal/req/req_uninstall.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/pip/_internal/req/req_uninstall.py b/src/pip/_internal/req/req_uninstall.py index e35e47615..6b551c91b 100644 --- a/src/pip/_internal/req/req_uninstall.py +++ b/src/pip/_internal/req/req_uninstall.py @@ -267,14 +267,16 @@ class StashedUninstallPathSet(object): def stash(self, path): # type: (str) -> str """Stashes the directory or file and returns its new location. + Handle symlinks as files to avoid modifying the symlink targets. """ - if os.path.isdir(path): + path_is_dir = os.path.isdir(path) and not os.path.islink(path) + if path_is_dir: new_path = self._get_directory_stash(path) else: new_path = self._get_file_stash(path) self._moves.append((path, new_path)) - if os.path.isdir(path) and os.path.isdir(new_path): + if (path_is_dir and os.path.isdir(new_path)): # If we're moving a directory, we need to # remove the destination first or else it will be # moved to inside the existing directory. @@ -301,7 +303,7 @@ class StashedUninstallPathSet(object): for new_path, path in self._moves: try: logger.debug('Replacing %s from %s', new_path, path) - if os.path.isfile(new_path): + if os.path.isfile(new_path) or os.path.islink(new_path): os.unlink(new_path) elif os.path.isdir(new_path): rmtree(new_path) From 04bc6d090dc6d4a11b4af1c470a63f8986a0c8c8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20Bidoul=20=28ACSONE=29?= Date: Fri, 6 Sep 2019 08:24:10 +0200 Subject: [PATCH 5/5] Remove non standard function from Path class --- tests/functional/test_uninstall.py | 3 ++- tests/lib/path.py | 4 ---- 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/tests/functional/test_uninstall.py b/tests/functional/test_uninstall.py index 2e7330fee..7d1984246 100644 --- a/tests/functional/test_uninstall.py +++ b/tests/functional/test_uninstall.py @@ -473,7 +473,8 @@ def test_uninstall_with_symlink(script, data, tmpdir): st_mode = symlink_target.stat().st_mode distinfo_path = script.site_packages_path / 'simple.dist-0.1.dist-info' record_path = distinfo_path / 'RECORD' - record_path.append_text("symlink,,\n") + with open(record_path, "a") as f: + f.write("symlink,,\n") uninstall_result = script.pip('uninstall', 'simple.dist', '-y') assert symlink_source in uninstall_result.files_deleted assert symlink_target.stat().st_mode == st_mode diff --git a/tests/lib/path.py b/tests/lib/path.py index d54146868..b2676a2e1 100644 --- a/tests/lib/path.py +++ b/tests/lib/path.py @@ -203,10 +203,6 @@ class Path(_base): with open(self, "w") as fp: fp.write(content) - def append_text(self, content): - with open(self, "a") as fp: - fp.write(content) - def touch(self): with open(self, "a") as fp: path = fp.fileno() if os.utime in supports_fd else self