cacheprovider: simplify cache directory creation #14121
Conversation
RonnyPfannschmidt
added
the
skip news
There was a problem hiding this comment.
Pull request overview
This PR refactors the cache directory creation logic in cacheprovider.py by extracting it into a dedicated _make_cachedir function, simplifying the code structure while maintaining atomic creation and proper race condition handling.
Changes:
- Consolidated file contents (README.md, .gitignore, CACHEDIR.TAG) into a single
CACHEDIR_FILESdictionary with bytes values - Extracted cache directory creation logic into a new
_make_cachedirfunction - Replaced
tempfile.TemporaryDirectorycontext manager withtempfile.mkdtemp+shutil.rmtreefor simpler cleanup
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
RonnyPfannschmidt
force-pushed
the
cachedir-fill-files
branch
from
007d36e to
b2505ff
Compare
Refactor _ensure_cache_dir_and_supporting_files to use a dedicated _make_cachedir function that atomically creates the cache directory with its supporting files (README.md, .gitignore, CACHEDIR.TAG). - Store all file contents as bytes in CACHEDIR_FILES dict - Use tempfile.mkdtemp + shutil.rmtree instead of TemporaryDirectory - Simplify cleanup logic for race conditions Co-authored-by: Cursor AI <ai@cursor.sh> Co-authored-by: Anthropic Claude Opus 4 <claude@anthropic.com>
RonnyPfannschmidt
force-pushed
the
cachedir-fill-files
branch
from
b2505ff to
212c1e1
Compare
| # gets "Directory not empty" from the rename. In this case, | ||
| # everything is handled so just continue after cleanup. | ||
| # On Windows, the error is a FileExistsError which translates to EEXIST. | ||
| shutil.rmtree(path, ignore_errors=True) |
There was a problem hiding this comment.
TemporaryDirectory does some extra stuff in its rmtree:
Perhaps it is prudent to keep using it?
Also I figure the TODO "pass ignore_cleanup_errors=True when we no longer support python < 3.10." is now applicable, maybe the TemporaryDirectory code is cleaner with that?
There was a problem hiding this comment.
i decided to go plain rmtree after assessing what we do and what TemporaryDirectory handles in addition and/or wrong
basically our goal is to make a directory , fill it with files, then push it in - so the extra errors it handles are not part of our flow, however we need to handle it breaking due to not supporting tunoff of delete in messy manners
just creating the directory, filling it in and renaming with rmtree on error seems way more simple/robust - no more working around TemporaryDirectory not fitting our use-case
bluetech
left a comment
bluetech
left a comment
There was a problem hiding this comment.
OK, LGTM.
The except BaseException case is not covered, would be nice to add a test for it but OK if it's too hard.
Co-authored-by: Cursor AI <ai@cursor.sh> Co-authored-by: Anthropic Claude Opus 4.5 <claude@anthropic.com>
2 participants
Refactor
_ensure_cache_dir_and_supporting_filesto use a dedicated_make_cachedirfunction that atomically creates the cache directory with its supporting files (README.md, .gitignore, CACHEDIR.TAG).CACHEDIR_FILESdicttempfile.mkdtemp+shutil.rmtreeinstead ofTemporaryDirectory