diff --git a/docs/html/reference/pip_install.rst b/docs/html/reference/pip_install.rst index ec58899743a..294e969f3e1 100644 --- a/docs/html/reference/pip_install.rst +++ b/docs/html/reference/pip_install.rst @@ -732,18 +732,11 @@ You can install local projects by specifying the project path to pip:: $ pip install path/to/SomeProject -pip treats this directory like an unpacked source archive, and directly -attempts installation. - -Prior to pip 20.1, pip copied the entire project directory to a temporary -location and attempted installation from that directory. This approach was the -cause of several performance issues, as well as various issues arising when the -project directory depends on its parent directory (such as the presence of a -VCS directory). The main user visible effect of this change is that secondary -build artifacts, if any, would be created in the local directory, whereas -earlier they were created in a temporary copy of the directory and then -deleted. This notably includes the ``build`` and ``.egg-info`` directories in -the case of the setuptools backend. +During regular installation, pip will copy the entire project directory to a +temporary location and install from there. The exception is that pip will +exclude .tox and .nox directories present in the top level of the project from +being copied. + .. _`editable-installs`: diff --git a/news/7555.bugfix b/news/7555.bugfix new file mode 100644 index 00000000000..f762236e235 --- /dev/null +++ b/news/7555.bugfix @@ -0,0 +1,2 @@ +Revert building of local directories in place, restoring the pre-20.1 +behaviour of copying to a temporary directory. diff --git a/src/pip/_internal/operations/prepare.py b/src/pip/_internal/operations/prepare.py index 30d5e3a308c..1fcbb775ece 100644 --- a/src/pip/_internal/operations/prepare.py +++ b/src/pip/_internal/operations/prepare.py @@ -24,9 +24,15 @@ PreviousBuildDirError, VcsHashUnsupported, ) +from pip._internal.utils.filesystem import copy2_fixed from pip._internal.utils.hashes import MissingHashes from pip._internal.utils.logging import indent_log -from pip._internal.utils.misc import display_path, hide_url +from pip._internal.utils.misc import ( + display_path, + hide_url, + path_to_display, + rmtree, +) from pip._internal.utils.temp_dir import TempDirectory from pip._internal.utils.typing import MYPY_CHECK_RUNNING from pip._internal.utils.unpacking import unpack_file @@ -127,6 +133,59 @@ def get_http_url( return File(from_path, content_type) +def _copy2_ignoring_special_files(src, dest): + # type: (str, str) -> None + """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(e), + path_to_display(src), + path_to_display(dest), + ) + + +def _copy_source_tree(source, target): + # type: (str, str) -> None + target_abspath = os.path.abspath(target) + target_basename = os.path.basename(target_abspath) + target_dirname = os.path.dirname(target_abspath) + + def ignore(d, names): + # type: (str, List[str]) -> List[str] + skipped = [] # type: List[str] + if d == source: + # 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 + skipped += ['.tox', '.nox'] + if os.path.abspath(d) == target_dirname: + # Prevent an infinite recursion if the target is in source. + # This can happen when TMPDIR is set to ${PWD}/... + # and we copy PWD to TMPDIR. + skipped += [target_basename] + return skipped + + 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 get_file_url( link, # type: Link download_dir=None, # type: Optional[str] @@ -180,9 +239,11 @@ def unpack_url( unpack_vcs_link(link, location) return None - # If it's a url to a local directory, we build in-place. - # There is nothing to be done here. + # If it's a url to a local directory if link.is_existing_dir(): + if os.path.isdir(location): + rmtree(location) + _copy_source_tree(link.file_path, location) return None # file urls @@ -354,25 +415,21 @@ def prepare_linked_requirement( with indent_log(): # Since source_dir is only set for editable requirements. assert req.source_dir is None - if link.is_existing_dir(): - # Build local directories in place. - req.source_dir = link.file_path - else: - req.ensure_has_source_dir(self.build_dir, autodelete_unpacked) - # If a checkout exists, it's unwise to keep going. version - # inconsistencies are logged later, but do not fail the - # installation. - # FIXME: this won't upgrade when there's an existing - # package unpacked in `req.source_dir` - if os.path.exists(os.path.join(req.source_dir, 'setup.py')): - raise PreviousBuildDirError( - "pip can't proceed with requirements '{}' due to a" - " pre-existing build directory ({}). This is " - "likely due to a previous installation that failed" - ". pip is being responsible and not assuming it " - "can delete this. Please delete it and try again." - .format(req, req.source_dir) - ) + req.ensure_has_source_dir(self.build_dir, autodelete_unpacked) + # If a checkout exists, it's unwise to keep going. version + # inconsistencies are logged later, but do not fail the + # installation. + # FIXME: this won't upgrade when there's an existing + # package unpacked in `req.source_dir` + if os.path.exists(os.path.join(req.source_dir, 'setup.py')): + raise PreviousBuildDirError( + "pip can't proceed with requirements '{}' due to a" + " pre-existing build directory ({}). This is " + "likely due to a previous installation that failed" + ". pip is being responsible and not assuming it " + "can delete this. Please delete it and try again." + .format(req, req.source_dir) + ) # Now that we have the real link, we can tell what kind of # requirements we have and raise some more informative errors diff --git a/src/pip/_internal/utils/filesystem.py b/src/pip/_internal/utils/filesystem.py index 85010ac25a3..437a7fd1482 100644 --- a/src/pip/_internal/utils/filesystem.py +++ b/src/pip/_internal/utils/filesystem.py @@ -3,6 +3,8 @@ import os import os.path import random +import shutil +import stat import sys from contextlib import contextmanager from tempfile import NamedTemporaryFile @@ -54,6 +56,36 @@ def check_path_owner(path): return False # assume we don't own the path +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. + """ + try: + shutil.copy2(src, dest) + except (OSError, IOError): + for f in [src, dest]: + try: + 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( + "`{f}` is a socket".format(**locals())) + + raise + + +def is_socket(path): + # type: (str) -> bool + return stat.S_ISSOCK(os.lstat(path).st_mode) + + @contextmanager def adjacent_tmp_file(path, **kwargs): # type: (str, **Any) -> Iterator[NamedTemporaryFileResult] diff --git a/tests/functional/test_cli.py b/tests/functional/test_cli.py index c401a7cf80f..e416315125f 100644 --- a/tests/functional/test_cli.py +++ b/tests/functional/test_cli.py @@ -27,9 +27,7 @@ def test_entrypoints_work(entrypoint, script): ) """.format(entrypoint))) - # expect_temp=True, because pip install calls setup.py which - # in turn creates fake_pkg.egg-info. - script.pip("install", "-vvv", str(fake_pkg), expect_temp=True) + script.pip("install", "-vvv", str(fake_pkg)) result = script.pip("-V") result2 = script.run("fake_pip", "-V", allow_stderr_warning=True) assert result.stdout == result2.stdout diff --git a/tests/functional/test_install.py b/tests/functional/test_install.py index 571049aa447..2cb44269502 100644 --- a/tests/functional/test_install.py +++ b/tests/functional/test_install.py @@ -2,6 +2,7 @@ import glob import os import re +import shutil import ssl import sys import textwrap @@ -28,6 +29,7 @@ skip_if_python2, windows_workaround_7667, ) +from tests.lib.filesystem import make_socket_file from tests.lib.local_repos import local_checkout from tests.lib.path import Path from tests.lib.server import ( @@ -574,6 +576,30 @@ 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' 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. + """ + egg_info_file = ( + script.site_packages / + "FSPkg-0.1.dev0-py{pyversion}.egg-info".format(**globals()) + ) + package_folder = script.site_packages / "fspkg" + to_copy = data.packages.joinpath("FSPkg") + to_install = tmpdir.joinpath("src") + + shutil.copytree(to_copy, to_install) + # Socket file, should be ignored. + socket_file_path = os.path.join(to_install, "example") + make_socket_file(socket_file_path) + + result = script.pip("install", "--verbose", to_install) + 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): """ Test installing from a local directory with no 'setup.py'. diff --git a/tests/functional/test_uninstall.py b/tests/functional/test_uninstall.py index c030dbaf27b..ab41917c986 100644 --- a/tests/functional/test_uninstall.py +++ b/tests/functional/test_uninstall.py @@ -271,15 +271,7 @@ def test_uninstall_console_scripts(script): sorted(result.files_created.keys()) ) result2 = script.pip('uninstall', 'discover', '-y') - assert_all_changes( - result, - result2, - [ - script.venv / 'build', - 'cache', - script.scratch / 'discover' / 'discover.egg-info', - ] - ) + assert_all_changes(result, result2, [script.venv / 'build', 'cache']) def test_uninstall_console_scripts_uppercase_name(script): diff --git a/tests/lib/filesystem.py b/tests/lib/filesystem.py new file mode 100644 index 00000000000..dc14b323e33 --- /dev/null +++ b/tests/lib/filesystem.py @@ -0,0 +1,48 @@ +"""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": + # 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. + 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_operations_prepare.py b/tests/unit/test_operations_prepare.py index bcfc8148669..0158eed5197 100644 --- a/tests/unit/test_operations_prepare.py +++ b/tests/unit/test_operations_prepare.py @@ -10,9 +10,18 @@ from pip._internal.models.link import Link from pip._internal.network.download import Downloader from pip._internal.network.session import PipSession -from pip._internal.operations.prepare import _download_http_url, unpack_url +from pip._internal.operations.prepare import ( + _copy_source_tree, + _download_http_url, + unpack_url, +) from pip._internal.utils.hashes import Hashes from pip._internal.utils.urls import path_to_url +from tests.lib.filesystem import ( + get_filelist, + make_socket_file, + make_unreadable_file, +) from tests.lib.path import Path from tests.lib.requests_mocks import MockResponse @@ -92,6 +101,76 @@ def clean_project(tmpdir_factory, data): 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' 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) + 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' or sys.version_info < (3,)") +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) + # All files without errors 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) + # All files without errors should have been copied. + assert expected_files == copied_files + + class Test_unpack_url(object): def prep(self, tmpdir, data): @@ -135,5 +214,40 @@ def test_unpack_url_thats_a_dir(self, tmpdir, data): unpack_url(dist_url, self.build_dir, downloader=self.no_downloader, download_dir=self.download_dir) - # test that nothing was copied to build_dir since we build in place - assert not os.path.exists(os.path.join(self.build_dir, 'fspkg')) + assert os.path.isdir(os.path.join(self.build_dir, 'fspkg')) + + +@pytest.mark.parametrize('exclude_dir', [ + '.nox', + '.tox' +]) +def test_unpack_url_excludes_expected_dirs(tmpdir, exclude_dir): + src_dir = tmpdir / 'src' + dst_dir = tmpdir / 'dst' + src_included_file = src_dir.joinpath('file.txt') + src_excluded_dir = src_dir.joinpath(exclude_dir) + src_excluded_file = src_dir.joinpath(exclude_dir, 'file.txt') + src_included_dir = src_dir.joinpath('subdir', exclude_dir) + + # set up source directory + src_excluded_dir.mkdir(parents=True) + src_included_dir.mkdir(parents=True) + src_included_file.touch() + src_excluded_file.touch() + + dst_included_file = dst_dir.joinpath('file.txt') + dst_excluded_dir = dst_dir.joinpath(exclude_dir) + dst_excluded_file = dst_dir.joinpath(exclude_dir, 'file.txt') + dst_included_dir = dst_dir.joinpath('subdir', exclude_dir) + + src_link = Link(path_to_url(src_dir)) + unpack_url( + src_link, + dst_dir, + Mock(side_effect=AssertionError), + download_dir=None + ) + assert not os.path.isdir(dst_excluded_dir) + assert not os.path.isfile(dst_excluded_file) + assert os.path.isfile(dst_included_file) + assert os.path.isdir(dst_included_dir) diff --git a/tests/unit/test_utils_filesystem.py b/tests/unit/test_utils_filesystem.py new file mode 100644 index 00000000000..3ef814dce4b --- /dev/null +++ b/tests/unit/test_utils_filesystem.py @@ -0,0 +1,61 @@ +import os +import shutil + +import pytest + +from pip._internal.utils.filesystem import copy2_fixed, is_socket +from tests.lib.filesystem import make_socket_file, make_unreadable_file +from tests.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) + + +skip_on_windows = pytest.mark.skipif("sys.platform == 'win32'") + + +@skip_on_windows +@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 + + +@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") + + with pytest.raises(error_type): + copy2_fixed(src, dest) + + assert not dest.exists()