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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.

I really like that `lit` could provide a unique-per-test temporary directory. This would simplify things quite a bit.

A few more comments:

1. The Lit documentation for `%T` should be updated
2. Does any `lit` test need to be added?
3. The `ssh.py` executor now needs to get passed `--execdir %T`, and it needs to copy the dependencies into the current execdir, and then tar up all the execdir.



================
Comment at: libcxx/utils/libcxx/test/newformat.py:204
     # Modified version of lit.TestRunner.executeShTest to handle custom parsers correctly.
     def _executeShTest(self, test, litConfig, steps, fileDependencies=None):
         if test.config.unsupported:
----------------
You can remove `fileDependencies=None` now.


================
Comment at: libcxx/utils/libcxx/test/newformat.py:209
         additionalCompileFlags = []
         fileDependencies = fileDependencies or []
         parsers = [
----------------
And this becomes `fileDependencies = []`, unconditionally.


================
Comment at: libcxx/utils/run.py:46
 
     # Create the execution directory, and make sure we remove it at the end.
     try:
----------------
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.


================
Comment at: llvm/utils/lit/lit/TestRunner.py:1500
+    tmpDir = os.path.dirname(tmpBase)
+    if os.path.isdir(tmpDir):
+        shutil.rmtree(tmpDir)
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78245