From 19cccd8a61c73e29314cc55a5dd08004903a24fd Mon Sep 17 00:00:00 2001 From: Chris Hunt Date: Sat, 27 Jul 2019 21:08:39 -0400 Subject: [PATCH 1/6] Failing test for local install with socket file. --- tests/functional/test_install.py | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/tests/functional/test_install.py b/tests/functional/test_install.py index 6e1e3dd400b..e52407d59d7 100644 --- a/tests/functional/test_install.py +++ b/tests/functional/test_install.py @@ -1,6 +1,8 @@ import distutils import glob import os +import shutil +import socket import sys import textwrap from os.path import curdir, join, pardir @@ -488,6 +490,26 @@ def test_install_from_local_directory_with_symlinks_to_directories( assert egg_info_folder in result.files_created, str(result) +@pytest.mark.skipif("sys.platform == 'win32'") +def test_install_from_local_directory_with_socket_file(script, data, tmpdir): + """ + Test installing from a local directory containing a socket file. + """ + egg_info_file = ( + script.site_packages / 'FSPkg-0.1.dev0-py%s.egg-info' % pyversion + ) + package_folder = script.site_packages / 'fspkg' + to_copy = data.packages.joinpath("FSPkg") + to_install = tmpdir.joinpath("src") + shutil.copytree(to_copy, to_install) + sock = socket.socket(socket.AF_UNIX) + sock.bind(os.path.join(to_install, "example")) + result = script.pip("install", to_install, expect_error=False) + + assert package_folder in result.files_created, str(result.stdout) + assert egg_info_file in result.files_created, str(result) + + def test_install_from_local_directory_with_no_setup_py(script, data): """ Test installing from a local directory with no 'setup.py'. From 761c8fa8d34acb227be4cd94cde52972495fb33a Mon Sep 17 00:00:00 2001 From: Chris Hunt Date: Sun, 28 Jul 2019 11:12:33 -0400 Subject: [PATCH 2/6] Ignore errors copying socket files for source installs. --- news/5306.bugfix | 1 + src/pip/_internal/download.py | 48 +++++++++++----- src/pip/_internal/utils/filesystem.py | 6 ++ tests/functional/test_install.py | 13 +++-- tests/lib/filesystem.py | 47 ++++++++++++++++ tests/unit/test_download.py | 80 +++++++++++++++++++++++++++ tests/unit/test_utils_filesystem.py | 41 ++++++++++++++ tox.ini | 2 +- 8 files changed, 216 insertions(+), 22 deletions(-) create mode 100644 news/5306.bugfix create mode 100644 tests/lib/filesystem.py create mode 100644 tests/unit/test_utils_filesystem.py diff --git a/news/5306.bugfix b/news/5306.bugfix new file mode 100644 index 00000000000..fe96916d453 --- /dev/null +++ b/news/5306.bugfix @@ -0,0 +1 @@ +Ignore errors copying socket files for local source installs. diff --git a/src/pip/_internal/download.py b/src/pip/_internal/download.py index 0c6492a8d78..78db4709200 100644 --- a/src/pip/_internal/download.py +++ b/src/pip/_internal/download.py @@ -33,7 +33,7 @@ # Import ssl from compat so the initial import occurs in only one place. from pip._internal.utils.compat import HAS_TLS, ssl from pip._internal.utils.encoding import auto_decode -from pip._internal.utils.filesystem import check_path_owner +from pip._internal.utils.filesystem import check_path_owner, is_socket from pip._internal.utils.glibc import libc_ver from pip._internal.utils.marker_files import write_delete_marker_file from pip._internal.utils.misc import ( @@ -63,7 +63,7 @@ if MYPY_CHECK_RUNNING: from typing import ( - Optional, Tuple, Dict, IO, Text, Union + Dict, IO, List, Optional, Text, Tuple, Union ) from optparse import Values from pip._internal.models.link import Link @@ -944,6 +944,36 @@ def unpack_http_url( os.unlink(from_path) +def _copy_source_tree(source, target): + # type: (str, str) -> None + def ignore(d, names): + # Pulling in those directories can potentially be very slow, + # exclude the following directories if they appear in the top + # level dir (and only it). + # See discussion at https://github.com/pypa/pip/pull/6770 + return ['.tox', '.nox'] if d == source else [] + + try: + shutil.copytree(source, target, ignore=ignore, symlinks=True) + except shutil.Error as e: + errors = e.args[0] # type: List[Tuple[str, str, shutil.Error]] + # Users may have locally-created socket files in the source + # directory. Copying socket files is not supported, so we skip errors + # related to them, for convenience. + + # Copy list to avoid mutation while iterating. + errors_copy = errors[:] + for i, err in reversed(list(enumerate(errors_copy))): + src, _dest, _error = err + if is_socket(src): + # Remove errors related to sockets to prevent distractions if + # we end up re-raising. + errors.pop(i) + + if errors: + raise + + def unpack_file_url( link, # type: Link location, # type: str @@ -959,21 +989,9 @@ def unpack_file_url( link_path = url_to_path(link.url_without_fragment) # If it's a url to a local directory if is_dir_url(link): - - def ignore(d, names): - # Pulling in those directories can potentially be very slow, - # exclude the following directories if they appear in the top - # level dir (and only it). - # See discussion at https://github.com/pypa/pip/pull/6770 - return ['.tox', '.nox'] if d == link_path else [] - if os.path.isdir(location): rmtree(location) - shutil.copytree(link_path, - location, - symlinks=True, - ignore=ignore) - + _copy_source_tree(link_path, location) if download_dir: logger.info('Link is a directory, ignoring download_dir') return diff --git a/src/pip/_internal/utils/filesystem.py b/src/pip/_internal/utils/filesystem.py index 1e6b0338581..1841de92418 100644 --- a/src/pip/_internal/utils/filesystem.py +++ b/src/pip/_internal/utils/filesystem.py @@ -1,5 +1,6 @@ import os import os.path +import stat from pip._internal.utils.compat import get_path_uid @@ -28,3 +29,8 @@ def check_path_owner(path): else: previous, path = path, os.path.dirname(path) return False # assume we don't own the path + + +def is_socket(path): + # type: (str) -> bool + return stat.S_ISSOCK(os.lstat(path).st_mode) diff --git a/tests/functional/test_install.py b/tests/functional/test_install.py index e52407d59d7..40750526c3d 100644 --- a/tests/functional/test_install.py +++ b/tests/functional/test_install.py @@ -2,7 +2,6 @@ import glob import os import shutil -import socket import sys import textwrap from os.path import curdir, join, pardir @@ -25,6 +24,7 @@ pyversion_tuple, requirements_file, ) +from tests.lib.filesystem import make_socket_file from tests.lib.local_repos import local_checkout from tests.lib.path import Path @@ -496,16 +496,17 @@ def test_install_from_local_directory_with_socket_file(script, data, tmpdir): Test installing from a local directory containing a socket file. """ egg_info_file = ( - script.site_packages / 'FSPkg-0.1.dev0-py%s.egg-info' % pyversion + script.site_packages / "FSPkg-0.1.dev0-py%s.egg-info" % pyversion ) - package_folder = script.site_packages / 'fspkg' + package_folder = script.site_packages / "fspkg" to_copy = data.packages.joinpath("FSPkg") to_install = tmpdir.joinpath("src") + shutil.copytree(to_copy, to_install) - sock = socket.socket(socket.AF_UNIX) - sock.bind(os.path.join(to_install, "example")) - result = script.pip("install", to_install, expect_error=False) + # Socket file, should be ignored. + make_socket_file(os.path.join(to_install, "example")) + result = script.pip("install", to_install, expect_error=False) assert package_folder in result.files_created, str(result.stdout) assert egg_info_file in result.files_created, str(result) diff --git a/tests/lib/filesystem.py b/tests/lib/filesystem.py new file mode 100644 index 00000000000..62f7b07633a --- /dev/null +++ b/tests/lib/filesystem.py @@ -0,0 +1,47 @@ +"""Helpers for filesystem-dependent tests. +""" +import os +import socket +import subprocess +import sys +from functools import partial +from itertools import chain + +from .path import Path + + +def make_socket_file(path): + # Socket paths are limited to 108 characters (sometimes less) so we + # chdir before creating it and use a relative path name. + cwd = os.getcwd() + os.chdir(os.path.dirname(path)) + try: + sock = socket.socket(socket.AF_UNIX) + sock.bind(os.path.basename(path)) + finally: + os.chdir(cwd) + + +def make_unreadable_file(path): + Path(path).touch() + os.chmod(path, 0o000) + if sys.platform == "win32": + username = os.environ["USERNAME"] + # Remove "Read Data/List Directory" permission for current user, but + # leave everything else. + args = ["icacls", path, "/deny", username + ":(RD)"] + subprocess.check_call(args) + + +def get_filelist(base): + def join(dirpath, dirnames, filenames): + relative_dirpath = os.path.relpath(dirpath, base) + join_dirpath = partial(os.path.join, relative_dirpath) + return chain( + (join_dirpath(p) for p in dirnames), + (join_dirpath(p) for p in filenames), + ) + + return set(chain.from_iterable( + join(*dirinfo) for dirinfo in os.walk(base) + )) diff --git a/tests/unit/test_download.py b/tests/unit/test_download.py index f7595f52bcd..52b5864b1ac 100644 --- a/tests/unit/test_download.py +++ b/tests/unit/test_download.py @@ -1,6 +1,7 @@ import functools import hashlib import os +import shutil import sys from io import BytesIO from shutil import copy, rmtree @@ -15,6 +16,7 @@ MultiDomainBasicAuth, PipSession, SafeFileCache, + _copy_source_tree, _download_http_url, _get_url_scheme, parse_content_disposition, @@ -28,6 +30,12 @@ from pip._internal.utils.hashes import Hashes from pip._internal.utils.misc import path_to_url from tests.lib import create_file +from tests.lib.filesystem import ( + get_filelist, + make_socket_file, + make_unreadable_file, +) +from tests.lib.path import Path @pytest.fixture(scope="function") @@ -334,6 +342,78 @@ def test_url_to_path_path_to_url_symmetry_win(): assert url_to_path(path_to_url(unc_path)) == unc_path +@pytest.fixture +def clean_project(tmpdir_factory, data): + tmpdir = Path(str(tmpdir_factory.mktemp("clean_project"))) + new_project_dir = tmpdir.joinpath("FSPkg") + path = data.packages.joinpath("FSPkg") + shutil.copytree(path, new_project_dir) + return new_project_dir + + +def test_copy_source_tree(clean_project, tmpdir): + target = tmpdir.joinpath("target") + expected_files = get_filelist(clean_project) + assert len(expected_files) == 3 + + _copy_source_tree(clean_project, target) + + copied_files = get_filelist(target) + assert expected_files == copied_files + + +@pytest.mark.skipif("sys.platform == 'win32'") +def test_copy_source_tree_with_socket(clean_project, tmpdir): + target = tmpdir.joinpath("target") + expected_files = get_filelist(clean_project) + make_socket_file(clean_project.joinpath("aaa")) + + _copy_source_tree(clean_project, target) + + copied_files = get_filelist(target) + assert expected_files == copied_files + + +@pytest.mark.skipif("sys.platform == 'win32'") +def test_copy_source_tree_with_socket_fails_with_no_socket_error( + clean_project, tmpdir +): + target = tmpdir.joinpath("target") + expected_files = get_filelist(clean_project) + make_socket_file(clean_project.joinpath("aaa")) + unreadable_file = clean_project.joinpath("bbb") + make_unreadable_file(unreadable_file) + + with pytest.raises(shutil.Error) as e: + _copy_source_tree(clean_project, target) + + errored_files = [err[0] for err in e.value.args[0]] + assert len(errored_files) == 1 + assert unreadable_file in errored_files + + copied_files = get_filelist(target) + # Even with errors, all files should have been copied. + assert expected_files == copied_files + + +def test_copy_source_tree_with_unreadable_dir_fails(clean_project, tmpdir): + target = tmpdir.joinpath("target") + expected_files = get_filelist(clean_project) + unreadable_file = clean_project.joinpath("bbb") + make_unreadable_file(unreadable_file) + + with pytest.raises(shutil.Error) as e: + _copy_source_tree(clean_project, target) + + errored_files = [err[0] for err in e.value.args[0]] + assert len(errored_files) == 1 + assert unreadable_file in errored_files + + copied_files = get_filelist(target) + # Even with errors, all files should have been copied. + assert expected_files == copied_files + + class Test_unpack_file_url(object): def prep(self, tmpdir, data): diff --git a/tests/unit/test_utils_filesystem.py b/tests/unit/test_utils_filesystem.py new file mode 100644 index 00000000000..533340f24d3 --- /dev/null +++ b/tests/unit/test_utils_filesystem.py @@ -0,0 +1,41 @@ +import os + +import pytest + +from pip._internal.utils.filesystem import is_socket + +from ..lib.filesystem import make_socket_file +from ..lib.path import Path + + +def make_file(path): + Path(path).touch() + + +def make_valid_symlink(path): + target = path + "1" + make_file(target) + os.symlink(target, path) + + +def make_broken_symlink(path): + os.symlink("foo", path) + + +def make_dir(path): + os.mkdir(path) + + +@pytest.mark.skipif("sys.platform == 'win32'") +@pytest.mark.parametrize("create,result", [ + (make_socket_file, True), + (make_file, False), + (make_valid_symlink, False), + (make_broken_symlink, False), + (make_dir, False), +]) +def test_is_socket(create, result, tmpdir): + target = tmpdir.joinpath("target") + create(target) + assert os.path.lexists(target) + assert is_socket(target) == result diff --git a/tox.ini b/tox.ini index fc1f3bd98ce..3dd128c9135 100644 --- a/tox.ini +++ b/tox.ini @@ -10,7 +10,7 @@ envlist = pip = python {toxinidir}/tools/tox_pip.py [testenv] -passenv = CI GIT_SSL_CAINFO +passenv = CI GIT_SSL_CAINFO USERNAME setenv = # This is required in order to get UTF-8 output inside of the subprocesses # that our tests use. From 3e043acb1a7be2cf72932bd97b653f15b7ce7580 Mon Sep 17 00:00:00 2001 From: Chris Hunt Date: Tue, 6 Aug 2019 21:28:55 -0400 Subject: [PATCH 3/6] Address review comments. --- src/pip/_internal/download.py | 50 +++++++++++++++++---------- src/pip/_internal/utils/filesystem.py | 30 ++++++++++++++++ tests/functional/test_install.py | 6 ++-- tests/unit/test_download.py | 15 +++++--- tests/unit/test_utils_filesystem.py | 30 +++++++++++++++- 5 files changed, 105 insertions(+), 26 deletions(-) diff --git a/src/pip/_internal/download.py b/src/pip/_internal/download.py index 78db4709200..fa6b61859f1 100644 --- a/src/pip/_internal/download.py +++ b/src/pip/_internal/download.py @@ -33,7 +33,7 @@ # Import ssl from compat so the initial import occurs in only one place. from pip._internal.utils.compat import HAS_TLS, ssl from pip._internal.utils.encoding import auto_decode -from pip._internal.utils.filesystem import check_path_owner, is_socket +from pip._internal.utils.filesystem import check_path_owner, copytree from pip._internal.utils.glibc import libc_ver from pip._internal.utils.marker_files import write_delete_marker_file from pip._internal.utils.misc import ( @@ -49,6 +49,7 @@ format_size, get_installed_version, netloc_has_port, + path_to_display, path_to_url, remove_auth_from_url, rmtree, @@ -63,7 +64,7 @@ if MYPY_CHECK_RUNNING: from typing import ( - Dict, IO, List, Optional, Text, Tuple, Union + Dict, IO, Optional, Text, Tuple, Union ) from optparse import Values from pip._internal.models.link import Link @@ -953,25 +954,36 @@ def ignore(d, names): # See discussion at https://github.com/pypa/pip/pull/6770 return ['.tox', '.nox'] if d == source else [] + def ignore_special_file_errors(error_details): + # Copying special files is not supported, so we skip errors related to + # them. This is a convenience to support users that may have tools + # creating e.g. socket files in their source directory. + src, dest, error = error_details + + if not isinstance(error, shutil.SpecialFileError): + # Then it is some other kind of error that we do want to report. + return True + + # SpecialFileError may be raised due to either the source or + # destination. If the destination was the cause then we would actually + # care, but since the destination directory is deleted prior to + # copy we ignore all of them assuming it is caused by the source. + logger.warning( + "Ignoring special file error '%s' encountered copying %s to %s.", + str(error), + path_to_display(src), + path_to_display(dest), + ) + try: - shutil.copytree(source, target, ignore=ignore, symlinks=True) + copytree(source, target, ignore=ignore, symlinks=True) except shutil.Error as e: - errors = e.args[0] # type: List[Tuple[str, str, shutil.Error]] - # Users may have locally-created socket files in the source - # directory. Copying socket files is not supported, so we skip errors - # related to them, for convenience. - - # Copy list to avoid mutation while iterating. - errors_copy = errors[:] - for i, err in reversed(list(enumerate(errors_copy))): - src, _dest, _error = err - if is_socket(src): - # Remove errors related to sockets to prevent distractions if - # we end up re-raising. - errors.pop(i) - - if errors: - raise + errors = e.args[0] + normal_file_errors = list( + filter(ignore_special_file_errors, errors) + ) + if normal_file_errors: + raise shutil.Error(normal_file_errors) def unpack_file_url( diff --git a/src/pip/_internal/utils/filesystem.py b/src/pip/_internal/utils/filesystem.py index 1841de92418..a9719701f49 100644 --- a/src/pip/_internal/utils/filesystem.py +++ b/src/pip/_internal/utils/filesystem.py @@ -1,5 +1,6 @@ import os import os.path +import shutil import stat from pip._internal.utils.compat import get_path_uid @@ -31,6 +32,35 @@ def check_path_owner(path): return False # assume we don't own the path +def copytree(*args, **kwargs): + """Wrap shutil.copytree() to map errors copying socket file to + SpecialFileError. + + See also https://bugs.python.org/issue37700. + """ + def to_correct_error(src, dest, error): + for f in [src, dest]: + try: + if is_socket(f): + new_error = shutil.SpecialFileError("`%s` is a socket" % f) + return (src, dest, new_error) + except OSError: + # An error has already occurred. Another error here is not + # a problem and we can ignore it. + pass + + return (src, dest, error) + + try: + shutil.copytree(*args, **kwargs) + except shutil.Error as e: + errors = e.args[0] + new_errors = [ + to_correct_error(src, dest, error) for src, dest, error in errors + ] + raise shutil.Error(new_errors) + + def is_socket(path): # type: (str) -> bool return stat.S_ISSOCK(os.lstat(path).st_mode) diff --git a/tests/functional/test_install.py b/tests/functional/test_install.py index 40750526c3d..963e94d56eb 100644 --- a/tests/functional/test_install.py +++ b/tests/functional/test_install.py @@ -504,11 +504,13 @@ def test_install_from_local_directory_with_socket_file(script, data, tmpdir): shutil.copytree(to_copy, to_install) # Socket file, should be ignored. - make_socket_file(os.path.join(to_install, "example")) + socket_file_path = os.path.join(to_install, "example") + make_socket_file(socket_file_path) - result = script.pip("install", to_install, expect_error=False) + result = script.pip("install", "--verbose", to_install, expect_error=False) assert package_folder in result.files_created, str(result.stdout) assert egg_info_file in result.files_created, str(result) + assert str(socket_file_path) in result.stderr def test_install_from_local_directory_with_no_setup_py(script, data): diff --git a/tests/unit/test_download.py b/tests/unit/test_download.py index 52b5864b1ac..ee8720dea58 100644 --- a/tests/unit/test_download.py +++ b/tests/unit/test_download.py @@ -363,16 +363,23 @@ def test_copy_source_tree(clean_project, tmpdir): @pytest.mark.skipif("sys.platform == 'win32'") -def test_copy_source_tree_with_socket(clean_project, tmpdir): +def test_copy_source_tree_with_socket(clean_project, tmpdir, caplog): target = tmpdir.joinpath("target") expected_files = get_filelist(clean_project) - make_socket_file(clean_project.joinpath("aaa")) + socket_path = str(clean_project.joinpath("aaa")) + make_socket_file(socket_path) _copy_source_tree(clean_project, target) copied_files = get_filelist(target) assert expected_files == copied_files + # Warning should have been logged. + assert len(caplog.records) == 1 + record = caplog.records[0] + assert record.levelname == 'WARNING' + assert socket_path in record.message + @pytest.mark.skipif("sys.platform == 'win32'") def test_copy_source_tree_with_socket_fails_with_no_socket_error( @@ -392,7 +399,7 @@ def test_copy_source_tree_with_socket_fails_with_no_socket_error( assert unreadable_file in errored_files copied_files = get_filelist(target) - # Even with errors, all files should have been copied. + # All files without errors should have been copied. assert expected_files == copied_files @@ -410,7 +417,7 @@ def test_copy_source_tree_with_unreadable_dir_fails(clean_project, tmpdir): assert unreadable_file in errored_files copied_files = get_filelist(target) - # Even with errors, all files should have been copied. + # All files without errors should have been copied. assert expected_files == copied_files diff --git a/tests/unit/test_utils_filesystem.py b/tests/unit/test_utils_filesystem.py index 533340f24d3..a3ca40326db 100644 --- a/tests/unit/test_utils_filesystem.py +++ b/tests/unit/test_utils_filesystem.py @@ -1,8 +1,9 @@ import os +import shutil import pytest -from pip._internal.utils.filesystem import is_socket +from pip._internal.utils.filesystem import copytree, is_socket from ..lib.filesystem import make_socket_file from ..lib.path import Path @@ -39,3 +40,30 @@ def test_is_socket(create, result, tmpdir): create(target) assert os.path.lexists(target) assert is_socket(target) == result + + +@pytest.mark.skipif("sys.platform == 'win32'") +def test_copytree_maps_socket_errors(tmpdir): + src_dir = tmpdir.joinpath("src") + make_dir(src_dir) + make_file(src_dir.joinpath("a")) + socket_src = src_dir.joinpath("b") + make_socket_file(socket_src) + make_file(src_dir.joinpath("c")) + + dest_dir = tmpdir.joinpath("dest") + socket_dest = dest_dir.joinpath("b") + + with pytest.raises(shutil.Error) as e: + copytree(src_dir, dest_dir) + + errors = e.value.args[0] + assert len(errors) == 1 + src, dest, error = errors[0] + assert src == str(socket_src) + assert dest == str(socket_dest) + assert isinstance(error, shutil.SpecialFileError) + + assert dest_dir.joinpath("a").exists() + assert not socket_dest.exists() + assert dest_dir.joinpath("c").exists() From 64fbe1ffa54ddb01100efc42e497d85bc002e3ac Mon Sep 17 00:00:00 2001 From: Chris Hunt Date: Wed, 7 Aug 2019 19:58:08 -0400 Subject: [PATCH 4/6] Python 3-only implementation. --- src/pip/_internal/download.py | 60 ++++++++++++++------------- src/pip/_internal/utils/filesystem.py | 29 ++++++------- tests/functional/test_install.py | 2 +- tests/unit/test_download.py | 4 +- tests/unit/test_utils_filesystem.py | 45 +++++++++----------- 5 files changed, 66 insertions(+), 74 deletions(-) diff --git a/src/pip/_internal/download.py b/src/pip/_internal/download.py index fa6b61859f1..29013df3cfa 100644 --- a/src/pip/_internal/download.py +++ b/src/pip/_internal/download.py @@ -21,6 +21,7 @@ from pip._vendor.requests.models import CONTENT_CHUNK_SIZE, Response from pip._vendor.requests.structures import CaseInsensitiveDict from pip._vendor.requests.utils import get_netrc_auth +from pip._vendor.six import PY2 # NOTE: XMLRPC Client is not annotated in typeshed as on 2017-07-17, which is # why we ignore the type on this import from pip._vendor.six.moves import xmlrpc_client # type: ignore @@ -33,7 +34,7 @@ # Import ssl from compat so the initial import occurs in only one place. from pip._internal.utils.compat import HAS_TLS, ssl from pip._internal.utils.encoding import auto_decode -from pip._internal.utils.filesystem import check_path_owner, copytree +from pip._internal.utils.filesystem import check_path_owner, copy2_fixed from pip._internal.utils.glibc import libc_ver from pip._internal.utils.marker_files import write_delete_marker_file from pip._internal.utils.misc import ( @@ -945,45 +946,48 @@ def unpack_http_url( os.unlink(from_path) -def _copy_source_tree(source, target): +def _copy2_ignoring_special_files(src, dest): # type: (str, str) -> None - def ignore(d, names): - # Pulling in those directories can potentially be very slow, - # exclude the following directories if they appear in the top - # level dir (and only it). - # See discussion at https://github.com/pypa/pip/pull/6770 - return ['.tox', '.nox'] if d == source else [] - - def ignore_special_file_errors(error_details): - # Copying special files is not supported, so we skip errors related to - # them. This is a convenience to support users that may have tools - # creating e.g. socket files in their source directory. - src, dest, error = error_details - - if not isinstance(error, shutil.SpecialFileError): - # Then it is some other kind of error that we do want to report. - return True - + """Copying special files is not supported, but as a convenience to users + we skip errors copying them. This supports tools that may create e.g. + socket files in the project source directory. + """ + try: + copy2_fixed(src, dest) + except shutil.SpecialFileError as e: # SpecialFileError may be raised due to either the source or # destination. If the destination was the cause then we would actually # care, but since the destination directory is deleted prior to # copy we ignore all of them assuming it is caused by the source. logger.warning( "Ignoring special file error '%s' encountered copying %s to %s.", - str(error), + str(e), path_to_display(src), path_to_display(dest), ) - try: - copytree(source, target, ignore=ignore, symlinks=True) - except shutil.Error as e: - errors = e.args[0] - normal_file_errors = list( - filter(ignore_special_file_errors, errors) + +def _copy_source_tree(source, target): + # type: (str, str) -> None + def ignore(d, names): + # Pulling in those directories can potentially be very slow, + # exclude the following directories if they appear in the top + # level dir (and only it). + # See discussion at https://github.com/pypa/pip/pull/6770 + return ['.tox', '.nox'] if d == source else [] + + if PY2: + # Python 2 does not support copy_function, so we let errors for + # uncopyable special files propagate. + shutil.copytree(source, target, ignore=ignore, symlinks=True) + else: + shutil.copytree( + source, + target, + copy_function=_copy2_ignoring_special_files, + ignore=ignore, + symlinks=True ) - if normal_file_errors: - raise shutil.Error(normal_file_errors) def unpack_file_url( diff --git a/src/pip/_internal/utils/filesystem.py b/src/pip/_internal/utils/filesystem.py index a9719701f49..c5233ebbc71 100644 --- a/src/pip/_internal/utils/filesystem.py +++ b/src/pip/_internal/utils/filesystem.py @@ -32,33 +32,28 @@ def check_path_owner(path): return False # assume we don't own the path -def copytree(*args, **kwargs): - """Wrap shutil.copytree() to map errors copying socket file to - SpecialFileError. +def copy2_fixed(src, dest): + # type: (str, str) -> None + """Wrap shutil.copy2() but map errors copying socket files to + SpecialFileError as expected. See also https://bugs.python.org/issue37700. """ - def to_correct_error(src, dest, error): + try: + shutil.copy2(src, dest) + except (OSError, IOError): for f in [src, dest]: try: - if is_socket(f): - new_error = shutil.SpecialFileError("`%s` is a socket" % f) - return (src, dest, new_error) + is_socket_file = is_socket(f) except OSError: # An error has already occurred. Another error here is not # a problem and we can ignore it. pass + else: + if is_socket_file: + raise shutil.SpecialFileError("`%s` is a socket" % f) - return (src, dest, error) - - try: - shutil.copytree(*args, **kwargs) - except shutil.Error as e: - errors = e.args[0] - new_errors = [ - to_correct_error(src, dest, error) for src, dest, error in errors - ] - raise shutil.Error(new_errors) + raise def is_socket(path): diff --git a/tests/functional/test_install.py b/tests/functional/test_install.py index 963e94d56eb..3151b77bb30 100644 --- a/tests/functional/test_install.py +++ b/tests/functional/test_install.py @@ -490,7 +490,7 @@ def test_install_from_local_directory_with_symlinks_to_directories( assert egg_info_folder in result.files_created, str(result) -@pytest.mark.skipif("sys.platform == 'win32'") +@pytest.mark.skipif("sys.platform == 'win32' or sys.version_info < (3,)") def test_install_from_local_directory_with_socket_file(script, data, tmpdir): """ Test installing from a local directory containing a socket file. diff --git a/tests/unit/test_download.py b/tests/unit/test_download.py index ee8720dea58..22eb9576d44 100644 --- a/tests/unit/test_download.py +++ b/tests/unit/test_download.py @@ -362,7 +362,7 @@ def test_copy_source_tree(clean_project, tmpdir): assert expected_files == copied_files -@pytest.mark.skipif("sys.platform == 'win32'") +@pytest.mark.skipif("sys.platform == 'win32' or sys.version_info < (3,)") def test_copy_source_tree_with_socket(clean_project, tmpdir, caplog): target = tmpdir.joinpath("target") expected_files = get_filelist(clean_project) @@ -381,7 +381,7 @@ def test_copy_source_tree_with_socket(clean_project, tmpdir, caplog): assert socket_path in record.message -@pytest.mark.skipif("sys.platform == 'win32'") +@pytest.mark.skipif("sys.platform == 'win32' or sys.version_info < (3,)") def test_copy_source_tree_with_socket_fails_with_no_socket_error( clean_project, tmpdir ): diff --git a/tests/unit/test_utils_filesystem.py b/tests/unit/test_utils_filesystem.py index a3ca40326db..b7da0de266d 100644 --- a/tests/unit/test_utils_filesystem.py +++ b/tests/unit/test_utils_filesystem.py @@ -3,9 +3,9 @@ import pytest -from pip._internal.utils.filesystem import copytree, is_socket +from pip._internal.utils.filesystem import copy2_fixed, is_socket -from ..lib.filesystem import make_socket_file +from ..lib.filesystem import make_socket_file, make_unreadable_file from ..lib.path import Path @@ -27,7 +27,10 @@ def make_dir(path): os.mkdir(path) -@pytest.mark.skipif("sys.platform == 'win32'") +skip_on_windows = pytest.mark.skipif("sys.platform == 'win32'") + + +@skip_on_windows @pytest.mark.parametrize("create,result", [ (make_socket_file, True), (make_file, False), @@ -42,28 +45,18 @@ def test_is_socket(create, result, tmpdir): assert is_socket(target) == result -@pytest.mark.skipif("sys.platform == 'win32'") -def test_copytree_maps_socket_errors(tmpdir): - src_dir = tmpdir.joinpath("src") - make_dir(src_dir) - make_file(src_dir.joinpath("a")) - socket_src = src_dir.joinpath("b") - make_socket_file(socket_src) - make_file(src_dir.joinpath("c")) - - dest_dir = tmpdir.joinpath("dest") - socket_dest = dest_dir.joinpath("b") - - with pytest.raises(shutil.Error) as e: - copytree(src_dir, dest_dir) +@pytest.mark.parametrize("create,error_type", [ + pytest.param( + make_socket_file, shutil.SpecialFileError, marks=skip_on_windows + ), + (make_unreadable_file, OSError), +]) +def test_copy2_fixed_raises_appropriate_errors(create, error_type, tmpdir): + src = tmpdir.joinpath("src") + create(src) + dest = tmpdir.joinpath("dest") - errors = e.value.args[0] - assert len(errors) == 1 - src, dest, error = errors[0] - assert src == str(socket_src) - assert dest == str(socket_dest) - assert isinstance(error, shutil.SpecialFileError) + with pytest.raises(error_type): + copy2_fixed(src, dest) - assert dest_dir.joinpath("a").exists() - assert not socket_dest.exists() - assert dest_dir.joinpath("c").exists() + assert not dest.exists() From ce6f64a94637ca4dfbf4280b860234a3f55fc1ad Mon Sep 17 00:00:00 2001 From: Chris Hunt Date: Fri, 9 Aug 2019 19:33:51 -0400 Subject: [PATCH 5/6] Construct copytree keyword arguments instead of using separate calls. --- src/pip/_internal/download.py | 45 +++++++++++++++++++++++++---------- 1 file changed, 32 insertions(+), 13 deletions(-) diff --git a/src/pip/_internal/download.py b/src/pip/_internal/download.py index 29013df3cfa..abff7bbc98e 100644 --- a/src/pip/_internal/download.py +++ b/src/pip/_internal/download.py @@ -65,15 +65,38 @@ if MYPY_CHECK_RUNNING: from typing import ( - Dict, IO, Optional, Text, Tuple, Union + Callable, Dict, List, IO, Optional, Text, Tuple, Union ) from optparse import Values + + from mypy_extensions import TypedDict + from pip._internal.models.link import Link from pip._internal.utils.hashes import Hashes from pip._internal.vcs.versioncontrol import AuthInfo, VersionControl Credentials = Tuple[str, str, str] + if PY2: + CopytreeKwargs = TypedDict( + 'CopytreeKwargs', + { + 'ignore': Callable[[str, List[str]], List[str]], + 'symlinks': bool, + }, + total=False, + ) + else: + CopytreeKwargs = TypedDict( + 'CopytreeKwargs', { + 'copy_function': Callable[[str, str], None], + 'ignore': Callable[[str, List[str]], List[str]], + 'ignore_dangling_symlinks': bool, + 'symlinks': bool, + }, + total=False, + ) + __all__ = ['get_file_content', 'is_url', 'url_to_path', 'path_to_url', @@ -976,18 +999,14 @@ def ignore(d, names): # See discussion at https://github.com/pypa/pip/pull/6770 return ['.tox', '.nox'] if d == source else [] - if PY2: - # Python 2 does not support copy_function, so we let errors for - # uncopyable special files propagate. - shutil.copytree(source, target, ignore=ignore, symlinks=True) - else: - shutil.copytree( - source, - target, - copy_function=_copy2_ignoring_special_files, - ignore=ignore, - symlinks=True - ) + kwargs = dict(ignore=ignore, symlinks=True) # type: CopytreeKwargs + + if not PY2: + # Python 2 does not support copy_function, so we only ignore + # errors on special file copy in Python 3. + kwargs['copy_function'] = _copy2_ignoring_special_files + + shutil.copytree(source, target, **kwargs) def unpack_file_url( From a7dbd12d7bfa02eadb772515c552ac9e3c24c9a0 Mon Sep 17 00:00:00 2001 From: Chris Hunt Date: Sun, 11 Aug 2019 15:09:33 -0400 Subject: [PATCH 6/6] Relative import to absolute import, and update news --- news/5306.bugfix | 2 +- src/pip/_internal/download.py | 3 ++- tests/lib/filesystem.py | 1 + tests/unit/test_utils_filesystem.py | 5 ++--- tox.ini | 1 + 5 files changed, 7 insertions(+), 5 deletions(-) diff --git a/news/5306.bugfix b/news/5306.bugfix index fe96916d453..bf040a95fcf 100644 --- a/news/5306.bugfix +++ b/news/5306.bugfix @@ -1 +1 @@ -Ignore errors copying socket files for local source installs. +Ignore errors copying socket files for local source installs (in Python 3). diff --git a/src/pip/_internal/download.py b/src/pip/_internal/download.py index abff7bbc98e..81d443ddf49 100644 --- a/src/pip/_internal/download.py +++ b/src/pip/_internal/download.py @@ -88,7 +88,8 @@ ) else: CopytreeKwargs = TypedDict( - 'CopytreeKwargs', { + 'CopytreeKwargs', + { 'copy_function': Callable[[str, str], None], 'ignore': Callable[[str, List[str]], List[str]], 'ignore_dangling_symlinks': bool, diff --git a/tests/lib/filesystem.py b/tests/lib/filesystem.py index 62f7b07633a..dc14b323e33 100644 --- a/tests/lib/filesystem.py +++ b/tests/lib/filesystem.py @@ -26,6 +26,7 @@ def make_unreadable_file(path): Path(path).touch() os.chmod(path, 0o000) if sys.platform == "win32": + # Once we drop PY2 we can use `os.getlogin()` instead. username = os.environ["USERNAME"] # Remove "Read Data/List Directory" permission for current user, but # leave everything else. diff --git a/tests/unit/test_utils_filesystem.py b/tests/unit/test_utils_filesystem.py index b7da0de266d..3ef814dce4b 100644 --- a/tests/unit/test_utils_filesystem.py +++ b/tests/unit/test_utils_filesystem.py @@ -4,9 +4,8 @@ import pytest from pip._internal.utils.filesystem import copy2_fixed, is_socket - -from ..lib.filesystem import make_socket_file, make_unreadable_file -from ..lib.path import Path +from tests.lib.filesystem import make_socket_file, make_unreadable_file +from tests.lib.path import Path def make_file(path): diff --git a/tox.ini b/tox.ini index 3dd128c9135..608d491e3fe 100644 --- a/tox.ini +++ b/tox.ini @@ -10,6 +10,7 @@ envlist = pip = python {toxinidir}/tools/tox_pip.py [testenv] +# Remove USERNAME once we drop PY2. passenv = CI GIT_SSL_CAINFO USERNAME setenv = # This is required in order to get UTF-8 output inside of the subprocesses