This is an archive of the discontinued LLVM Phabricator instance.

Add remote testing support to the lit config.
ClosedPublic

Authored by jroelofs on Feb 3 2015, 10:25 AM.

Details

Reviewers
danalbert
EricWF
Summary

This doesn't claim to support ShTest tests yet, but I've added in a hacked-together wrapper script remoteRun.py as one possible solution. I'm still not certain how to orchestrate executable copy-in for those cases.

Here I've also punted on what to do about test data files. The affected tests are:

libc++ :: std/input.output/file.streams/fstreams/filebuf.virtuals/pbackfail.pass.cpp
libc++ :: std/input.output/file.streams/fstreams/filebuf.virtuals/underflow.pass.cpp
libc++ :: std/input.output/file.streams/fstreams/ifstream.assign/member_swap.pass.cpp
libc++ :: std/input.output/file.streams/fstreams/ifstream.assign/move_assign.pass.cpp
libc++ :: std/input.output/file.streams/fstreams/ifstream.assign/nonmember_swap.pass.cpp
libc++ :: std/input.output/file.streams/fstreams/ifstream.cons/move.pass.cpp
libc++ :: std/input.output/file.streams/fstreams/ifstream.cons/pointer.pass.cpp
libc++ :: std/input.output/file.streams/fstreams/ifstream.cons/string.pass.cpp
libc++ :: std/input.output/file.streams/fstreams/ifstream.members/close.pass.cpp
libc++ :: std/input.output/file.streams/fstreams/ifstream.members/open_pointer.pass.cpp
libc++ :: std/input.output/file.streams/fstreams/ifstream.members/open_string.pass.cpp
libc++ :: std/input.output/file.streams/fstreams/ifstream.members/rdbuf.pass.cpp
libc++ :: std/localization/locales/locale.convenience/conversions/conversions.buffer/pbackfail.pass.cpp
libc++ :: std/localization/locales/locale.convenience/conversions/conversions.buffer/underflow.pass.cpp

I tested this on my mac with an SSHExecutor that connects to localhost, and copies executables into /tmp to get a feel for the overhead. That run took ~414s, whereas a normal one takes about half that at ~204s.

This isn't quite ready for commit, but it's a lot closer... There's still a few things I'd like to pick your brains on, @danalbert and @EricWF.

Diff Detail

Event Timeline

