-
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Initialize cache directory in isolation #12168
Conversation
1710bb5
to
62f895d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot @tamird, appreciate the PR!
Left a few comments, please take a look.
Also, please include a changelog entry in changelog/12167.trivial.rst
:
cache: ensure supporting files (``CACHEDIR.TAG``, ``.gitignore``, etc) are always created in the cache directory, even in the event of test session being interrupted.
78fc304
to
2374cf7
Compare
2374cf7
to
9057525
Compare
d42c4fc
to
c24b40b
Compare
The original reason is as valid as it ever was - I saw this symptom on my local machine. It wasn't communicated properly to start, but validity hasn't changed. |
An interruption between But if @bluetech prefers, then definitely move forward with this. |
bf91b27
to
4a43b67
Compare
4f938d3
to
97ec412
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
at first glance the proposed mechanism is broken in most linux deployments simply by degrading to copytree on anything that has TMP on tmpfs
97ec412
to
7192d31
Compare
Creating and initializing the cache directory is interruptible; this avoids a pathological case where interrupting a cache write can cause the cache directory to never be properly initialized with its supporting files. Unify `Cache.mkdir` with `Cache.set` while I'm here so the former also properly initializes the cache directory. Closes pytest-dev#12167.
7192d31
to
2e65f4e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
given how temporarydirectory actually is used in our case,
a followup at a later point might want to replace it with a simpler mechanism
good work
@nicoddemus could you have a look? I believe this is blocked on your approval. |
It is not blocked, I will defer merging to @bluetech or @RonnyPfannschmidt. 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh my bad, did not realize that.
Relutanctly approving, as I mentioned I'm -0 on this -- need to mark as "Approve" to pass the merge requirements.
Now I just need someone to click merge 😄 |
Turns out the behavior I was seeing which prompted me to send this PR was in fact caused by |
Creating and initializing the cache directory is interruptible; this
avoids a pathological case where interrupting a cache write can cause
the cache directory to never be properly initialized with its supporting
files.
Unify
Cache.mkdir
withCache.set
while I'm here so the former alsoproperly initializes the cache directory.
Closes #12167.