From 4083ab0d177f785f66f424d9a998e5515454ccd2 Mon Sep 17 00:00:00 2001 From: Cristian Libotean Date: Fri, 2 Sep 2022 14:34:37 +0300 Subject: [PATCH 01/15] Fix problem with deleting temporary folders on Windows (re-created patch #1032) --- src/poetry/core/utils/helpers.py | 24 +++++++++++++++++++++++- tests/utils/test_helpers.py | 27 +++++++++++++++++++++++++++ 2 files changed, 50 insertions(+), 1 deletion(-) diff --git a/src/poetry/core/utils/helpers.py b/src/poetry/core/utils/helpers.py index 3e03d0c28..852513535 100644 --- a/src/poetry/core/utils/helpers.py +++ b/src/poetry/core/utils/helpers.py @@ -4,6 +4,7 @@ import shutil import stat import tempfile +import time import unicodedata from contextlib import contextmanager @@ -32,7 +33,7 @@ def normalize_version(version: str) -> str: def temporary_directory(*args: Any, **kwargs: Any) -> Iterator[str]: name = tempfile.mkdtemp(*args, **kwargs) yield name - safe_rmtree(name) + robust_rmtree(name) def parse_requires(requires: str) -> list[str]: @@ -91,6 +92,27 @@ def safe_rmtree(path: str | Path) -> None: shutil.rmtree(path, onerror=_on_rm_error) +def robust_rmtree(path: str, max_timeout: float = 1) -> None: + """ + Robustly tries to delete paths. + Retries several times if an OSError occurs. + If the final attempt fails, the Exception is propagated + to the caller. + """ + timeout = 0.001 + while timeout < max_timeout: + try: + shutil.rmtree(path) + return # Only hits this on success + except OSError: + # Increase the timeout and try again + time.sleep(timeout) + timeout *= 2 + + # Final attempt, pass any Exceptions up to caller. + safe_rmtree(path) + + def readme_content_type(path: str | Path) -> str: suffix = Path(path).suffix if suffix == ".rst": diff --git a/tests/utils/test_helpers.py b/tests/utils/test_helpers.py index 74c89a5b8..c4e808a57 100644 --- a/tests/utils/test_helpers.py +++ b/tests/utils/test_helpers.py @@ -1,6 +1,7 @@ from __future__ import annotations import os +import tempfile from pathlib import Path from stat import S_IREAD @@ -11,6 +12,7 @@ from poetry.core.utils.helpers import normalize_version from poetry.core.utils.helpers import parse_requires from poetry.core.utils.helpers import readme_content_type +from poetry.core.utils.helpers import robust_rmtree from poetry.core.utils.helpers import temporary_directory @@ -178,3 +180,28 @@ def test_utils_helpers_readme_content_type( readme: str | Path, content_type: str ) -> None: assert readme_content_type(readme) == content_type + + +def test_robust_rmtree(mocker): + mocked_rmtree = mocker.patch("shutil.rmtree") + + # this should work after an initial exception + name = tempfile.mkdtemp() + mocked_rmtree.side_effect = [ + OSError( + "Couldn't delete file yet, waiting for references to clear", "mocked path" + ), + None, + ] + robust_rmtree(name) + + # this should give up after retrying multiple times + name = tempfile.mkdtemp() + mocked_rmtree.side_effect = OSError( + "Couldn't delete file yet, this error won't go away after first attempt" + ) + with pytest.raises(OSError): + robust_rmtree(name, max_timeout=0.04) + + # clear the side effect (breaks the tear-down otherwise) + mocked_rmtree.side_effect = None From 523fb72ad2aef65de92c041fbd8018ebba4ceac8 Mon Sep 17 00:00:00 2001 From: Cristian Libotean Date: Fri, 2 Sep 2022 14:41:43 +0300 Subject: [PATCH 02/15] Attempt to fix flake8 error --- tests/utils/test_helpers.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/tests/utils/test_helpers.py b/tests/utils/test_helpers.py index c4e808a57..c4bb474f7 100644 --- a/tests/utils/test_helpers.py +++ b/tests/utils/test_helpers.py @@ -5,6 +5,7 @@ from pathlib import Path from stat import S_IREAD +from typing import TYPE_CHECKING import pytest @@ -16,6 +17,10 @@ from poetry.core.utils.helpers import temporary_directory +if TYPE_CHECKING: + from pytest_mock import MockerFixture + + @pytest.mark.parametrize( "version,normalized_version", [ @@ -182,7 +187,7 @@ def test_utils_helpers_readme_content_type( assert readme_content_type(readme) == content_type -def test_robust_rmtree(mocker): +def test_robust_rmtree(mocker: MockerFixture): mocked_rmtree = mocker.patch("shutil.rmtree") # this should work after an initial exception From dfc8bf0901bc456f917b1318b22c63aefb2b7331 Mon Sep 17 00:00:00 2001 From: Cristian Libotean Date: Fri, 2 Sep 2022 14:49:57 +0300 Subject: [PATCH 03/15] Added return type to test function --- tests/utils/test_helpers.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/utils/test_helpers.py b/tests/utils/test_helpers.py index c4bb474f7..d8b8b3225 100644 --- a/tests/utils/test_helpers.py +++ b/tests/utils/test_helpers.py @@ -187,7 +187,7 @@ def test_utils_helpers_readme_content_type( assert readme_content_type(readme) == content_type -def test_robust_rmtree(mocker: MockerFixture): +def test_robust_rmtree(mocker: MockerFixture) -> None: mocked_rmtree = mocker.patch("shutil.rmtree") # this should work after an initial exception From 5707d059fea9881dc71c9531abbe3d77fa248e7b Mon Sep 17 00:00:00 2001 From: Cristian Libotean Date: Mon, 24 Apr 2023 17:34:04 +0300 Subject: [PATCH 04/15] Removed safe_rmtree() and included the logic in robust_rmtree instead. --- src/poetry/core/utils/helpers.py | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/src/poetry/core/utils/helpers.py b/src/poetry/core/utils/helpers.py index 852513535..49e50ac87 100644 --- a/src/poetry/core/utils/helpers.py +++ b/src/poetry/core/utils/helpers.py @@ -85,13 +85,6 @@ def _on_rm_error(func: Any, path: str | Path, exc_info: Any) -> None: func(path) -def safe_rmtree(path: str | Path) -> None: - if Path(path).is_symlink(): - return os.unlink(str(path)) - - shutil.rmtree(path, onerror=_on_rm_error) - - def robust_rmtree(path: str, max_timeout: float = 1) -> None: """ Robustly tries to delete paths. @@ -102,7 +95,12 @@ def robust_rmtree(path: str, max_timeout: float = 1) -> None: timeout = 0.001 while timeout < max_timeout: try: - shutil.rmtree(path) + # both os.unlink and shutil.rmtree can throw exceptions on Windows + # if the files are in use when called + if Path(path).is_symlink(): + os.unlink(str(path)) + else: + shutil.rmtree(path) return # Only hits this on success except OSError: # Increase the timeout and try again @@ -110,7 +108,7 @@ def robust_rmtree(path: str, max_timeout: float = 1) -> None: timeout *= 2 # Final attempt, pass any Exceptions up to caller. - safe_rmtree(path) + shutil.rmtree(path, onerror=_on_rm_error) def readme_content_type(path: str | Path) -> str: From 3464eb11ccd432bd08529b57c3521814b6c74714 Mon Sep 17 00:00:00 2001 From: Cristian Libotean Date: Mon, 24 Apr 2023 17:42:16 +0300 Subject: [PATCH 05/15] Fixed import errors in test_helpers.py after fixing merge conflicts. --- tests/utils/test_helpers.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/utils/test_helpers.py b/tests/utils/test_helpers.py index 55743fb61..127680cd6 100644 --- a/tests/utils/test_helpers.py +++ b/tests/utils/test_helpers.py @@ -1,13 +1,15 @@ from __future__ import annotations import os +import tempfile from pathlib import Path from stat import S_IREAD import pytest +from pytest_mock import MockerFixture -from poetry.core.utils.helpers import combine_unicode +from poetry.core.utils.helpers import combine_unicode, robust_rmtree from poetry.core.utils.helpers import parse_requires from poetry.core.utils.helpers import readme_content_type from poetry.core.utils.helpers import temporary_directory From 09a14a1557238b845893c9eb22e712336cde52b2 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Mon, 24 Apr 2023 14:42:37 +0000 Subject: [PATCH 06/15] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- tests/utils/test_helpers.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/utils/test_helpers.py b/tests/utils/test_helpers.py index 127680cd6..a8e6be8b3 100644 --- a/tests/utils/test_helpers.py +++ b/tests/utils/test_helpers.py @@ -7,11 +7,13 @@ from stat import S_IREAD import pytest + from pytest_mock import MockerFixture -from poetry.core.utils.helpers import combine_unicode, robust_rmtree +from poetry.core.utils.helpers import combine_unicode from poetry.core.utils.helpers import parse_requires from poetry.core.utils.helpers import readme_content_type +from poetry.core.utils.helpers import robust_rmtree from poetry.core.utils.helpers import temporary_directory From de39b183dc4cb12d6fb5b289882ffefb78b88ebd Mon Sep 17 00:00:00 2001 From: Cristian Libotean Date: Mon, 24 Apr 2023 17:45:51 +0300 Subject: [PATCH 07/15] Import MockerFixture under a type check block. --- tests/utils/test_helpers.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/utils/test_helpers.py b/tests/utils/test_helpers.py index a8e6be8b3..0e8d90449 100644 --- a/tests/utils/test_helpers.py +++ b/tests/utils/test_helpers.py @@ -2,18 +2,18 @@ import os import tempfile - from pathlib import Path from stat import S_IREAD +from typing import TYPE_CHECKING import pytest -from pytest_mock import MockerFixture +if TYPE_CHECKING: + from pytest_mock import MockerFixture -from poetry.core.utils.helpers import combine_unicode +from poetry.core.utils.helpers import combine_unicode, robust_rmtree from poetry.core.utils.helpers import parse_requires from poetry.core.utils.helpers import readme_content_type -from poetry.core.utils.helpers import robust_rmtree from poetry.core.utils.helpers import temporary_directory From 8334d723411a8a7e556895f3aa66a3c3d55de6d2 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Mon, 24 Apr 2023 14:47:42 +0000 Subject: [PATCH 08/15] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- tests/utils/test_helpers.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/tests/utils/test_helpers.py b/tests/utils/test_helpers.py index 0e8d90449..041f72b87 100644 --- a/tests/utils/test_helpers.py +++ b/tests/utils/test_helpers.py @@ -2,18 +2,21 @@ import os import tempfile + from pathlib import Path from stat import S_IREAD from typing import TYPE_CHECKING import pytest + if TYPE_CHECKING: from pytest_mock import MockerFixture -from poetry.core.utils.helpers import combine_unicode, robust_rmtree +from poetry.core.utils.helpers import combine_unicode from poetry.core.utils.helpers import parse_requires from poetry.core.utils.helpers import readme_content_type +from poetry.core.utils.helpers import robust_rmtree from poetry.core.utils.helpers import temporary_directory From ba701895c3b652d188a0f9927d3bbc057fd8037d Mon Sep 17 00:00:00 2001 From: Cristian Libotean Date: Wed, 26 Apr 2023 20:20:42 +0300 Subject: [PATCH 09/15] Accept both str and Path in robust_rmtree. Reworked test for robust_rmtree to actually delete the temp stuff created during the test. --- src/poetry/core/utils/helpers.py | 7 ++++--- tests/utils/test_helpers.py | 4 +++- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/src/poetry/core/utils/helpers.py b/src/poetry/core/utils/helpers.py index 4cebae66b..63f8445ec 100644 --- a/src/poetry/core/utils/helpers.py +++ b/src/poetry/core/utils/helpers.py @@ -91,20 +91,21 @@ def _on_rm_error(func: Any, path: str | Path, exc_info: Any) -> None: func(path) -def robust_rmtree(path: str, max_timeout: float = 1) -> None: +def robust_rmtree(path: str | Path, max_timeout: float = 1) -> None: """ Robustly tries to delete paths. Retries several times if an OSError occurs. If the final attempt fails, the Exception is propagated to the caller. """ + path = Path(path) # make sure this is a Path object, not str timeout = 0.001 while timeout < max_timeout: try: # both os.unlink and shutil.rmtree can throw exceptions on Windows # if the files are in use when called - if Path(path).is_symlink(): - os.unlink(str(path)) + if path.is_symlink(): + path.unlink() else: shutil.rmtree(path) return # Only hits this on success diff --git a/tests/utils/test_helpers.py b/tests/utils/test_helpers.py index 36a8d79a0..b2bc252e8 100644 --- a/tests/utils/test_helpers.py +++ b/tests/utils/test_helpers.py @@ -149,4 +149,6 @@ def test_robust_rmtree(mocker: MockerFixture) -> None: robust_rmtree(name, max_timeout=0.04) # clear the side effect (breaks the tear-down otherwise) - mocked_rmtree.side_effect = None + mocker.stop(mocked_rmtree) + # use the real method to remove the temp folder we created for this test + robust_rmtree(name) From f1db06b9df032002412eb856de2c301f2695e3e9 Mon Sep 17 00:00:00 2001 From: Cristian Libotean Date: Thu, 27 Apr 2023 17:28:26 +0300 Subject: [PATCH 10/15] For Python 3.10+ use tempfile.TemporaryDirectory() to get a temporary directory. --- src/poetry/core/utils/helpers.py | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/src/poetry/core/utils/helpers.py b/src/poetry/core/utils/helpers.py index 63f8445ec..a2ba2ef42 100644 --- a/src/poetry/core/utils/helpers.py +++ b/src/poetry/core/utils/helpers.py @@ -3,6 +3,7 @@ import os import shutil import stat +import sys import tempfile import time import unicodedata @@ -41,9 +42,13 @@ def normalize_version(version: str) -> str: @contextmanager def temporary_directory(*args: Any, **kwargs: Any) -> Iterator[str]: - name = tempfile.mkdtemp(*args, **kwargs) - yield name - robust_rmtree(name) + if sys.version_info >= (3, 10): + with tempfile.TemporaryDirectory(*args, **kwargs, ignore_cleanup_errors=True) as name: + yield name + else: + name = tempfile.mkdtemp(*args, **kwargs) + yield name + robust_rmtree(name) def parse_requires(requires: str) -> list[str]: From add8534eb3cb1aa517e2cd9888fe3bf0b0e1bbc2 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Thu, 27 Apr 2023 14:30:18 +0000 Subject: [PATCH 11/15] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- src/poetry/core/utils/helpers.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/poetry/core/utils/helpers.py b/src/poetry/core/utils/helpers.py index a2ba2ef42..dc46ab074 100644 --- a/src/poetry/core/utils/helpers.py +++ b/src/poetry/core/utils/helpers.py @@ -43,7 +43,9 @@ def normalize_version(version: str) -> str: @contextmanager def temporary_directory(*args: Any, **kwargs: Any) -> Iterator[str]: if sys.version_info >= (3, 10): - with tempfile.TemporaryDirectory(*args, **kwargs, ignore_cleanup_errors=True) as name: + with tempfile.TemporaryDirectory( + *args, **kwargs, ignore_cleanup_errors=True + ) as name: yield name else: name = tempfile.mkdtemp(*args, **kwargs) From 8cf4debb4fd454be90423988e9235b3e61e0e179 Mon Sep 17 00:00:00 2001 From: Cristian Libotean Date: Thu, 27 Apr 2023 18:22:42 +0300 Subject: [PATCH 12/15] Added unit tests for python 3.9- and 3.10+ to check that TemporaryDirectory is actually used. --- src/poetry/core/utils/helpers.py | 7 ++++--- tests/utils/test_helpers.py | 31 +++++++++++++++++++++++++++++++ 2 files changed, 35 insertions(+), 3 deletions(-) diff --git a/src/poetry/core/utils/helpers.py b/src/poetry/core/utils/helpers.py index dc46ab074..887bb2532 100644 --- a/src/poetry/core/utils/helpers.py +++ b/src/poetry/core/utils/helpers.py @@ -43,9 +43,10 @@ def normalize_version(version: str) -> str: @contextmanager def temporary_directory(*args: Any, **kwargs: Any) -> Iterator[str]: if sys.version_info >= (3, 10): - with tempfile.TemporaryDirectory( - *args, **kwargs, ignore_cleanup_errors=True - ) as name: + # mypy reports an error if ignore_cleanup_errors is + # specified literally in the call + kwargs["ignore_cleanup_errors"] = True + with tempfile.TemporaryDirectory(*args, **kwargs) as name: yield name else: name = tempfile.mkdtemp(*args, **kwargs) diff --git a/tests/utils/test_helpers.py b/tests/utils/test_helpers.py index b2bc252e8..adaac2801 100644 --- a/tests/utils/test_helpers.py +++ b/tests/utils/test_helpers.py @@ -1,6 +1,7 @@ from __future__ import annotations import os +import sys import tempfile from pathlib import Path @@ -127,6 +128,36 @@ def test_utils_helpers_readme_content_type( assert readme_content_type(readme) == content_type +def test_temporary_directory_python_3_9_or_older(mocker: MockerFixture) -> None: + mocked_rmtree = mocker.patch("shutil.rmtree") + mocked_temp_dir = mocker.patch("tempfile.TemporaryDirectory") + mocked_mkdtemp = mocker.patch("tempfile.mkdtemp") + + mocker.patch.object(sys, "version_info", (3, 10)) + with temporary_directory() as tmp: + assert tmp + + assert not mocked_rmtree.called + assert not mocked_mkdtemp.called + mocked_temp_dir.assert_called_with(ignore_cleanup_errors=True) + + +def test_temporary_directory_python_3_10_or_newer(mocker: MockerFixture) -> None: + mocked_rmtree = mocker.patch("shutil.rmtree") + mocked_temp_dir = mocker.patch("tempfile.TemporaryDirectory") + mocked_mkdtemp = mocker.patch("tempfile.mkdtemp") + + mocked_mkdtemp.return_value = "hello from test" + + mocker.patch.object(sys, "version_info", (3, 9)) + with temporary_directory() as tmp: + assert tmp == "hello from test" + + assert mocked_rmtree.called + assert mocked_mkdtemp.called + assert not mocked_temp_dir.called + + def test_robust_rmtree(mocker: MockerFixture) -> None: mocked_rmtree = mocker.patch("shutil.rmtree") From ab38ee975ce9379244bc35000bbcc86ecff14da4 Mon Sep 17 00:00:00 2001 From: Cristian Libotean Date: Thu, 27 Apr 2023 18:32:18 +0300 Subject: [PATCH 13/15] Mixed the names for the temporary_directory() tests, should be the other way around --- tests/utils/test_helpers.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/utils/test_helpers.py b/tests/utils/test_helpers.py index adaac2801..83e64484a 100644 --- a/tests/utils/test_helpers.py +++ b/tests/utils/test_helpers.py @@ -128,7 +128,7 @@ def test_utils_helpers_readme_content_type( assert readme_content_type(readme) == content_type -def test_temporary_directory_python_3_9_or_older(mocker: MockerFixture) -> None: +def test_temporary_directory_python_3_10_or_newer(mocker: MockerFixture) -> None: mocked_rmtree = mocker.patch("shutil.rmtree") mocked_temp_dir = mocker.patch("tempfile.TemporaryDirectory") mocked_mkdtemp = mocker.patch("tempfile.mkdtemp") @@ -142,7 +142,7 @@ def test_temporary_directory_python_3_9_or_older(mocker: MockerFixture) -> None: mocked_temp_dir.assert_called_with(ignore_cleanup_errors=True) -def test_temporary_directory_python_3_10_or_newer(mocker: MockerFixture) -> None: +def test_temporary_directory_python_3_9_or_older(mocker: MockerFixture) -> None: mocked_rmtree = mocker.patch("shutil.rmtree") mocked_temp_dir = mocker.patch("tempfile.TemporaryDirectory") mocked_mkdtemp = mocker.patch("tempfile.mkdtemp") From 0e16074cc3b2e60b00bfa2a2d53c9259177111b4 Mon Sep 17 00:00:00 2001 From: Cristian Libotean Date: Sat, 29 Apr 2023 10:02:42 +0300 Subject: [PATCH 14/15] Code review suggestions --- tests/utils/test_helpers.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/utils/test_helpers.py b/tests/utils/test_helpers.py index 83e64484a..4d2cc32b8 100644 --- a/tests/utils/test_helpers.py +++ b/tests/utils/test_helpers.py @@ -172,7 +172,6 @@ def test_robust_rmtree(mocker: MockerFixture) -> None: robust_rmtree(name) # this should give up after retrying multiple times - name = tempfile.mkdtemp() mocked_rmtree.side_effect = OSError( "Couldn't delete file yet, this error won't go away after first attempt" ) @@ -183,3 +182,4 @@ def test_robust_rmtree(mocker: MockerFixture) -> None: mocker.stop(mocked_rmtree) # use the real method to remove the temp folder we created for this test robust_rmtree(name) + assert not Path(name).exists() \ No newline at end of file From a2d6dcfbdf2e97c946de02747f546db2780703f3 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Sat, 29 Apr 2023 07:03:18 +0000 Subject: [PATCH 15/15] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- tests/utils/test_helpers.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/utils/test_helpers.py b/tests/utils/test_helpers.py index 4d2cc32b8..af54bc979 100644 --- a/tests/utils/test_helpers.py +++ b/tests/utils/test_helpers.py @@ -182,4 +182,4 @@ def test_robust_rmtree(mocker: MockerFixture) -> None: mocker.stop(mocked_rmtree) # use the real method to remove the temp folder we created for this test robust_rmtree(name) - assert not Path(name).exists() \ No newline at end of file + assert not Path(name).exists()