This is an archive of the discontinued LLVM Phabricator instance.

[libcxx] Add locales to available_features for tests.
ClosedPublic

Authored by danalbert on Aug 1 2014, 3:06 PM.

Details

Summary

Linux has a lot of failures caused by not having support for certain locales. Since these come out as a lot of noise in the test results, have lit.cfg detect the presence of the various locales used in the tests and add them to config.available_features as locale.LOCALE_NAME.

This patch also adds REQUIRES: locale.REQUIRED_LOCALE to every test that I saw failing in this manner. We probably need to add more for all the tests requiring en_US.UTF-8, but we can do that on an as-needed basis.

One thing that concerns me is how many tests get skipped because of missing locales (especially in regex/). We should make a point of splitting up any tests that test default behavior _and_ behavior under a given locale so that we aren't losing coverage for default behavior.

Diff Detail

Event Timeline

danalbert updated this revision to Diff 12120.Aug 1 2014, 3:06 PM
danalbert retitled this revision from to [libcxx] Add locales to available_features for tests..
danalbert updated this object.
danalbert edited the test plan for this revision. (Show Details)
danalbert added reviewers: mclow.lists, EricWF.
danalbert added a subscriber: Unknown Object (MLST).
EricWF edited edge metadata.Aug 1 2014, 4:00 PM

I'm super happy to see this patch. I had just been skipping the localization tests all together on machines that lacked these locales. I also like the idea of splitting up the test by locale and will lend a hand to do so. The patch itself looks good but I have some questions about its general direction.

  1. Do you understand why these specific locales where chosen for testing? (I dont). I'm concerned that we might are marginalizing important tests.
  2. As more LIT features become integrated in the testsuite whats the best way to prevent fragmentation between LIT and testit? (does testit even work on linux?)

As an addendum to this patch I plan to add documentation to the website about the locales required to run the testsuite. Since its possible for all of these tests to be supported within a linux-gnu environment I think we need to document that. I've been meaning to add this documentation for a while and this patch forces my hand.

test/lit.cfg
192–193

Should we emit a warning here about the missing locale?

I believe these locales were chosen simply because they can be considered fairly common locales, but I'm only guessing.

IMO, the right way to solver fragmentation between LIT and testit is to remove testit from the tree. It doesn't work on Linux out of the box, but can be made to work with some modification. While that is easily fixed, it's non-parallel, doesn't have very helpful output, and doesn't handle XFAIL in the first place, let alone REQUIRES.

Adding documentation for requirements to the website would be great.

On a somewhat related note, I'm really hoping to finally look at zorg one of these days so we can have buildbots set up (for Linux, FreeBSD, and Mac, at the very least). That way we can do away with the manually generated and uploaded test results and just link to the latest buildbot results.

test/lit.cfg
192–193

That's a good idea. LIT will mention that the tests are unsupported, but it isn't explicit about why.

EricWF added a comment.Aug 1 2014, 4:40 PM

As much as I would like to take testit out back and shoot it, previous conversations with @mclow.lists leads me to believe its still widely used. Since lit usually isn't available outside of the LLVM repo I'm not sure that removing testit is viable at this point.

I would like to see LIT become a standalone "first-class" binary so it could be easier to use outside of makefile rules. Daniel Dunbar expressed some thoughts to this end a while ago. I will follow those up with him.

My current workflow is something like:

alias lit="python /path/to/llvm/utils/lit/lit.py"
cp libcxx-build/lit.site.cfg libcxx/test/lit.site.cfg

Until its as easy to use LIT in the source tree as it is the testit script I don't think we can even consider removing it.

danalbert updated this revision to Diff 12125.Aug 1 2014, 4:40 PM
danalbert edited edge metadata.

Emit warnings about unsupported tests when checking required locales.

In D4759#6, @EricWF wrote:

As much as I would like to take testit out back and shoot it, previous conversations with @mclow.lists leads me to believe its still widely used. Since lit usually isn't available outside of the LLVM repo I'm not sure that removing testit is viable at this point.

I would like to see LIT become a standalone "first-class" binary so it could be easier to use outside of makefile rules. Daniel Dunbar expressed some thoughts to this end a while ago. I will follow those up with him.

My current workflow is something like:

alias lit="python /path/to/llvm/utils/lit/lit.py"
cp libcxx-build/lit.site.cfg libcxx/test/lit.site.cfg

Until its as easy to use LIT in the source tree as it is the testit script I don't think we can even consider removing it.

I wonder how much of the reason that testit is more widely used is because it's the most obvious path to running the tests. I know when I first started working with the project I had no idea that LIT was even there (or what it was, for that matter). The libc++ docs make no mention of how to use LIT for the tests (by contrast, it's now the only thing mentioned for libc++abi, albeit indirectly though the cmake target).

Still, I'll concede that removing testit isn't something we can do just yet.

EricWF accepted this revision.Aug 1 2014, 6:49 PM
EricWF edited edge metadata.

LGTM. I would suggest making the warning mention that it is a locale that is not supported. Otherwise it seems unclear.

This revision is now accepted and ready to land.Aug 1 2014, 6:49 PM
danalbert updated this revision to Diff 12137.Aug 1 2014, 7:19 PM
danalbert edited edge metadata.

Oops. Previous patch would only work on Linux. Now support Darwin and Windows as
well (although I haven't tested them).

EricWF added a comment.Aug 1 2014, 7:23 PM

I can test this on Mac (and have tested on linux). Can anybody test against Windows?

I pulled the locale names for Windows out of test/support/platform_support.h, so presumably it will work. Probably fine to just check in and fix it up later on the off chance it is broken.

danalbert closed this revision.Aug 4 2014, 11:54 AM

I've tested on Linux and Mac. Submitted as r214753. We can clean up any Windows locale name issues if they crop up.