This is an archive of the discontinued LLVM Phabricator instance.

[libc++][test] Use the Japanese locale.
ClosedPublic

Authored by Mordante on Mar 30 2022, 8:35 AM.

Details

Reviewers
ldionne
Group Reviewers
Restricted Project
Commits
rGd49c0ba5abde: [libc++][test] Use the Japanese locale.
Summary

This change is done to see whether all platforms have a CI with the
Japanese locale installed.

This wires in the locale in the tests and uses it in one test. This is
a preparation for the tests of the chrono formatters.

Diff Detail

Event Timeline

Mordante created this revision.Mar 30 2022, 8:35 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 30 2022, 8:35 AM
Mordante requested review of this revision.Mar 30 2022, 8:35 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 30 2022, 8:35 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Mordante planned changes to this revision.Mar 30 2022, 8:38 AM

Only put up for review to get an initial view which CI jobs have a Japanese locale installed.

libcxx/test/std/localization/locale.categories/category.time/locale.time.put.byname/put1.pass.cpp
15

Process this TODO.

libcxx/utils/ci/buildkite-pipeline.yml
20

Process this TODO.

Mordante updated this revision to Diff 419479.Mar 31 2022, 8:35 AM

Add debug info to look at unexpected CI failures.

mstorsjo added inline comments.Mar 31 2022, 12:22 PM
libcxx/test/std/localization/locale.categories/category.time/locale.time.put.byname/put1.pass.cpp
87

FWIW in other locale tests, we've used unicode escapes like \u0123 instead of spelling them out as utf8 in the source.

Mordante planned changes to this revision.Mar 31 2022, 12:37 PM

The Linux CI changes are committed, but it will take some time before they are propagated to the build nodes.
I will look in a little while whether the propagation has been completed.

libcxx/test/std/localization/locale.categories/category.time/locale.time.put.byname/put1.pass.cpp
87

Good point. I'll address that in the next revision.

Mordante updated this revision to Diff 420569.Apr 5 2022, 10:10 AM

Address a review comment.
Rebased to see whether the CI Docker images are updated.

Mordante planned changes to this revision.Apr 5 2022, 10:10 AM
Mordante marked an inline comment as done.
Mordante updated this revision to Diff 420901.Apr 6 2022, 8:51 AM

Try to fix the CI.

Mordante updated this revision to Diff 420941.Apr 6 2022, 10:30 AM

Attempt to fix CI

Mordante updated this revision to Diff 420945.Apr 6 2022, 10:57 AM

Fix a copy paste error.

Mordante updated this revision to Diff 421220.Apr 7 2022, 8:03 AM

Test requires is used.

Mordante planned changes to this revision.Apr 7 2022, 8:36 AM
Mordante updated this revision to Diff 421272.Apr 7 2022, 10:27 AM

Rebased.
The previous runs are as expected, prepare for review by removing unwanted changes.

Mordante edited the summary of this revision. (Show Details)Apr 7 2022, 10:33 AM

This is ready for review, assuming the CI passes ;-)

libcxx/test/std/localization/locale.categories/category.time/locale.time.put.byname/put1.pass.cpp
14

For info I tested with this value set and an assert(false); in the tests to verify all platforms support Japanese. Only on AIX it's unsupported. However this platform has a lot (all?) locale tests disabled.

The build can be found here
https://buildkite.com/llvm-project/libcxx-ci/builds/10064

Mordante retitled this revision from [libc++] Use the Japanese locale. to [libc++][test] Use the Japanese locale..Apr 7 2022, 10:35 AM
Mordante updated this revision to Diff 421335.Apr 7 2022, 1:44 PM

Rebased to test status.

Mordante updated this revision to Diff 421531.Apr 8 2022, 8:16 AM

Rebase to trigger CI.

ldionne accepted this revision.Apr 8 2022, 10:53 AM

LGTM, but have you verified that we were running that test in the CI? Since it REQUIRES: locale.ja_JP.UTF-8, we would simply skip it if the CI node didn't support that locale. Please make sure that's not what's happening, as we would simply lose coverage of that test.

This revision is now accepted and ready to land.Apr 8 2022, 10:53 AM

LGTM, but have you verified that we were running that test in the CI? Since it REQUIRES: locale.ja_JP.UTF-8, we would simply skip it if the CI node didn't support that locale. Please make sure that's not what's happening, as we would simply lose coverage of that test.

Thanks for the review!

I indeed have verified that, per https://reviews.llvm.org/D122738#inline-1180541

For info I tested with this value set and an assert(false); in the tests to verify all platforms support Japanese. Only on AIX it's unsupported. However this platform has a lot (all?) locale tests disabled.

The build can be found here
https://buildkite.com/llvm-project/libcxx-ci/builds/10064
This revision was automatically updated to reflect the committed changes.