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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
Comment at: libcxx/utils/run.py:46
 
     # Create the execution directory, and make sure we remove it at the end.
     try:
----------------
EricWF wrote:
> 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.
The problem is that we must then `cd <directory>` before we can execute the test when copy-pasting a `RUN` line from a failed test. The thing I *really* love right now with the new format is that you can always just copy-paste the compiler command line (after `COMPILED AS:`) and then the execution line (after `EXECUTED AS:`) from any directory, and it all just works.

Perhaps we could make it clearer by adding the directory where a test is executed to `lit`'s output? Otherwise, we have to basically infer it from the compiler command-line with the location of `-o`, and that's just annoying to do every time.



================
Comment at: llvm/utils/lit/lit/TestRunner.py:1500
+    tmpDir = os.path.dirname(tmpBase)
+    if os.path.isdir(tmpDir):
+        shutil.rmtree(tmpDir)
----------------
EricWF wrote:
> 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.
They are not anymore -- you can just copy-paste the `COMPILED AS:` command line and it just works. It does with the new format currently, at least. This was one of the explicit design goals, because I was really annoyed by that specific shortcoming of the old format.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78245