Page MenuHomePhabricator

[LIT] Make `%T` unique for every test
AbandonedPublic

Authored by ldionne on Apr 15 2020, 2:32 PM.

Details

Reviewers
EricWF
Group Reviewers
Restricted Project
Restricted Project
Summary

This allows libc++ to use the same temp directory created by LIT's sh test, because that path is now hermetic to the test.

Diff Detail

Event Timeline

EricWF created this revision.Apr 15 2020, 2:32 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptApr 15 2020, 2:32 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
EricWF marked 2 inline comments as done.Apr 15 2020, 2:33 PM
EricWF added inline comments.
llvm/utils/lit/lit/TestRunner.py
1062

@ldionne Here's the relevant LIT change.

1500

This change probably isn't needed.

ldionne requested changes to this revision.Apr 15 2020, 3:00 PM

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.
libcxx/utils/libcxx/test/newformat.py
204

You can remove fileDependencies=None now.

209

And this becomes fileDependencies = [], unconditionally.

libcxx/utils/run.py
46

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.

llvm/utils/lit/lit/TestRunner.py
1500

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.

This revision now requires changes to proceed.Apr 15 2020, 3:00 PM

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.

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.

I was aware of it, and TBH I don't know why it's that way, but I've wished it would be different several times. Doing mkdir %t isn't an option for us, as all tests would need to do it. So we'd use the solution we used before this patch.

@thakis Could you link to previous discussions about %T? I'm sure there are reasons for the way things are today, but I'd like to understand them.

EricWF marked 2 inline comments as done.Apr 16 2020, 9:26 AM

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.

libcxx/utils/run.py
46

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.

llvm/utils/lit/lit/TestRunner.py
1500

I actually really do like having the failing executables still on the filesystem, because they're a pain to reproduce manually.

ldionne added inline comments.Apr 16 2020, 11:20 AM
libcxx/utils/run.py
46

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.

llvm/utils/lit/lit/TestRunner.py
1500

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.

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.

I was aware of it, and TBH I don't know why it's that way, but I've wished it would be different several times. Doing mkdir %t isn't an option for us, as all tests would need to do it. So we'd use the solution we used before this patch.

@thakis Could you link to previous discussions about %T? I'm sure there are reasons for the way things are today, but I'd like to understand them.

Gentle ping -- are there previous discussions on why %T was deprecated? I'm sure there is, but in the absence of any documentation supporting otherwise, it seems to be a useful feature.

@thakis Could you link to previous discussions about %T? I'm sure there are reasons for the way things are today, but I'd like to understand them.

I remember reading the discussion, but I failed to find it just now. @dsanders added the docs that say "deprecated"; maybe he remembers where the thread on moving off %T was?

Herald added a project: Restricted Project. · View Herald TranscriptApr 29 2020, 12:47 PM

Ah, I did find it after all, by git log --grep '%T' llvm/test. The discussion was at https://reviews.llvm.org/D35396.

(Thanks @dblaikie for the good commit message :) )

D35396 is pretty long, so as a public service, summarizing the objections to %T from D35396, having lit always pre-create a %T directory would slow down test runs. Considering %T has been deprecated and work done to remove it from llvm, clang, etc, there are not many tests that use it and it would pretty much be an unnecessary penalty for everyone (except libcxx, who needs to create the directory anyway). (I think I got that right.)

If lit would do this directory pre-creation only if it actually saw %T in the test, that I think would answer the objection.

@thakis Could you link to previous discussions about %T? I'm sure there are reasons for the way things are today, but I'd like to understand them.

I remember reading the discussion, but I failed to find it just now. @dsanders added the docs that say "deprecated"; maybe he remembers where the thread on moving off %T was?

I only added more whitespace to the line to make %{/s:regex_replacement} fit in the column. The bit about %T being deprecated was added in:
commit a2e0c2462a5ed4ddabab03dfddb97efc54682bde
Author: Kuba Mracek <mracek@apple.com>
Date: Sat Aug 25 01:27:48 2018 +0000

[llvm] Document "%T" as deprecated in CommandGuide/lit.rst

Differential Revision: https://reviews.llvm.org/D48842

llvm-svn: 340677

I went ahead and committed 4e813bbdf335626a741398314709a535ef52fe8e, which basically implements this functionality for libc++ only. A followup patch will cleanup the need for tracking file dependencies everywhere.

I think this review can be closed.

rG82f3c5d4a66 cross-ref so i can find this faster next time

MaskRay added a subscriber: MaskRay.Sep 3 2020, 3:45 PM

rG82f3c5d4a66 cross-ref so i can find this faster next time

@EricWF Seems that the revision can be abandoned now?

ldionne commandeered this revision.Sep 14 2020, 1:26 PM
ldionne edited reviewers, added: EricWF; removed: ldionne.

Commandeering so I can abandon to clean up the review queue.

ldionne abandoned this revision.Sep 14 2020, 1:27 PM

Abandoning since this was implemented in 4e813bbdf335626a741398314709a535ef52fe8e. Thanks @EricWF for the improvement, which turned out to simplify a bunch of things!