This is an archive of the discontinued LLVM Phabricator instance.

[libcxx] Add better support for custom test runners.
ClosedPublic

Authored by danalbert on Nov 22 2014, 12:42 PM.

Details

Summary

I finally got around to merging the many, many changes to lit.cfg into
Android's libc++. This patch makes it simpler to actually use a custom
configuration and test format.

First, I've factored out _build, _run, and _clean methods from
_execute_test, since these are the likely parts that will need to be
overridden. This is likely a first step in the work jroelofs has been
doing with improving cross-compiling test execution.

Second, I've added a configuration_variant to the config. This
entry, if present, is a string that forms the prefix of the class that
is to be used to configure the test runner. For example, Android sets
config.configuration_variant = 'Android', and this causes an object
of type AndroidConfiguration to be constructed.

As an example of how this will be used, see:
https://android-review.googlesource.com/#/c/116022/

Diff Detail

Event Timeline

danalbert updated this revision to Diff 16530.Nov 22 2014, 12:42 PM
danalbert retitled this revision from to [libcxx] Add better support for custom test runners..
danalbert updated this object.
danalbert edited the test plan for this revision. (Show Details)
danalbert added reviewers: EricWF, jroelofs, mclow.lists.
danalbert added a subscriber: Unknown Object (MLST).
EricWF added inline comments.Nov 24 2014, 11:16 AM
test/lit.cfg
204

exec_path is always created so it should always be removed. If creating the file doesn't belong to a customization point then neither should cleaning it up.

Small nit: Will there ever be a case we want _clean(...) to throw? Should exception handling be the responsibility of _clean(...)?

529

Is there a prettier way to right this line? Just so I understand globals()['%sConfiguration' % cfg_variant] looks up and evaluates to a type with the name %sConfiguration?

danalbert added inline comments.Nov 24 2014, 11:32 AM
test/lit.cfg
204

The build step is the one that creates the file, and that _is_ customizable. Remote test executors need to customize the cleanup step because they need to remove the output from both the compilation location and the run location.

Yeah, I'm thinking _clean() worries about the exceptions, since the action taken might vary based on the internals (adb fails often, so probably just retry, whereas os.remove() will probably only fail for bad permissions, in which case just ignore).

529

I believe getattr(__module__, '%sConfiguration' % cfg_variant)(lit_config, config) would work too, but that isn't all that much better imo. At some point I'l like to teach LIT to add the directory of lit.cfg to PYTHONPATH so we can split some things out into different modules. Then this could be done in lit.site.cfg as from android import AndroidConfiguration as Configuration and lit.cfg would become

try:
    configuration = Configuration(lit_config, config)
except NameError:
    configuration = DefaultConfiguration(lit_config, config)
EricWF added inline comments.Nov 24 2014, 11:35 AM
test/lit.cfg
204

Isn't the file created by exec_file = tempfile.NamedTemporaryFile(suffix="exe", delete=False) on line 170? The _build step just overwrites it.

danalbert added inline comments.Nov 24 2014, 1:04 PM
test/lit.cfg
204

Still potentially have to deal with cleanup up remote paths. Also, in Android we generate a .o before linking (this is what our build system does, and I want to test our builds exactly).

EricWF added inline comments.Nov 24 2014, 1:31 PM
test/lit.cfg
204

Sure, I'm not saying scrap _clean as a customization point. I'm just wondering if we should always try and clean up exec_path since we always create it.

danalbert added inline comments.Nov 24 2014, 1:49 PM
test/lit.cfg
204

Eh. If you're overriding _clean(), it's your responsibility to call super if you want it to do the same things.

EricWF accepted this revision.Nov 24 2014, 1:56 PM
EricWF edited edge metadata.

LGTM modulo requested comments.

test/lit.cfg
204

I'll buy that. Can you add a comment so that is clear?

529

Please add a comment explaining this line.

This revision is now accepted and ready to land.Nov 24 2014, 1:56 PM
danalbert updated this revision to Diff 16582.Nov 24 2014, 2:21 PM
danalbert edited edge metadata.

Address review comments.

danalbert closed this revision.Nov 24 2014, 2:24 PM
danalbert updated this revision to Diff 16584.

Closed by commit rL222698 (authored by @danalbert).