diff --git a/src/pip/_internal/utils/temp_dir.py b/src/pip/_internal/utils/temp_dir.py index 29fd8097390..1f80cb48f56 100644 --- a/src/pip/_internal/utils/temp_dir.py +++ b/src/pip/_internal/utils/temp_dir.py @@ -128,12 +128,7 @@ def cleanup(self): try: rmtree(self._path) except OSError as e: - skip_error = ( - sys.platform == 'win32' and - e.errno == errno.EACCES and - getattr(e, 'winerror', 0) in {5, 32} - ) - if skip_error: + if sys.platform == 'win32' and e.errno == errno.EACCES: logger.warning( "%s (virus scanner may be holding it)." "cannot remove '%s'", diff --git a/tests/lib/filesystem.py b/tests/lib/filesystem.py index a33a703f951..983e95e0ae2 100644 --- a/tests/lib/filesystem.py +++ b/tests/lib/filesystem.py @@ -64,6 +64,7 @@ def external_file_opener(conn): # Open the file try: f = open(path, 'r') + # NOTE: action is for future use and may be unused if action == 'lock': lock_action(f) elif action == 'noread': diff --git a/tests/unit/test_utils_filesystem.py b/tests/unit/test_utils_filesystem.py index 3ef814dce4b..fc336e23add 100644 --- a/tests/unit/test_utils_filesystem.py +++ b/tests/unit/test_utils_filesystem.py @@ -1,13 +1,19 @@ import os import shutil +import psutil 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.filesystem import make_socket_file, make_unreadable_file, FileOpener from tests.lib.path import Path +@pytest.fixture() +def process(): + return psutil.Process() + + def make_file(path): Path(path).touch() @@ -27,6 +33,7 @@ def make_dir(path): skip_on_windows = pytest.mark.skipif("sys.platform == 'win32'") +skip_unless_windows = pytest.mark.skipif("sys.platform != 'win32'") @skip_on_windows @@ -59,3 +66,50 @@ def test_copy2_fixed_raises_appropriate_errors(create, error_type, tmpdir): copy2_fixed(src, dest) assert not dest.exists() + + +def test_file_opener_no_file(process): + # The FileOpener cleans up the subprocess even if the parent never sends the path + with FileOpener(): + pass + assert len(process.children()) == 0 + + +def test_file_opener_not_found(tmpdir, process): + # The FileOpener cleans up the subprocess even if the file cannot be opened + path = tmpdir.joinpath('foo.txt') + with FileOpener(path): + pass + assert len(process.children()) == 0 + + +def test_file_opener_normal(tmpdir, process): + # The FileOpener cleans up the subprocess when the file exists; also tests that path may be deferred + path = tmpdir.joinpath('foo.txt') + with open(path, 'w') as f: + f.write('Hello\n') + with FileOpener() as opener: + opener.send(path) + assert len(process.children()) == 0 + + +@skip_unless_windows +def test_file_opener_produces_unlink_error(tmpdir, process): + path = tmpdir.joinpath('foo.txt') + with open(path, 'w') as f: + f.write('Hello\n') + with FileOpener(path): + with pytest.raises(OSError): + os.unlink(path) + + +@skip_unless_windows +def test_file_opener_produces_rmtree_error(tmpdir, process): + subdir = tmpdir.joinpath('foo') + os.mkdir(subdir) + path = subdir.joinpath('bar.txt') + with open(path, 'w') as f: + f.write('Hello\n') + with FileOpener(path): + with pytest.raises(OSError): + shutil.rmtree(subdir) diff --git a/tests/unit/test_utils_temp_dir.py b/tests/unit/test_utils_temp_dir.py index 9b45a75b9e2..5967ff8606b 100644 --- a/tests/unit/test_utils_temp_dir.py +++ b/tests/unit/test_utils_temp_dir.py @@ -1,7 +1,10 @@ import itertools +import logging import os +import shutil import stat import tempfile +import time import pytest @@ -13,6 +16,8 @@ global_tempdir_manager, ) +from tests.lib.filesystem import FileOpener + # No need to test symlinked directories on Windows @pytest.mark.skipif("sys.platform == 'win32'") @@ -207,3 +212,33 @@ def test_tempdirectory_asserts_global_tempdir(monkeypatch): monkeypatch.setattr(temp_dir, "_tempdir_manager", None) with pytest.raises(AssertionError): TempDirectory(globally_managed=True) + + +@pytest.mark.skipif("sys.platform != 'win32'") +def test_temp_dir_warns_if_cannot_clean(caplog): + temp_dir = TempDirectory() + temp_dir_path = temp_dir.path + + stime = time.time() + + # Capture only at WARNING level and up + with caplog.at_level(logging.WARNING, 'pip._internal.utils.temp_dir'): + # open a file within the temporary directory in a sub-process, so it cannot be cleaned up + with FileOpener() as opener: + subpath = os.path.join(temp_dir_path, 'foo.txt') + with open(subpath, 'w') as f: + f.write('Cannot be deleted') + opener.send(subpath) + # with the file open, attempt to remove the log directory + temp_dir.cleanup() + + # assert that a WARNING was logged and that it contains a note about a potential virus scanner + assert 'WARNING' in caplog.text + assert 'virus scanner' in caplog.text + + # Assure that the cleanup was properly retried; this fix did not change retries + duration = time.time() - stime + assert duration >= 2.0 + + # Clean-up for failed TempDirectory cleanup + shutil.rmtree(temp_dir_path, ignore_errors=True) diff --git a/tools/requirements/tests.txt b/tools/requirements/tests.txt index 747ea321fb6..4e3fc9bd413 100644 --- a/tools/requirements/tests.txt +++ b/tools/requirements/tests.txt @@ -2,6 +2,8 @@ cryptography==2.8 freezegun mock pretend +# Below is to count sub-processes and assure child was cleaned up properly +psutil pytest==3.8.2 pytest-cov # Prevent installing 7.0 which has install_requires "pytest >= 3.10".