From 88e6e6bc5c877bfb15cce6f622dfbf9221c7b479 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20Bidoul?= Date: Sun, 22 Mar 2020 14:18:11 +0100 Subject: [PATCH 1/5] build in place --- docs/html/reference/pip_install.rst | 9 +++-- news/7555.removal | 9 +++++ src/pip/_internal/operations/prepare.py | 47 ++++++++++++------------- tests/unit/test_operations_prepare.py | 39 ++------------------ 4 files changed, 40 insertions(+), 64 deletions(-) create mode 100644 news/7555.removal diff --git a/docs/html/reference/pip_install.rst b/docs/html/reference/pip_install.rst index 08149a76773..6840e25fb1b 100644 --- a/docs/html/reference/pip_install.rst +++ b/docs/html/reference/pip_install.rst @@ -728,8 +728,13 @@ You can install local projects by specifying the project path to pip:: $ pip install path/to/SomeProject -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. +Until version 20.0, pip did copy the entire project directory to a temporary +location and installed from there. This approach was the cause of several +performance and correctness issues. As of version 20.1 pip installs from the +local project directory. Depending on the build backend used by the project, +this may generate secondary build artifacts in the project directory, such as +the ``.egg-info`` and ``build`` directories in the case of the setuptools +backend. .. _`editable-installs`: diff --git a/news/7555.removal b/news/7555.removal new file mode 100644 index 00000000000..2f5747f17d4 --- /dev/null +++ b/news/7555.removal @@ -0,0 +1,9 @@ +Building of local directories is now done in place. Previously pip did copy the +local directory tree to a temporary location before building. That approach had +a number of drawbacks, among which performance issues, as well as various +issues arising when the python project directory depends on its parent +directory (such as the presence of a VCS directory). The user visible effect of +this change is that secondary build artifacts, if any, may therefore be created +in the local directory, whereas before 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 build backend. diff --git a/src/pip/_internal/operations/prepare.py b/src/pip/_internal/operations/prepare.py index 1fcbb775ece..3817323bdb8 100644 --- a/src/pip/_internal/operations/prepare.py +++ b/src/pip/_internal/operations/prepare.py @@ -27,12 +27,7 @@ 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, - path_to_display, - rmtree, -) +from pip._internal.utils.misc import display_path, hide_url, path_to_display 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 @@ -239,11 +234,9 @@ def unpack_url( unpack_vcs_link(link, location) return None - # If it's a url to a local directory + # If it's a url to a local directory, we build in-place. + # There is nothing to be done here. if link.is_existing_dir(): - if os.path.isdir(location): - rmtree(location) - _copy_source_tree(link.file_path, location) return None # file urls @@ -415,21 +408,25 @@ def prepare_linked_requirement( with indent_log(): # Since source_dir is only set for editable requirements. assert req.source_dir is None - 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) - ) + 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) + ) # 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/tests/unit/test_operations_prepare.py b/tests/unit/test_operations_prepare.py index 0158eed5197..3df5429189a 100644 --- a/tests/unit/test_operations_prepare.py +++ b/tests/unit/test_operations_prepare.py @@ -214,40 +214,5 @@ 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) - 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) + # 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')) From 873f1e6332aa827c886266c7d858067c12521e80 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20Bidoul?= Date: Sun, 22 Mar 2020 14:26:58 +0100 Subject: [PATCH 2/5] remove _copy_source_tree and friends --- src/pip/_internal/operations/prepare.py | 56 +---------------- src/pip/_internal/utils/filesystem.py | 32 ---------- tests/functional/test_install.py | 26 -------- tests/lib/filesystem.py | 48 --------------- tests/unit/test_operations_prepare.py | 81 +------------------------ tests/unit/test_utils_filesystem.py | 61 ------------------- 6 files changed, 2 insertions(+), 302 deletions(-) delete mode 100644 tests/lib/filesystem.py delete mode 100644 tests/unit/test_utils_filesystem.py diff --git a/src/pip/_internal/operations/prepare.py b/src/pip/_internal/operations/prepare.py index 3817323bdb8..30d5e3a308c 100644 --- a/src/pip/_internal/operations/prepare.py +++ b/src/pip/_internal/operations/prepare.py @@ -24,10 +24,9 @@ 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, path_to_display +from pip._internal.utils.misc import display_path, hide_url 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 @@ -128,59 +127,6 @@ 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] diff --git a/src/pip/_internal/utils/filesystem.py b/src/pip/_internal/utils/filesystem.py index 36578fb6244..ab20a7f042e 100644 --- a/src/pip/_internal/utils/filesystem.py +++ b/src/pip/_internal/utils/filesystem.py @@ -2,8 +2,6 @@ import os import os.path import random -import shutil -import stat import sys from contextlib import contextmanager from tempfile import NamedTemporaryFile @@ -54,36 +52,6 @@ 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_install.py b/tests/functional/test_install.py index 50fa2e81d85..1562e6b1d4e 100644 --- a/tests/functional/test_install.py +++ b/tests/functional/test_install.py @@ -2,7 +2,6 @@ import glob import os import re -import shutil import ssl import sys import textwrap @@ -29,7 +28,6 @@ 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 ( @@ -576,30 +574,6 @@ 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/lib/filesystem.py b/tests/lib/filesystem.py deleted file mode 100644 index dc14b323e33..00000000000 --- a/tests/lib/filesystem.py +++ /dev/null @@ -1,48 +0,0 @@ -"""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 3df5429189a..bcfc8148669 100644 --- a/tests/unit/test_operations_prepare.py +++ b/tests/unit/test_operations_prepare.py @@ -10,18 +10,9 @@ 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 ( - _copy_source_tree, - _download_http_url, - unpack_url, -) +from pip._internal.operations.prepare import _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 @@ -101,76 +92,6 @@ 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): diff --git a/tests/unit/test_utils_filesystem.py b/tests/unit/test_utils_filesystem.py deleted file mode 100644 index 3ef814dce4b..00000000000 --- a/tests/unit/test_utils_filesystem.py +++ /dev/null @@ -1,61 +0,0 @@ -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() From ace0c1653121770a58ae3cccf9059227702edde5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20Bidoul?= Date: Sat, 28 Mar 2020 19:24:20 +0100 Subject: [PATCH 3/5] fix test_entrypoints_work test Since we now build in place, pip install calls setup.py in place which in turn creates fake_pkg.egg-info. Since in this test the package we are installing is in script.temp_path, we must tell script to expect temporary files to be created. --- tests/functional/test_cli.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/functional/test_cli.py b/tests/functional/test_cli.py index e416315125f..c401a7cf80f 100644 --- a/tests/functional/test_cli.py +++ b/tests/functional/test_cli.py @@ -27,7 +27,9 @@ def test_entrypoints_work(entrypoint, script): ) """.format(entrypoint))) - script.pip("install", "-vvv", str(fake_pkg)) + # 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) result = script.pip("-V") result2 = script.run("fake_pip", "-V", allow_stderr_warning=True) assert result.stdout == result2.stdout From 877e1ccc7776548918537c17630decb4f87f665f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20Bidoul?= Date: Sat, 28 Mar 2020 19:25:34 +0100 Subject: [PATCH 4/5] fix test_uninstall_console_scripts This particular test checks which files are created. Since we now build in place, expect the .egg-info directory to be created. --- tests/functional/test_uninstall.py | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/tests/functional/test_uninstall.py b/tests/functional/test_uninstall.py index ab41917c986..c030dbaf27b 100644 --- a/tests/functional/test_uninstall.py +++ b/tests/functional/test_uninstall.py @@ -271,7 +271,15 @@ 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']) + assert_all_changes( + result, + result2, + [ + script.venv / 'build', + 'cache', + script.scratch / 'discover' / 'discover.egg-info', + ] + ) def test_uninstall_console_scripts_uppercase_name(script): From 9e02a1a97f335d2aeca91047a3a7d987657a9347 Mon Sep 17 00:00:00 2001 From: Pradyun Gedam Date: Mon, 13 Apr 2020 04:55:57 +0530 Subject: [PATCH 5/5] Reword based on suggestion Co-Authored-By: Noah --- docs/html/reference/pip_install.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/html/reference/pip_install.rst b/docs/html/reference/pip_install.rst index 6840e25fb1b..56b16781e6b 100644 --- a/docs/html/reference/pip_install.rst +++ b/docs/html/reference/pip_install.rst @@ -728,7 +728,7 @@ You can install local projects by specifying the project path to pip:: $ pip install path/to/SomeProject -Until version 20.0, pip did copy the entire project directory to a temporary +Until version 20.0, pip copied the entire project directory to a temporary location and installed from there. This approach was the cause of several performance and correctness issues. As of version 20.1 pip installs from the local project directory. Depending on the build backend used by the project,