[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Mark de Wever via Phabricator via cfe-commits]

Mordante added a comment.

In D126907#3739923 <https://reviews.llvm.org/D126907#3739923>, @aaron.ballman wrote:

> In D126907#3739750 <https://reviews.llvm.org/D126907#3739750>, @Mordante wrote:
>
>> In D126907#3738383 <https://reviews.llvm.org/D126907#3738383>, @erichkeane wrote:
>>
>>> There was a test I dealt with previously where a ton of the header files were run with modules enabled (and an auto generated files), so I'm shocked to find there is MORE differences here.  @ldionne : This is another example where trying to test libcxx is particularly difficult to do apparently.
>>
>> I disagree; testing libcxx is easy.
>
> FWIW, that's not been my experience. I can't even get libcxx to build for me on Windows with Visual Studio cmake integration, let alone test it.

Fair point, I can't vouch for how well that build is supported, since I don't have access to Windows. This version isn't tested in libc++ CI. The other two Windows builds `CMake + ninja (MSVC)` and `CMake + ninja (MSVC)` are tested in libc++ CI. The wording on the website is similar to what is used in libc++ CI `libcxx/utils/ci/run-buildbot` but it misses the ` -DLIBCXX_EXTRA_SITE_DEFINES="_LIBCPP_HAS_NO_INT128"`.

>> Unfortunately what's missing is good documentation explaining how to test different configurations. A similar libc++ related issue came up in another Clang review recently where the libc++ CI setup was unclear. Afterwards I had a talk with @aaron.ballman and I agreed to improve the documentation. While improving the documentation I noticed some issues with our CI setup. So before publishing documentation that doesn't reflect reality, I first wanted to fix these issues. While testing my fixes I ran into the build failure due to this patch. So now we're at a full circle.
>
> And I greatly appreciate the improvements that have been going on here!

You're welcome!

In D126907#3739967 <https://reviews.llvm.org/D126907#3739967>, @erichkeane wrote:

> In D126907#3739750 <https://reviews.llvm.org/D126907#3739750>, @Mordante wrote:
>
>> If you need further assistance feel free to reach out to me or other libc++ developers, the easiest way to reach us is #libcxx on Discord.
>
> This is the 3rd time some non-obvious-how-to-run libcxx test failure has caused a revert request to this patch, which is obviously getting frustrating. Additionally, in none of the 3 cases was I alerted automatically.

I understand that it's frustrating. I haven't seen the other two reverts, but by default non libc++ patches aren't tested in the libc++ pre-commit CI. The reason is simple; a pre-commit build runs 50+ builds and takes about one hour to complete. So the CI doesn't have the capacity to test every Clang diff. If you want to run the libc++ pre-commit CI the easiest way is to add a dummy file in the libc++ tree. Note that will add the libc++ review group to this review.

Next to testing pre-commit changes it tests main every 4 hours, but doesn't send e-mails when an error occurs. This is something @ldionne wanted to look into.

This behaviour is exactly the kind of documentation that is missing for Clang developers and what @aaron.ballman and I talked about. (There is more documentation that should be added, but that is generic information.)

> I appreciate that there are actually ways to RUN these tests on my machine (Aaron is not as lucky, see above), but the lack of a "test all the libcxx things" flag is shocking.  It seems that I'd need at least 2 separate CMake directories to test these configurations?  That seems shocking to me.

You can use one libc++ installation to test multiple different configuration as long as they don't use different build-time options. So testing with the different language standards c++03, c++11, c++17, c++20, c++2b, the modular build, and several other options can be done from one build. But you need to invoke the tests for each option or combination of options separately.

If you want to test without `wchar_t` support you need to build a different libc++, simply since that removes parts of the dylib, like the `wchar_t` facets from the locales. (Basically the same as when you want to test Clang with or without assertions, there you need two builds too.)

What do you exactly mean with "test all the libcxx things" flag?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126907