This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Refactor test components into modules.
ClosedPublic

Authored by danalbert on Jan 8 2015, 12:22 PM.

Details

Summary

I've moved the bulk of lit.cfg into test/libcxx/testconfig.py and
test/libcxx/testformat.py. All that remains in lit.cfg is the
logic to discover lit.site.cfg if lit.cfg was run directly, and the
logic for loading configuration variants.

The configuration variant flow has changed with this patch. Rather
than instantiating an object of type <VARIANT>Configuration, we now
instatiate an object of type Configuration that was loaded from the
module <VARIANT>.testconfig.py.

This has to be done on a per-project basis rather than in LIT itself
because LIT doesn't actually know where the real test directory is,
only where the site configuration is (which is usually in the output
directory). It's simple enough to do though, so it's fine to require
each project to do it themselves.

I also cleaned up all the pylint issues while I was here, which was
mostly just a matter of fixing long lines.

Diff Detail

Event Timeline

danalbert updated this revision to Diff 17906.Jan 8 2015, 12:22 PM
danalbert retitled this revision from to [libc++] Refactor test components into modules..
danalbert updated this object.
danalbert edited the test plan for this revision. (Show Details)
danalbert added reviewers: EricWF, jroelofs, mclow.lists.
danalbert set the repository for this revision to rL LLVM.
danalbert added a subscriber: Unknown Object (MLST).
EricWF edited edge metadata.Jan 8 2015, 12:25 PM

Whooo! I'm super happy to see this patch! Lets do some bike-shedding!

I would like to see the test module go into a directory by itself. Should this directory live in libcxx/utils or libcxx/test?
What should we name the module? I like LibcxxTest.

Once testconfig.py and testformat.py live in a subdirectory can we drop the test prefix?

I would like to see the test module go into a directory by itself. Should this directory live in libcxx/utils or libcxx/test?
What should we name the module? I like LibcxxTest.

Once testconfig.py and testformat.py live in a subdirectory can we drop the test prefix?

I had considered using libcxx.test.format/libcxx.test.config. I was sort of leaning toward that anyway. I'll make the change.

danalbert updated this revision to Diff 17908.Jan 8 2015, 12:41 PM
danalbert edited edge metadata.

Move libcxx.test* modules into libcxx.test.*.

EricWF accepted this revision.Jan 8 2015, 1:32 PM
EricWF edited edge metadata.

Just a nit, could we give the python module a more descriptive name than test? This change will make it possible to share libc++'s configuration and format with libc++abi. When using it in libc++abi the name test will be quite vague.

I'm also not sure I like the libcxx root being a python module. What is the benefit of that?

Also I would introduce a .gitignore into the top of libc++ so git users don't see the generated .pyc files.

This revision is now accepted and ready to land.Jan 8 2015, 1:32 PM
EricWF added a comment.Jan 8 2015, 1:53 PM

Just a nit, could we give the python module a more descriptive name than test? This change will make it possible to share libc++'s configuration and format with libc++abi. When using it in libc++abi the name test will be quite vague.

I'm also not sure I like the libcxx root being a python module. What is the benefit of that?

Woops, I misunderstood the layout of this patch. I would still like to see the configuration live somewhere else. The currently layout seems needlessly complicated. Also I think it could live in libcxx/test instead of libcxx/test/libcxx, leaving libcxx/test/libcxx to only contain tests.

Also, @jroelofs doesn't want any generated files. Could we use sys.dont_write_bytecode = True in lit.cfg to turn this off? This way SVN users won't have to worry about generated .pyc files either, and we keep the source directory clean.

jroelofs edited edge metadata.Jan 8 2015, 2:02 PM

Yeah... it would be really nice to be able to check out libcxx into a directory, then mark it all read-only, then build. *.pyc's get in the way of that (though I'm ok with using the '-B' python flag, or sys.dont_write_bytecode = True).

Woops, I misunderstood the layout of this patch. I would still like to see the configuration live somewhere else. The currently layout seems needlessly complicated. Also I think it could live in libcxx/test instead of libcxx/test/libcxx, leaving libcxx/test/libcxx to only contain tests.

I had considered adding them directly to <libcxx-root>/test/ and having the sys.path be <libcxx-root>/.., but since the root isn't guaranteed to be named libcxx, you can't reliably import it. We can't set the sys.path to <libcxx-root>, because then they would be imported as import test.libcxxconfig (or whatever), and we need a more unique name for our top level package (there is already a test package). If we're willing to say that your top level directory must be named libcxx, then I'd be happy adding them directly to <libcxx-root>/test.

Also, @jroelofs doesn't want any generated files. Could we use sys.dont_write_bytecode = True in lit.cfg to turn this off? This way SVN users won't have to worry about generated .pyc files either, and we keep the source directory clean.

Ah, my mistake. I have .py[cod] in my global .gitignore, so I hadn't noticed it. I'll add the local .gitignore and be sure to set the svn props.

danalbert updated this revision to Diff 17913.Jan 8 2015, 2:29 PM
danalbert edited edge metadata.

Add a gitignore for Python things.

EricWF added a comment.Jan 9 2015, 9:58 AM

Changes LGTM.

danalbert closed this revision.Jan 9 2015, 10:04 AM

Closed by r225532 (arc for some reason left out that detail).