Merge pull request #8221 from pradyunsg/revert-in-place-builds

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.
+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.
 
-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.
 
 .. _`editable-installs`:
 

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.

src/pip/_internal/operations/prepare.py

@@ -24,9 +24,15 @@ from pip._internal.exceptions import (
     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 @@ class RequirementPreparer(object):
         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

src/pip/_internal/utils/filesystem.py

@@ -3,6 +3,8 @@ import fnmatch
 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]

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

tests/functional/test_install.py

@@ -2,6 +2,7 @@ import distutils
 import glob
 import os
 import re
+import shutil
 import ssl
 import sys
 import textwrap
@@ -28,6 +29,7 @@ from tests.lib import (
     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'.

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):

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)
+    ))

tests/unit/test_operations_prepare.py

@@ -10,9 +10,18 @@ from pip._internal.exceptions import HashMismatch
 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 @@ class Test_unpack_url(object):
         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)

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()