From c3226b072e22c9be4c56d6a27b0f2aefe3faaf66 Mon Sep 17 00:00:00 2001 From: winsonluk Date: Mon, 28 Dec 2020 20:41:04 -0500 Subject: [PATCH 1/9] Mitigate issues when site-packages are not owned by user. --- src/pip/_internal/commands/install.py | 7 ++++++- src/pip/_internal/req/req_uninstall.py | 15 ++++++++++++++- src/pip/_internal/utils/misc.py | 6 ++++++ 3 files changed, 26 insertions(+), 2 deletions(-) diff --git a/src/pip/_internal/commands/install.py b/src/pip/_internal/commands/install.py index cd97ecb6042..c16d835bb7f 100644 --- a/src/pip/_internal/commands/install.py +++ b/src/pip/_internal/commands/install.py @@ -721,11 +721,16 @@ def create_os_error_message(error, show_traceback, using_user_site): if error.errno == errno.EACCES: user_option_part = "Consider using the `--user` option" permissions_part = "Check the permissions" + chown_part = "Oftentimes packages are unnecessarily installed with " + "`sudo pip`, which sets the folder owner to root. In this case, " + "consider changing the owner to $USER: " + "`$ sudo chown -R $USER [package folder]`" if not using_user_site: parts.extend([ user_option_part, " or ", - permissions_part.lower(), + permissions_part.lower(), ". ", + chown_part ]) else: parts.append(permissions_part) diff --git a/src/pip/_internal/req/req_uninstall.py b/src/pip/_internal/req/req_uninstall.py index eea2508db86..95d5c790b1c 100644 --- a/src/pip/_internal/req/req_uninstall.py +++ b/src/pip/_internal/req/req_uninstall.py @@ -398,8 +398,21 @@ def remove(self, auto_confirm=False, verbose=False): for_rename = compress_for_rename(self.paths) for path in sorted(compact(for_rename)): - moved.stash(path) logger.debug('Removing file or directory %s', path) + try: + moved.stash(path) + except PermissionError as ex: + raise UninstallationError( + "{!r}\nFailed to uninstall {!r} because " + "this user lacks permissions to the package " + "folder. Consider using the `--user` option or " + "check the permissions.\nOftentimes packages " + "are unnecessarily installed with `sudo pip`, " + "which sets the folder owner to root. In this " + "case, consider changing the owner to $USER: " + "`$ sudo chown -R $USER [package folder]`" + .format(str(ex), dist_name_version) + ) for pth in self.pth.values(): pth.remove() diff --git a/src/pip/_internal/utils/misc.py b/src/pip/_internal/utils/misc.py index bd205ada5db..04cd3ea3490 100644 --- a/src/pip/_internal/utils/misc.py +++ b/src/pip/_internal/utils/misc.py @@ -327,6 +327,11 @@ def renames(old, new): """Like os.renames(), but handles renaming across devices.""" # Implementation borrowed from os.renames(). head, tail = os.path.split(new) + if not os.access(old, os.W_OK): + raise PermissionError( + 'Rename aborted. User does not have write access to ' + old + ) + if head and tail and not os.path.exists(head): os.makedirs(head) @@ -458,6 +463,7 @@ def user_test(d): return [d for d in working_set if local_test(d) and d.key not in skip and + d.key[0].isalnum() and editable_test(d) and editables_only_test(d) and user_test(d) From 153f48322dfb489a547a58677ff260ec3d9dc1d0 Mon Sep 17 00:00:00 2001 From: winsonluk Date: Mon, 28 Dec 2020 21:58:18 -0500 Subject: [PATCH 2/9] Add NEWS entries --- news/4692.bugfix.rst | 1 + news/4727.bugfix.rst | 1 + news/7269.bugfix.rst | 1 + news/7307.bugfix.rst | 1 + news/7450.bugfix.rst | 1 + news/7763.bugfix.rst | 1 + news/7940.bugfix.rst | 1 + news/9235.bugfix.rst | 1 + 8 files changed, 8 insertions(+) create mode 100644 news/4692.bugfix.rst create mode 100644 news/4727.bugfix.rst create mode 100644 news/7269.bugfix.rst create mode 100644 news/7307.bugfix.rst create mode 100644 news/7450.bugfix.rst create mode 100644 news/7763.bugfix.rst create mode 100644 news/7940.bugfix.rst create mode 100644 news/9235.bugfix.rst diff --git a/news/4692.bugfix.rst b/news/4692.bugfix.rst new file mode 100644 index 00000000000..0b5e6300680 --- /dev/null +++ b/news/4692.bugfix.rst @@ -0,0 +1 @@ +This change adds a warning for situations where pip is run with sudo. diff --git a/news/4727.bugfix.rst b/news/4727.bugfix.rst new file mode 100644 index 00000000000..0b5e6300680 --- /dev/null +++ b/news/4727.bugfix.rst @@ -0,0 +1 @@ +This change adds a warning for situations where pip is run with sudo. diff --git a/news/7269.bugfix.rst b/news/7269.bugfix.rst new file mode 100644 index 00000000000..84acf2ae721 --- /dev/null +++ b/news/7269.bugfix.rst @@ -0,0 +1 @@ +This change fixes pip freeze by suppressing warnings for invalid packages. diff --git a/news/7307.bugfix.rst b/news/7307.bugfix.rst new file mode 100644 index 00000000000..1dec29a24e4 --- /dev/null +++ b/news/7307.bugfix.rst @@ -0,0 +1 @@ +This change prevents the unintended creation of temporary package directories. diff --git a/news/7450.bugfix.rst b/news/7450.bugfix.rst new file mode 100644 index 00000000000..1dec29a24e4 --- /dev/null +++ b/news/7450.bugfix.rst @@ -0,0 +1 @@ +This change prevents the unintended creation of temporary package directories. diff --git a/news/7763.bugfix.rst b/news/7763.bugfix.rst new file mode 100644 index 00000000000..1dec29a24e4 --- /dev/null +++ b/news/7763.bugfix.rst @@ -0,0 +1 @@ +This change prevents the unintended creation of temporary package directories. diff --git a/news/7940.bugfix.rst b/news/7940.bugfix.rst new file mode 100644 index 00000000000..0b5e6300680 --- /dev/null +++ b/news/7940.bugfix.rst @@ -0,0 +1 @@ +This change adds a warning for situations where pip is run with sudo. diff --git a/news/9235.bugfix.rst b/news/9235.bugfix.rst new file mode 100644 index 00000000000..84acf2ae721 --- /dev/null +++ b/news/9235.bugfix.rst @@ -0,0 +1 @@ +This change fixes pip freeze by suppressing warnings for invalid packages. From 5893bfe5375ed147c6717336b95ad38643dc39fc Mon Sep 17 00:00:00 2001 From: winsonluk Date: Mon, 28 Dec 2020 23:11:23 -0500 Subject: [PATCH 3/9] Fix tests and chown message --- src/pip/_internal/commands/install.py | 8 ++++---- tests/unit/test_command_install.py | 20 ++++++++++++++------ 2 files changed, 18 insertions(+), 10 deletions(-) diff --git a/src/pip/_internal/commands/install.py b/src/pip/_internal/commands/install.py index c16d835bb7f..9d482663073 100644 --- a/src/pip/_internal/commands/install.py +++ b/src/pip/_internal/commands/install.py @@ -721,15 +721,15 @@ def create_os_error_message(error, show_traceback, using_user_site): if error.errno == errno.EACCES: user_option_part = "Consider using the `--user` option" permissions_part = "Check the permissions" - chown_part = "Oftentimes packages are unnecessarily installed with " - "`sudo pip`, which sets the folder owner to root. In this case, " - "consider changing the owner to $USER: " + chown_part = "Oftentimes packages are unnecessarily installed with "\ + "`sudo pip`, which sets the folder owner to root. In this case, "\ + "consider changing the owner to $USER: "\ "`$ sudo chown -R $USER [package folder]`" if not using_user_site: parts.extend([ user_option_part, " or ", - permissions_part.lower(), ". ", + permissions_part.lower(), ".\n", chown_part ]) else: diff --git a/tests/unit/test_command_install.py b/tests/unit/test_command_install.py index 66eb8ef3881..cefeba5b86c 100644 --- a/tests/unit/test_command_install.py +++ b/tests/unit/test_command_install.py @@ -89,9 +89,13 @@ def test_rejection_for_location_requirement_options(): # show_traceback = True, using_user_site = False (OSError("Illegal byte sequence"), True, False, 'Could not' ' install packages due to an OSError.\n'), - (OSError(errno.EACCES, "No file permission"), True, False, 'Could' - ' not install packages due to an OSError.\nConsider using the' - ' `--user` option or check the permissions.\n'), + (OSError(errno.EACCES, "No file permission"), True, False, + 'Could not install packages due to an OSError.' + '\nConsider using the `--user` option or check the permissions.' + '\nOftentimes packages are unnecessarily installed with `sudo pip`, ' + 'which sets the folder owner to root. In this ' + 'case, consider changing the owner to $USER: ' + '`$ sudo chown -R $USER [package folder]`.\n'), # show_traceback = False, using_user_site = True (OSError("Illegal byte sequence"), False, True, 'Could not' ' install packages due to an OSError: Illegal byte' @@ -104,9 +108,13 @@ def test_rejection_for_location_requirement_options(): ' install packages due to an OSError: Illegal byte sequence' '\n'), (OSError(errno.EACCES, "No file permission"), False, False, - 'Could not install packages due to an OSError: [Errno 13] No' - ' file permission\nConsider using the `--user` option or check the' - ' permissions.\n'), + 'Could not install packages due to an OSError: ' + '[Errno 13] No file permission\nConsider using the `--user` ' + 'option or check the permissions.' + '\nOftentimes packages are unnecessarily installed with `sudo pip`, ' + 'which sets the folder owner to root. In this ' + 'case, consider changing the owner to $USER: ' + '`$ sudo chown -R $USER [package folder]`.\n') ]) def test_create_os_error_message( error, show_traceback, using_user_site, expected From 208f9b9bd9707b65d1522cdd43cbcf371a88f254 Mon Sep 17 00:00:00 2001 From: winsonluk Date: Mon, 28 Dec 2020 23:22:12 -0500 Subject: [PATCH 4/9] Fix E122 continuation line --- src/pip/_internal/commands/install.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/pip/_internal/commands/install.py b/src/pip/_internal/commands/install.py index 9d482663073..a97a20340b1 100644 --- a/src/pip/_internal/commands/install.py +++ b/src/pip/_internal/commands/install.py @@ -721,10 +721,10 @@ def create_os_error_message(error, show_traceback, using_user_site): if error.errno == errno.EACCES: user_option_part = "Consider using the `--user` option" permissions_part = "Check the permissions" - chown_part = "Oftentimes packages are unnecessarily installed with "\ - "`sudo pip`, which sets the folder owner to root. In this case, "\ - "consider changing the owner to $USER: "\ - "`$ sudo chown -R $USER [package folder]`" + chown_part = ("Oftentimes packages are unnecessarily installed with " + "`sudo pip`, which sets the folder owner to root. In this case, " + "consider changing the owner to $USER: " + "`$ sudo chown -R $USER [package folder]`") if not using_user_site: parts.extend([ From 259ccac7cdb2dd0938444a100f5ec071766d4312 Mon Sep 17 00:00:00 2001 From: winsonluk Date: Mon, 28 Dec 2020 23:27:42 -0500 Subject: [PATCH 5/9] Fix E128 intentation --- src/pip/_internal/commands/install.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/pip/_internal/commands/install.py b/src/pip/_internal/commands/install.py index a97a20340b1..1453c27f422 100644 --- a/src/pip/_internal/commands/install.py +++ b/src/pip/_internal/commands/install.py @@ -722,9 +722,9 @@ def create_os_error_message(error, show_traceback, using_user_site): user_option_part = "Consider using the `--user` option" permissions_part = "Check the permissions" chown_part = ("Oftentimes packages are unnecessarily installed with " - "`sudo pip`, which sets the folder owner to root. In this case, " - "consider changing the owner to $USER: " - "`$ sudo chown -R $USER [package folder]`") + "`sudo pip`, which sets the folder owner to root. In this case, " + "consider changing the owner to $USER: " + "`$ sudo chown -R $USER [package folder]`") if not using_user_site: parts.extend([ From 5fc3d4b1bd82c4e1b498da3045c0919a4e31226a Mon Sep 17 00:00:00 2001 From: winsonluk Date: Mon, 28 Dec 2020 23:33:05 -0500 Subject: [PATCH 6/9] Actually fix E128 indentation --- src/pip/_internal/commands/install.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/pip/_internal/commands/install.py b/src/pip/_internal/commands/install.py index 1453c27f422..6d863941a4b 100644 --- a/src/pip/_internal/commands/install.py +++ b/src/pip/_internal/commands/install.py @@ -722,9 +722,9 @@ def create_os_error_message(error, show_traceback, using_user_site): user_option_part = "Consider using the `--user` option" permissions_part = "Check the permissions" chown_part = ("Oftentimes packages are unnecessarily installed with " - "`sudo pip`, which sets the folder owner to root. In this case, " - "consider changing the owner to $USER: " - "`$ sudo chown -R $USER [package folder]`") + "`sudo pip`, which sets the folder owner to root. " + "In this case, consider changing the owner to $USER: " + "`$ sudo chown -R $USER [package folder]`") if not using_user_site: parts.extend([ From 0b0da94b03ea7d22a949644e946ec0268c9d558c Mon Sep 17 00:00:00 2001 From: winsonluk Date: Mon, 28 Dec 2020 23:56:40 -0500 Subject: [PATCH 7/9] Ignore leading dash --- src/pip/_internal/utils/misc.py | 2 +- tests/functional/test_freeze.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/pip/_internal/utils/misc.py b/src/pip/_internal/utils/misc.py index 04cd3ea3490..a03f035245c 100644 --- a/src/pip/_internal/utils/misc.py +++ b/src/pip/_internal/utils/misc.py @@ -463,7 +463,7 @@ def user_test(d): return [d for d in working_set if local_test(d) and d.key not in skip and - d.key[0].isalnum() and + '-' not in d.key[0] and editable_test(d) and editables_only_test(d) and user_test(d) diff --git a/tests/functional/test_freeze.py b/tests/functional/test_freeze.py index 5d3d4968619..cf7f6b79484 100644 --- a/tests/functional/test_freeze.py +++ b/tests/functional/test_freeze.py @@ -123,7 +123,7 @@ def fake_install(pkgname, dest): valid_pkgnames = ('middle-dash', 'middle_underscore', 'middle.dot') invalid_pkgnames = ( - '-leadingdash', '_leadingunderscore', '.leadingdot', + '_leadingunderscore', '.leadingdot', 'trailingdash-', 'trailingunderscore_', 'trailingdot.' ) for pkgname in valid_pkgnames + invalid_pkgnames: From 96d136144a3d18ed79c1e5d1bbf309666e4188ad Mon Sep 17 00:00:00 2001 From: winsonluk Date: Tue, 29 Dec 2020 00:18:14 -0500 Subject: [PATCH 8/9] Test ignored pkgnames --- tests/functional/test_freeze.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/tests/functional/test_freeze.py b/tests/functional/test_freeze.py index cf7f6b79484..aa0b373bc82 100644 --- a/tests/functional/test_freeze.py +++ b/tests/functional/test_freeze.py @@ -123,9 +123,12 @@ def fake_install(pkgname, dest): valid_pkgnames = ('middle-dash', 'middle_underscore', 'middle.dot') invalid_pkgnames = ( - '_leadingunderscore', '.leadingdot', 'trailingdash-', 'trailingunderscore_', 'trailingdot.' ) + ignored_pkgnames = ) + '-leadingdash', '_leadingunderscore', '.leadingdot' + ) + for pkgname in valid_pkgnames + invalid_pkgnames: fake_install(pkgname, script.site_packages_path) result = script.pip('freeze', expect_stderr=True) @@ -142,6 +145,9 @@ def fake_install(pkgname, dest): 'distribution {}...'.format(dist_repr) ) _check_output(result.stderr, expected) + for pkgname in ignored_pkgnames: + assert(pkgname not in result.stdout) + assert(pkgname not in result.stderr) # Also check that the parse error details occur at least once. # We only need to find one occurrence to know that exception details From 47837fd4e29f125bd7198fd7fa00200a61f8fef5 Mon Sep 17 00:00:00 2001 From: winsonluk Date: Tue, 29 Dec 2020 00:21:12 -0500 Subject: [PATCH 9/9] Fix pkgname test --- tests/functional/test_freeze.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/functional/test_freeze.py b/tests/functional/test_freeze.py index aa0b373bc82..2f27722374c 100644 --- a/tests/functional/test_freeze.py +++ b/tests/functional/test_freeze.py @@ -125,7 +125,7 @@ def fake_install(pkgname, dest): invalid_pkgnames = ( 'trailingdash-', 'trailingunderscore_', 'trailingdot.' ) - ignored_pkgnames = ) + ignored_pkgnames = ( '-leadingdash', '_leadingunderscore', '.leadingdot' )