jroelofs updated this revision to Diff 19239.Feb 3 2015, 10:25 AM
jroelofs retitled this revision from to Add remote testing support to the lit config..
jroelofs updated this object.
jroelofs edited the test plan for this revision. (Show Details)
jroelofs added reviewers: danalbert, EricWF.
jroelofs added subscribers: Unknown Object (MLST), EricWF, danalbert.
jroelofs added inline comments.Feb 3 2015, 10:41 AM
remoteRun.py
8 ↗(On Diff #19239)

I considered passing this in as the first argument, and using eval. Better ideas?

jroelofs added inline comments.Feb 3 2015, 10:41 AM
libcxx/test/config.py
152

I forgot a

else:
    self.target_executor = None

here

libcxx/test/format.py
116–118

I kept these cases separate to avoid clashing with your configuration, @danalbert. It might be better to drop the else side of this, and have target_executor be a LocalExecutor instead of None when doing local testing. I'll save that decision for later to minimize invasiveness.

libcxx/test/remote.py
6 ↗(On Diff #19239)

I couldn't figure out how to get the include of lit.util to work here when using the wrapper script. It would be better to include that than to duplicate the code here.

lit.site.cfg.in
21

This part is obviously not supposed to go upstream, but I put it in the patch to show how to use it.

I think there are at least two common patterns that might be worth exposing through cmake:

config.target_executor = "SSHExecutor('username', 'hostname')"
config.target_executor = "PrefixExecutor('/path/to/run/script', LocalExecutor())"

Thoughts? Should we just pass the whole blob in on the cmake configure command line, or do something more sophisticated?

jroelofs updated this revision to Diff 19416.Feb 5 2015, 10:29 AM

Fix the LocalExecutor

jroelofs updated this revision to Diff 19418.Feb 5 2015, 10:33 AM

Set target_executor to None when there isn't one.

Another thing I haven't attempted to deal with in this patch is that there are a lot of assumptions that $host==$target for test parameter discovery. Need to clean those up at a later point.

test/libcxx/test/config.py
270 ↗(On Diff #19418)

I don't know where this came from. Bad merge maybe?

test/libcxx/test/remote.py
202 ↗(On Diff #19418)

This one is already deprecated. Going to remove it as soon as the lit timeouts patch goes in.

318 ↗(On Diff #19418)

This class screams that it wants reflection... I just don't know how to do that.

test/lit.site.cfg.in
21 ↗(On Diff #19418)

Again, I don't want to upstream this part... it's just an example.

test/remoteRun.py
1 ↗(On Diff #19418)

This file isn't used yet, but we could use it (or something like it) to to ShTest tests.

danalbert requested changes to this revision.Feb 5 2015, 2:27 PM
danalbert edited edge metadata.

I'm happy this is mostly unintrusive. I was expecting a bigger mess :)

If you decide not to go with my suggestions on L122, I'll probably have more to say about the executors themselves, otherwise I'll wait for the next revision since that will completely change them.

libcxx/test/format.py
97

No reason for this to be an argument, it's a member.

116–118

and have target_executor be a LocalExecutor instead of None when doing local testing

Was going to suggest exactly that. Should keep things cleaner and make sure neither side bit rots (because there will only be one side).

test/libcxx/test/config.py
14 ↗(On Diff #19418)

Should avoid wildcard imports.

123 ↗(On Diff #19418)

Ewwwwwww eval? Unless there's a really good reason to use an expression that can be evaluated over just naming a type, I think this should be:

mod_path, _, executor = exec_str.rpartition('.')
mod = importlib.import_module(mod_path)
self.target_executor = getattr(mod, executor)()

Where exec_str is the fully qualified type name (i.e. libcxx.test.remote.LocalExecutor). Then the wildcard import and the ugly eval can go away.

270 ↗(On Diff #19418)

Yes. ericwf added this the other day because it was something that went missing from the libcxxabi config (one test has a // REQUIRES: linux2)

test/libcxx/test/format.py
122 ↗(On Diff #19418)

I think this part of the logic could probably just be pushed down in to the executor. The parameters to exc.run() would become exec_path, source_path, env={}, valgrind_mess. exc.copy_in() becomes unnecessary, be the executor can decide for itself what should be copied. Same goes for exc.remote_from_local_dir() and exc.remote_from_local_file(), and exc.delete_remote(). This entire added block (and some the the existing code above) would become:

out, err, rc = executor.run(exec_path, source_path, self.exec_env, valgrind_mess)

That should also help with replacing target_executor = None with executor = LocalExecutor(), as each method of LocalExecutor would become a no-op.

This will probably cause some duplication between the various remote executors, so it might make sense to have a common base for all of them that factors that out.

149 ↗(On Diff #19418)

This should stay as it was. The target executor should clean up its own mess, but not the mess from the higher level (this is true with and without my suggestion at L122).

test/libcxx/test/remote.py
6 ↗(On Diff #19418)

What makes import hard here?

193 ↗(On Diff #19418)

This would be a nice way to factor out that cruft. Good thinking.

212 ↗(On Diff #19418)

Don't align assignments (you haven't matched all of them anyway).

223 ↗(On Diff #19418)

Your naming is inconsistent. self.scpCommand but self.remote_temp_files. I prefer PEP8 stlye (with underscores), but LIT already chose the anti-Python style, so let's try to stick with that. (I'm guilty of this crime too.)

255 ↗(On Diff #19418)
if local_file not in self.filemap:
306 ↗(On Diff #19418)

Use != for comparing strings rather than is not. is not should be used to test that two things are exactly the same object.

318 ↗(On Diff #19418)

You probably want something along the lines of http://stackoverflow.com/a/3467879/632035

Should make the whole class unnecessary.

This revision now requires changes to proceed.Feb 5 2015, 2:27 PM
jroelofs added inline comments.Feb 5 2015, 2:57 PM
test/libcxx/test/format.py
149 ↗(On Diff #19418)

Good point.

test/libcxx/test/remote.py
6 ↗(On Diff #19418)

when imported from remoteRun.py, the imports here don't work because we do funny business somewhere in the lit.cfg that adds the llvm imports to some path that python uses to search from.

I'll add the imports, drop this duped code, and drop the remoteRun.py. Can add that back and fix the import problem later when we try to get ShTest going.

212 ↗(On Diff #19418)

ok.

I really wish phabricator used a monospaced font for diffs.... haven't been able to find a setting that changes that.

223 ↗(On Diff #19418)

as in camelCase for vars, and underscore_separated_functions() ? Ok.

danalbert added inline comments.Feb 5 2015, 4:27 PM
test/libcxx/test/remote.py
223 ↗(On Diff #19418)

Actually, looking at lit's Test.py, it looks like the format is underscores for variables, camel-case for functions.

jroelofs updated this revision to Diff 19537.Feb 7 2015, 12:20 PM
jroelofs edited edge metadata.

Address most of @danalbert's comments. There are still a few remaining that I need to take care of though.

jroelofs added inline comments.Feb 7 2015, 12:25 PM
test/libcxx/test/config.py
126 ↗(On Diff #19537)

Drop this line.

123 ↗(On Diff #19418)

This means that the executors cannot be configured or chained via their constructor arguments, which is something I think is necessary to make, say, SSHExecutors work.... unless I start parsing the args, which leads to just re-inventing eval.

test/libcxx/test/format.py
122 ↗(On Diff #19537)

Still need to push most of this into target_executor.run

test/libcxx/test/remote.py
115 ↗(On Diff #19537)

!=

283 ↗(On Diff #19537)

I think this is much better, but it's also extremely complicated now. :/

danalbert added inline comments.Feb 9 2015, 2:10 PM
test/libcxx/test/config.py
123 ↗(On Diff #19418)

Ah, right. That makes sense. Carry on.

jroelofs updated this revision to Diff 19618.Feb 9 2015, 3:00 PM
jroelofs edited edge metadata.

Address the rest of @danalbert's comments

jroelofs updated this revision to Diff 19629.Feb 9 2015, 4:42 PM

Add TargetInfo class for determining things like locale support and platform name.

jroelofs updated this revision to Diff 19630.Feb 9 2015, 4:44 PM

Uploaded a diff from the wrong dir. Sorry for the spam.

jroelofs updated this revision to Diff 19631.Feb 9 2015, 4:46 PM

Remove something I was using for testing an idea. It didn't pan out.

jroelofs updated this revision to Diff 19632.Feb 9 2015, 4:51 PM

Make capitalization consistent.

jroelofs updated this object.Feb 10 2015, 7:06 AM
EricWF edited edge metadata.Feb 10 2015, 4:34 PM

Drive by comments.

test/libcxx/test/config.py
120 ↗(On Diff #19632)

Isn't this always set to the default by CMake?

122 ↗(On Diff #19632)

Remove this. Use the lit_config reporting methods.

test/libcxx/test/format.py
78 ↗(On Diff #19632)

Isn't this always true? Can we detect wether we have a local executor for the time being?

test/libcxx/test/remote.py
11–21 ↗(On Diff #19632)

I would throw a NotImplemented or similar error from these "abstract" methods

52 ↗(On Diff #19632)

I would probably throw from these abstract methods too if they shouldn't be called.

jroelofs updated this revision to Diff 19722.Feb 10 2015, 5:14 PM
jroelofs edited edge metadata.

Address @EricWF's driveby comments.

Looking better. At first glance, I think there's some more cleanup that can be done in remote.py, but I'll finish the review first thing tomorrow.

BTW, I like the TargetInfo bit. It looks like that change is actually cleanly separates from the rest of this. That part LGTM, so go ahead and submit that portion as its own commit.

test/libcxx/test/config.py
1 ↗(On Diff #19722)

Unused.

116 ↗(On Diff #19722)

Now that even the base case is a LocalExecutor, I think the target_ prefix of this is misleading.

120 ↗(On Diff #19722)

Default should be "LocalExecutor". Then the if/else can disappear.

128 ↗(On Diff #19722)

useValgrind should just go away. If the user wants valgrind, they can add the ValgrindExecutor themselves.

592 ↗(On Diff #19722)

Use parens rather than the \

And keep the not with the thing it's negating.

test/libcxx/test/format.py
89 ↗(On Diff #19722)

No reason to pass this as an argument. Just refer to it as self.target_executor in _evaluate_pass_test.

118 ↗(On Diff #19722)

The above three lines are just env = self.exec_env.

118 ↗(On Diff #19722)

Ignore the above. I traced it back and found that the default self.exec_env is {}, not None. Phabricator isn't letting me delete the comment though...

Here's my take on remote.py: https://gist.github.com/DanAlbert/a45ac580c3fc61917856

This patch no longer applies cleanly to ToT, and I don't know what revision it was generate against, so I wasn't able to apply this to my working tree to test it. Once it's rebased I'll verify.

jroelofs updated this revision to Diff 19860.Feb 12 2015, 3:03 PM

Rebase & use @danalbert's remote.py + tracing.py almost verbatim.

jroelofs updated this revision to Diff 19861.Feb 12 2015, 3:14 PM

Address (what I think are) the rest of @danalbert's comments on this patch.

I split the TargetInfo part into its own patch: http://reviews.llvm.org/D7601

This patch will need a slight rebase after that goes in.

libcxx/test/config.py
474

I think this needs to stay as lowercase, actually.

test/libcxx/test/config.py
120 ↗(On Diff #19632)

Oh yeah.

122 ↗(On Diff #19632)

oops

128 ↗(On Diff #19722)

This suggestion is a buildbot-breaking change. Can clean this up after any zorg bots that care about valgrind are moved over to using the new cmake flag.

jroelofs updated this revision to Diff 19913.Feb 13 2015, 12:06 PM

Fix the race condition in SSHExecutor, and leave a note saying to be careful.

jroelofs updated this revision to Diff 19915.Feb 13 2015, 12:08 PM

Didn't get all the files in the diff. Sorry for the spam.

jroelofs added inline comments.Feb 25 2015, 11:57 AM
libcxx/test/config.py
608

I think this is evidence that I need to rebase the patch. The target_info stuff already went in.

libcxx/test/tracing.py
6

delete this line.

danalbert accepted this revision.Feb 25 2015, 1:14 PM
danalbert edited edge metadata.

LGTM with the one nit.

Thanks a lot for taking care of this. Should greatly simplify my test setup.

libcxx/test/config.py
125

This isn't inferred, this is explicit. The else path is inferred.

This revision is now accepted and ready to land.Feb 25 2015, 1:14 PM
jroelofs closed this revision.Feb 25 2015, 4:44 PM

And with much rejoicing, there was r230592.