Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Build local directories in place #7882

Merged
merged 5 commits into from
Apr 13, 2020
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions docs/html/reference/pip_install.rst
Original file line number Diff line number Diff line change
Expand Up @@ -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
pradyunsg marked this conversation as resolved.
Show resolved Hide resolved
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`:
Expand Down
9 changes: 9 additions & 0 deletions news/7555.removal
Original file line number Diff line number Diff line change
@@ -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.
Comment on lines +1 to +9
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels a bit excessive. I think we should only summarize the change here, add a section to the documentation and link to that from here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pradyunsg would you have a suggestion about a suitable place for this in the documentation?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://pip.pypa.io/en/latest/reference/pip_install/#local-project-installs needs updating, so that would likely be the best place.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pradyunsg thanks for the documentation pointer, I updated that section. Regarding the changelog I thought the breaking change warranted a bit more verbosity than usual.

101 changes: 22 additions & 79 deletions src/pip/_internal/operations/prepare.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,15 +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,
rmtree,
)
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
Expand Down Expand Up @@ -133,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]
Expand Down Expand Up @@ -239,11 +180,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
Expand Down Expand Up @@ -415,21 +354,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():
pradyunsg marked this conversation as resolved.
Show resolved Hide resolved
# 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
Expand Down
32 changes: 0 additions & 32 deletions src/pip/_internal/utils/filesystem.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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]
Expand Down
4 changes: 3 additions & 1 deletion tests/functional/test_cli.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
26 changes: 0 additions & 26 deletions tests/functional/test_install.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
import glob
import os
import re
import shutil
import ssl
import sys
import textwrap
Expand All @@ -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 (
Expand Down Expand Up @@ -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'.
Expand Down
10 changes: 9 additions & 1 deletion tests/functional/test_uninstall.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down
48 changes: 0 additions & 48 deletions tests/lib/filesystem.py

This file was deleted.

Loading