fix: remove potential partial cache-population in case of error - #2892
Conversation
Co-Authored-By: Copilot <198982749+Copilot@users.noreply.github.com>
| @@ -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) | |||
There was a problem hiding this comment.
Should the download call have a remove_on_error wrapper as well? (macOS has the same pattern)
More generally - is there any reason that download() shouldn't incorporate remove_on_error handling? Is there any use case for downloading and preserving a failed download?
There was a problem hiding this comment.
All non-cache persisted download cases should be downloads to a temp directory -as is the case here - which is already taken care of when cibuildwheel exits.
I prefer the consistency of FileLock+remove_on_error always appearing together when it comes to preventing cache pollution, even if the download helper was updated with remove_on_error.
I consider that updating download might be worthwhile but orthogonal to the issue being fixed here and if it were updated, I'd rather keep the FileLock+remove_on_error consistency and be in a "belt and braces" situation in case like:
diff --git a/cibuildwheel/platforms/windows.py b/cibuildwheel/platforms/windows.py
index 7dbd3a56..f55fc54d 100644
@@ -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
There was a problem hiding this comment.
Ah - I missed the intersection of this with tmp usage. In which case - ignore my comment.
mayeut
changed the title
mayeut
changed the title
mhsmith
left a comment
mhsmith
left a comment
There was a problem hiding this comment.
The Android changes look fine to me.
5 participants
This address the "Resource Management & Concurrency" section of #2854
🤖 AI text below 🤖
This PR introduces a shared “cleanup on error” mechanism intended to prevent partially-populated cache entries (and related artifacts) from being left behind when downloads/extractions/install steps fail, and applies it across several cache-population code paths.
Changes:
remove_on_errorcontext manager incibuildwheel.util.fileto delete a target path if an exception occurs.remove_on_errorto clean up partial cache state on failure.