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] 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 08149a767..6840e25fb 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 000000000..2f5747f17 --- /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 1fcbb775e..3817323bd 100644 --- a/src/pip/_internal/operations/prepare.py +++ b/src/pip/_internal/operations/prepare.py @@ -27,12 +27,7 @@ from pip._internal.exceptions import ( 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 @@ class RequirementPreparer(object): 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 0158eed51..3df542918 100644 --- a/tests/unit/test_operations_prepare.py +++ b/tests/unit/test_operations_prepare.py @@ -214,40 +214,5 @@ class Test_unpack_url(object): 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'))