[PATCH] D76829: [lit] Introduce setup and teardown routines

[Louis Dionne via Phabricator via llvm-commits]

ldionne added a comment.

In D76829#1946482 <https://reviews.llvm.org/D76829#1946482>, @broadwaylamb wrote:

> In D76829#1946451 <https://reviews.llvm.org/D76829#1946451>, @ldionne wrote:
>
> > FWIW, that's needed for correctness because tests could modify these files. When running locally, we would also need to copy the files to temporary directories, but we don't do it.
>
>
> If such tests are introduced (those that modify their inputs), we'll just copy their inputs only for them. But there are no such tests now.


Well, those are tests, so if they do the wrong thing (and fail), what tells us they didn't modify the files? We're basically relying on the tests working correctly for the test suite to be correct, which is not very good. The only way of really being correct in libc++'s case is to copy the files and run the test on those copies. The only reason we're not doing it right now is because we're being lazy, but I'm actually working on fixing that right now.

I personally believe that a more useful feature for lit would be to allow batching sets of RUN commands or something like that. For example, imagine if we could compile all the tests locally and only then execute them on the remote host -- you wouldn't only save the cost of `scp`ing N times, you would also save on all the `ssh`'s. That would be a major improvement. To me, trying to just save on copying the inputs is taking the easy way for less benefit, and it's not clear to me this will even work with the upcoming libc++ lit configuration, where each test will require its copy of the inputs.

By the way, I don't mean to say that this patch isn't useful -- I'm just saying that in the context of trying to solve the specific problem at hand (libc++'s `std::filesystem` on a remote host), I'm not convinced this is the right thing.


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

https://reviews.llvm.org/D76829