[libcxx-commits] [PATCH] D119930: [libcxx] [test] Use proper UTF-8 locales on Windows

[Mark de Wever via Phabricator via libcxx-commits]

Mordante accepted this revision as: Mordante.
Mordante added a comment.

Great that we can test it in CMake, thanks!
LGTM modulo one small issue.



================
Comment at: libcxx/test/std/localization/locale.categories/category.ctype/locale.ctype.byname/tolower_1.pass.cpp:22
+// or Windows 11).
+// XFAIL: windows
+
----------------
ldionne wrote:
> mstorsjo wrote:
> > Mordante wrote:
> > > Would it be easy to detect the UCRT version in CMake and add an appropriate flag `libcpp-no-vcruntime`? Then we can use `UNSUPPORTED: libcpp-ucrt-has-utf8-bug`. That way others who want to test on Windows don't need to use the exact same version as the CI has.
> > > 
> > > When that's not possible I would prefer UNSUPPORTED over XFAIL. Again to avoid failing tests depending on which Windows platform they're executed on.
> > Hmm, unsure if it's possible to check the version of UCRT. But I guess we could do a proper runtime check when starting testing - like we already do to see if the system supports e.g. the locales we test. I'll give that a shot. (That can be used for other temporary platform bugs too.)
> > 
> > Yeah UNSUPPORTED is good if there's going to be variability - but with the drawback that it's easy to forget that in place for too long, when the issue is resolved in the CI environment. (While it's nice if the tests pass on any version of Windows, once the CI environment no longer require workarounds, I'd move towards removing them.)
> I think it's definitely better to use XFAIL than UNSUPPORTED, especially in these circumstances. XFAIL allows forgetting about this, since it'll start XPASSing when the issue gets fixed eventually. If we use UNSUPPORTED, we are basically dropping this test coverage forever, unless someone comes back and notices that.
> 
> I agree even better would be to have a version number -- then we can use either UNSUPPORTED or XFAIL, it doesn't matter, since it won't apply once we update to something newer.
My reason for UNSUPPORTED over XFAIL in this case, is that it depends on the version of the Windows system the user runs. So XFAIL would be nice for our CI, but will be annoying when a user tests it on a newer Windows than our CI.

In general I agree that an XFAIL is better or a UNSUPPORTED on a specific version.

But it seems that @mstorsjo found a way to detect it in CMake :-)


================
Comment at: libcxx/utils/libcxx/test/features.py:81
+          #include <locale.h>
+          #include <ctype.h>
+          int main(int, char**) {
----------------
This header provides `towlower`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D119930/new/

https://reviews.llvm.org/D119930