Initialize cache directory in isolation

[tamird]

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 #12167.

[nicoddemus]

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.

[nicoddemus]

TBH I'm starting to see this change as -0: we are changing how the supporting files are being created for little reason (as the original reason for the issue is no longer valid). I fear we will be introducing subtle bugs for little gain.

[tamird]

TBH I'm starting to see this change as -0: we are changing how the supporting files are being created for little reason (as the original reason for the issue is no longer valid). I fear we will be introducing subtle bugs for little gain.

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.

[nicoddemus]

The original reason is as valid as it ever was - I saw this symptom on my local machine

An interruption between mkdir and writing the files? This seems highly unlikely to happen, those lines are next to each other -- the current code will solve this indeed, but I fear we will be introducing subtle issues and breaking test suites for minimal/marginal gains, hence -0.

But if @bluetech prefers, then definitely move forward with this.

[nicoddemus]

I left a few comments which are good to apply, but I will remove myself from reviewer and defer the final decision here to @bluetech. 👍

Regardless of the outcome, thanks @tamird for the PR, we appreciate it. 👍

[RonnyPfannschmidt]

at first glance the proposed mechanism is broken in most linux deployments simply by degrading to copytree on anything that has TMP on tmpfs

[bluetech]

LGTM, thanks!

[RonnyPfannschmidt]

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

[tamird]

@nicoddemus could you have a look? I believe this is blocked on your approval.

[nicoddemus]

It is not blocked, I will defer merging to @bluetech or @RonnyPfannschmidt. 👍

[tamird]

[nicoddemus]

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.

[tamird]

Now I just need someone to click merge 😄

[tamird]

Turns out the behavior I was seeing which prompted me to send this PR was in fact caused by mkdir not properly initializing the cache directory. Plugins (such as pytest-insta) use this function; if they are the first user, then the directory does not get initialized properly. Anyway, this PR fixes that, but we should probably add a test -- I'll send another PR shortly.

[tamird]

#12199