From 6cd2d197c1d4a92539360c32887927b5926e8656 Mon Sep 17 00:00:00 2001 From: Matthieu Darbois Date: Fri, 5 Jun 2026 14:10:48 +0200 Subject: [PATCH] fix: remove potential partial cache-population in case of error (#2892) fix: remove potential partial extraction in case of error Co-authored-by: Copilot <198982749+Copilot@users.noreply.github.com> --- cibuildwheel/platforms/android.py | 11 +++- cibuildwheel/platforms/ios.py | 13 ++++- cibuildwheel/platforms/macos.py | 25 +++++++-- cibuildwheel/platforms/pyodide.py | 58 +++++++++++--------- cibuildwheel/platforms/windows.py | 17 ++++-- cibuildwheel/util/file.py | 19 ++++++- cibuildwheel/util/python_build_standalone.py | 5 +- cibuildwheel/venv.py | 5 +- unit_test/utils_test.py | 55 ++++++++++++++++++- 9 files changed, 159 insertions(+), 49 deletions(-) diff --git a/cibuildwheel/platforms/android.py b/cibuildwheel/platforms/android.py index 032d7730..52773e05 100644 --- a/cibuildwheel/platforms/android.py +++ b/cibuildwheel/platforms/android.py @@ -30,7 +30,13 @@ from cibuildwheel.frontend import ( from cibuildwheel.logger import log from cibuildwheel.util import resources from cibuildwheel.util.cmd import call, shell -from cibuildwheel.util.file import CIBW_CACHE_PATH, copy_test_sources, download, move_file +from cibuildwheel.util.file import ( + CIBW_CACHE_PATH, + copy_test_sources, + download, + move_file, + remove_on_error, +) from cibuildwheel.util.helpers import prepare_command from cibuildwheel.util.packaging import find_compatible_wheel from cibuildwheel.util.python_build_standalone import create_python_build_standalone_environment @@ -182,7 +188,8 @@ def setup_target_python(config: PythonConfiguration, build_path: Path) -> Path: python_tgz = CIBW_CACHE_PATH / config.url.rpartition("/")[-1] with FileLock(f"{python_tgz}.lock"): if not python_tgz.exists(): - download(config.url, python_tgz, sha256=config.sha256) + with remove_on_error(python_tgz): + download(config.url, python_tgz, sha256=config.sha256) python_dir = build_path / "python" python_dir.mkdir() diff --git a/cibuildwheel/platforms/ios.py b/cibuildwheel/platforms/ios.py index b232ba3e..751f3616 100644 --- a/cibuildwheel/platforms/ios.py +++ b/cibuildwheel/platforms/ios.py @@ -25,7 +25,13 @@ from cibuildwheel.logger import log from cibuildwheel.platforms.macos import install_cpython as install_build_cpython from cibuildwheel.util import resources from cibuildwheel.util.cmd import call, shell, split_command -from cibuildwheel.util.file import CIBW_CACHE_PATH, copy_test_sources, download, move_file +from cibuildwheel.util.file import ( + CIBW_CACHE_PATH, + copy_test_sources, + download, + move_file, + remove_on_error, +) from cibuildwheel.util.helpers import prepare_command, unwrap_preserving_paragraphs from cibuildwheel.util.packaging import find_compatible_wheel from cibuildwheel.venv import constraint_flags, virtualenv @@ -140,8 +146,9 @@ def install_target_cpython(tmp: Path, config: PythonConfiguration, free_threadin if not installation_path.exists(): downloaded_tar_gz = tmp / ios_python_tar_gz download(config.url, downloaded_tar_gz, sha256=config.sha256) - installation_path.mkdir(parents=True, exist_ok=True) - call("tar", "-C", installation_path, "-xf", downloaded_tar_gz) + with remove_on_error(installation_path): + installation_path.mkdir(parents=True) + call("tar", "-C", installation_path, "-xf", downloaded_tar_gz) downloaded_tar_gz.unlink() return installation_path diff --git a/cibuildwheel/platforms/macos.py b/cibuildwheel/platforms/macos.py index 3ee9ff0c..c2aa23df 100644 --- a/cibuildwheel/platforms/macos.py +++ b/cibuildwheel/platforms/macos.py @@ -27,7 +27,13 @@ from cibuildwheel.frontend import ( from cibuildwheel.logger import log from cibuildwheel.util import resources from cibuildwheel.util.cmd import call, shell -from cibuildwheel.util.file import CIBW_CACHE_PATH, copy_test_sources, download, move_file +from cibuildwheel.util.file import ( + CIBW_CACHE_PATH, + copy_test_sources, + download, + move_file, + remove_on_error, +) from cibuildwheel.util.helpers import prepare_command, unwrap from cibuildwheel.util.packaging import find_compatible_wheel, get_pip_version from cibuildwheel.venv import constraint_flags, find_uv, target_marker_env, virtualenv @@ -196,7 +202,8 @@ def install_pypy(tmp: Path, url: str, sha256: str) -> Path: downloaded_tar_bz2 = tmp / pypy_tar_bz2 download(url, downloaded_tar_bz2, sha256=sha256) installation_path.parent.mkdir(parents=True, exist_ok=True) - call("tar", "-C", installation_path.parent, "-xf", downloaded_tar_bz2) + with remove_on_error(installation_path): + call("tar", "-C", installation_path.parent, "-xf", downloaded_tar_bz2) downloaded_tar_bz2.unlink() return installation_path / "bin" / "pypy3" @@ -210,9 +217,17 @@ def install_graalpy(tmp: Path, url: str, sha256: str) -> Path: if not installation_path.exists(): downloaded_archive = tmp / graalpy_archive download(url, downloaded_archive, sha256=sha256) - installation_path.mkdir(parents=True) - # GraalPy top-folder name is inconsistent with archive name - call("tar", "-C", installation_path, "--strip-components=1", "-xzf", downloaded_archive) + with remove_on_error(installation_path): + installation_path.mkdir(parents=True) + # GraalPy top-folder name is inconsistent with archive name + call( + "tar", + "-C", + installation_path, + "--strip-components=1", + "-xzf", + downloaded_archive, + ) downloaded_archive.unlink() return installation_path / "bin" / "graalpy" diff --git a/cibuildwheel/platforms/pyodide.py b/cibuildwheel/platforms/pyodide.py index 1fb20ddb..b4f7cd1b 100644 --- a/cibuildwheel/platforms/pyodide.py +++ b/cibuildwheel/platforms/pyodide.py @@ -29,6 +29,7 @@ from cibuildwheel.util.file import ( extract_tar, extract_zip, move_file, + remove_on_error, ) from cibuildwheel.util.helpers import prepare_command, unwrap, unwrap_preserving_paragraphs from cibuildwheel.util.packaging import find_compatible_wheel, get_pip_version @@ -92,10 +93,11 @@ def ensure_node(major_version: str) -> Path: with TemporaryDirectory() as tmp_path: archive = Path(tmp_path) / f"{name}.{ext}" download(url, archive) - if ext == "zip": - extract_zip(archive, path.parent) - else: - extract_tar(archive, path.parent) + with remove_on_error(path): + if ext == "zip": + extract_zip(archive, path.parent) + else: + extract_tar(archive, path.parent) assert path.exists() if not IS_WIN: return path / "bin" @@ -110,18 +112,19 @@ def install_emscripten(env: dict[str, str], version: str, xbuildenv_cache_path: with FileLock(CIBW_CACHE_PATH / "emscripten.lock"): if emscripten_dir.exists(): return emscripten_dir - call( - "pyodide", - "xbuildenv", - "install-emscripten", - "--force", - "--version", - version, - "--path", - str(xbuildenv_cache_path), - env=env, - cwd=CIBW_CACHE_PATH, - ) + with remove_on_error(emscripten_dir): + call( + "pyodide", + "xbuildenv", + "install-emscripten", + "--force", + "--version", + version, + "--path", + str(xbuildenv_cache_path), + env=env, + cwd=CIBW_CACHE_PATH, + ) assert emscripten_dir.exists() return emscripten_dir @@ -208,17 +211,18 @@ def install_xbuildenv(env: dict[str, str], xbuildenv_cache_path: Path, pyodide_v env.pop("PYODIDE_ROOT", None) # Install the xbuildenv - call( - "pyodide", - "xbuildenv", - "install", - "--path", - str(xbuildenv_cache_path), - pyodide_version, - env=env, - cwd=CIBW_CACHE_PATH, - ) - assert pyodide_root.exists() + with remove_on_error(xbuildenv_cache_path / pyodide_version): + call( + "pyodide", + "xbuildenv", + "install", + "--path", + str(xbuildenv_cache_path), + pyodide_version, + env=env, + cwd=CIBW_CACHE_PATH, + ) + assert pyodide_root.exists() return str(pyodide_root) diff --git a/cibuildwheel/platforms/windows.py b/cibuildwheel/platforms/windows.py index 7dbd3a56..f55fc54d 100644 --- a/cibuildwheel/platforms/windows.py +++ b/cibuildwheel/platforms/windows.py @@ -30,6 +30,7 @@ from cibuildwheel.util.file import ( download, extract_zip, move_file, + remove_on_error, ) from cibuildwheel.util.helpers import prepare_command, unwrap from cibuildwheel.util.packaging import find_compatible_wheel, get_pip_version @@ -112,7 +113,8 @@ def _ensure_nuget() -> Path: nuget = CIBW_CACHE_PATH / "nuget.exe" with FileLock(str(nuget) + ".lock"): if not nuget.exists(): - download("https://dist.nuget.org/win-x86-commandline/latest/nuget.exe", nuget) + with remove_on_error(nuget): + download("https://dist.nuget.org/win-x86-commandline/latest/nuget.exe", nuget) return nuget @@ -128,7 +130,8 @@ def install_cpython(configuration: PythonConfiguration, arch: str | None = None) with FileLock(str(base_output_dir) + f"-{version}{free_threaded_str}-{arch}.lock"): if not installation_path.exists(): nuget = _ensure_nuget() - call(nuget, "install", *nuget_args) + with remove_on_error(installation_path.parent): + call(nuget, "install", *nuget_args) return installation_path / "python.exe" @@ -144,8 +147,9 @@ def install_pypy(tmp: Path, arch: str, url: str, sha256: str) -> Path: if not installation_path.exists(): pypy_zip = tmp / zip_filename download(url, pypy_zip, sha256=sha256) - # Extract to the parent directory because the zip file still contains a directory - extract_zip(pypy_zip, installation_path.parent) + with remove_on_error(installation_path): + # Extract to the parent directory because the zip file still contains a directory + extract_zip(pypy_zip, installation_path.parent) return installation_path / "python.exe" @@ -158,8 +162,9 @@ def install_graalpy(tmp: Path, url: str, sha256: str) -> Path: if not installation_path.exists(): graalpy_zip = tmp / zip_filename download(url, graalpy_zip, sha256=sha256) - # Extract to the parent directory because the zip file still contains a directory - extract_zip(graalpy_zip, installation_path.parent) + with remove_on_error(installation_path): + # Extract to the parent directory because the zip file still contains a directory + extract_zip(graalpy_zip, installation_path.parent) return installation_path / "bin" / "graalpy.exe" diff --git a/cibuildwheel/util/file.py b/cibuildwheel/util/file.py index 45a7932f..b92727ac 100644 --- a/cibuildwheel/util/file.py +++ b/cibuildwheel/util/file.py @@ -7,6 +7,7 @@ import ssl import tarfile import time import urllib.request +from contextlib import contextmanager from pathlib import Path, PurePath from typing import Final from zipfile import ZipFile @@ -18,7 +19,7 @@ from cibuildwheel.errors import FatalError TYPE_CHECKING = False if TYPE_CHECKING: - from collections.abc import Callable + from collections.abc import Callable, Generator DEFAULT_CIBW_CACHE_PATH: Final[Path] = user_cache_path(appname="cibuildwheel", appauthor="pypa") CIBW_CACHE_PATH: Final[Path] = Path( @@ -26,6 +27,22 @@ CIBW_CACHE_PATH: Final[Path] = Path( ).resolve() +@contextmanager +def remove_on_error(path: Path) -> Generator[None, None, None]: + try: + yield + except BaseException as original_exception: + try: + if path.is_dir() and not path.is_symlink(): + shutil.rmtree(path) + elif path.exists() or path.is_symlink(): + path.unlink() + except BaseException as cleanup_exception: + msg = f"Failed to remove {path}. Please remove it manually." + raise BaseExceptionGroup(msg, [original_exception, cleanup_exception]) from None + raise + + def ensure_cache_sentinel(cache_path: Path) -> None: """Create a sentinel file to mark a cibuildwheel cache directory. diff --git a/cibuildwheel/util/python_build_standalone.py b/cibuildwheel/util/python_build_standalone.py index 5f6cb2f5..186569bb 100644 --- a/cibuildwheel/util/python_build_standalone.py +++ b/cibuildwheel/util/python_build_standalone.py @@ -10,7 +10,7 @@ import typing from filelock import FileLock -from cibuildwheel.util.file import download, extract_tar +from cibuildwheel.util.file import download, extract_tar, remove_on_error from cibuildwheel.util.resources import PYTHON_BUILD_STANDALONE_RELEASES TYPE_CHECKING = False @@ -157,7 +157,8 @@ def _download_or_get_from_cache( return asset_cache_path print(f"Downloading python_build_standalone: {asset_url} to {asset_cache_path}") - download(asset_url, asset_cache_path, sha256=sha256 or None) + with remove_on_error(asset_cache_path): + download(asset_url, asset_cache_path, sha256=sha256 or None) return asset_cache_path diff --git a/cibuildwheel/venv.py b/cibuildwheel/venv.py index 68404cac..174b2a1b 100644 --- a/cibuildwheel/venv.py +++ b/cibuildwheel/venv.py @@ -16,7 +16,7 @@ from packaging.version import Version from cibuildwheel.util import resources from cibuildwheel.util.cmd import call -from cibuildwheel.util.file import CIBW_CACHE_PATH, download +from cibuildwheel.util.file import CIBW_CACHE_PATH, download, remove_on_error TYPE_CHECKING = False if TYPE_CHECKING: @@ -57,7 +57,8 @@ def _ensure_virtualenv(version: str) -> tuple[Path, Version]: path = CIBW_CACHE_PATH / f"virtualenv-{version}.pyz" with FileLock(str(path) + ".lock"): if not path.exists(): - download(url, path, sha256=sha256) + with remove_on_error(path): + download(url, path, sha256=sha256) return (path, Version(version)) diff --git a/unit_test/utils_test.py b/unit_test/utils_test.py index 85f38ebf..2322a057 100644 --- a/unit_test/utils_test.py +++ b/unit_test/utils_test.py @@ -1,3 +1,4 @@ +import re import textwrap from pathlib import Path, PurePath from unittest.mock import Mock, call @@ -6,7 +7,7 @@ import pytest from cibuildwheel import errors from cibuildwheel.ci import fix_ansi_codes_for_github_actions -from cibuildwheel.util.file import copy_test_sources +from cibuildwheel.util.file import copy_test_sources, remove_on_error from cibuildwheel.util.helpers import ( FlexibleVersion, format_safe, @@ -395,6 +396,58 @@ def test_copy_test_sources_alternate_copy_into(sample_project: Path) -> None: ) +def test_remove_on_error_keeps_paths(tmp_path: Path) -> None: + file_path = tmp_path / "file.txt" + file_path.write_text("data") + dir_path = tmp_path / "dir" + dir_path.mkdir() + + for path in (file_path, dir_path): + with remove_on_error(path): + pass + + assert file_path.exists() + assert dir_path.exists() + + +def test_remove_on_error_file_or_tree(tmp_path: Path) -> None: + file_path = tmp_path / "file.txt" + file_path.write_text("data") + dir_path = tmp_path / "dir" + nested_file_path = dir_path / "nested.txt" + nested_file_path.parent.mkdir() + nested_file_path.write_text("data") + + for path in (file_path, dir_path): + with pytest.raises(RuntimeError), remove_on_error(path): + raise RuntimeError + + assert not file_path.exists() + assert not dir_path.exists() + + +def test_remove_on_error_cleanup_failure(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: + path = tmp_path / "cleanup-failure" + path.mkdir() + original_exception = RuntimeError("original error") + cleanup_exception = PermissionError("cleanup error") + + def _raise_cleanup_error(_: Path) -> None: + raise cleanup_exception + + monkeypatch.setattr("cibuildwheel.util.file.shutil.rmtree", _raise_cleanup_error) + with ( + pytest.raises( + BaseExceptionGroup, + match=re.escape(f"Failed to remove {path}. Please remove it manually."), + ) as caught_exception, + remove_on_error(path), + ): + raise original_exception + + assert caught_exception.value.exceptions == (original_exception, cleanup_exception) + + def test_unwrap() -> None: assert ( unwrap("""