[libcxx-commits] [PATCH] D78245: [LIT] Make `%T` unique for every test

[Eric Fiselier via Phabricator via libcxx-commits]

EricWF marked 2 inline comments as done.
EricWF added a comment.

In D78245#1985201 <https://reviews.llvm.org/D78245#1985201>, @thakis wrote:

> Are you aware of https://llvm.org/docs/CommandGuide/lit.html#substitutions ("%T: parent directory of %t (not unique, deprecated, do not use)")? We've talked about this a bunch in the past, and the decision back then was to do `mkdir %t` in the tests that need a dir, and to remove %T over time.


As @ldionne said, this is less than ideal for the libc++ tests, which aren't structured as true ShTest's. I would be happy with a solution that allowed libc++ to twist these knobs without affecting everybody.



================
Comment at: libcxx/utils/run.py:46
 
     # Create the execution directory, and make sure we remove it at the end.
     try:
----------------
ldionne wrote:
> A few comments around this part of the change:
> 
> 1. Must fix this comment -- instead we can say that we assume it to already exist.
> 2. I think you can remove that `try-finally` altogether now.
> 
> However, this change means that we won't be able to copy-paste a `RUN` line from a failed test and execute it as-is. Before we do that, we'll need to make sure that the `--execdir` in the command-line exists. This wouldn't be an issue if we kept this code as-is, which would be my preference.
I would rather remove execdir and assume we run the binary in the same directory it's in. Then it's trivially true that the directory exists.
That's much less complicated than all this.


================
Comment at: llvm/utils/lit/lit/TestRunner.py:1500
+    tmpDir = os.path.dirname(tmpBase)
+    if os.path.isdir(tmpDir):
+        shutil.rmtree(tmpDir)
----------------
ldionne wrote:
> EricWF wrote:
> > This change probably isn't needed.
> I think it is, actually, cause we do need to make sure that tests either clean-up after themselves (which they did before in `run.py` due to `shutil.rmtree(args.execdir)`), or that they clean up before they run in case a previous run left something weird.
> 
>  @EricWF As-is, the exec dir for each test will contain stuff between test runs, which takes up space and you have asked me to remove that IIRC . Instead, why don't we remove the directory after we're done running the test in `run.py` above? This would address this issue, in addition to making `RUN` lines in libc++ copy-pasteable.
I actually really do like having the failing executables still on the filesystem, because they're a pain to reproduce manually.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D78245/new/

https://reviews.llvm.org/D78245