[PATCH] D129048: Rewording the "static_assert" to static assertion

Fri Jul 22 06:04:51 PDT 2022

aaron.ballman added a comment.

In D129048#3670212 <https://reviews.llvm.org/D129048#3670212>, @ldionne wrote:

> The libc++ CI runs pre-commit only, and it runs on all commits that touch something under `libcxx/`, `libcxxabi/`, `runtimes/` and `libunwind/`. In particular, it won't trigger if you make changes to `clang/` only -- that would create too much traffic and concretely it wouldn't help much, as we're rarely broken by Clang changes (this is an exception). Once the tests under `libcxx/` were touched, the libc++ CI triggered, I found this build for instance: https://buildkite.com/llvm-project/libcxx-ci/builds/12210. I do think it boils down to familiarity -- while the Clang pre-commit CI is sometimes overlooked because it fails spuriously, the libc++ CI usually doesn't, so when it fails, there's usually a good reason. I do understand why someone mostly familiar with the Clang workflow could think they needed to be overlooked, though.

Thanks for the explanation!

FWIW, the specific reason why I overloooked it was because of running against old versions of Clang -- I saw the diagnostic was correct in the test and presumed that the old clang was a bot configuration issue. It definitely doesn't help that Clang's precommit CI tends to be spuriously broken quite often; it's trained some of us reviewers to be more dismissive of failing precommit CI than we should.

> Concretely, libc++ supports the two latest releases of Clang, and so the CI tests against those. In fact, we run most of the tests using pre-installed Clangs since it allows us to bypass building Clang itself, which is a necessary optimization in the current setup. Regarding the tests themselves, I am able to find the following kind of output in the CI job that failed:

Yeah, which makes a lot of sense for libc++!

>   FAIL: llvm-libc++-shared.cfg.in :: std/numerics/numbers/illformed.verify.cpp (4388 of 7532)
>   [...]
>   error: 'error' diagnostics expected but not seen:
>     File /home/libcxx-builder/.buildkite-agent/builds/2b1c77dd9e90-1/llvm-project/libcxx-ci/build/generic-cxx2b/include/c++/v1/numbers Line * (directive at /home/libcxx-builder/.buildkite-agent/builds/2b1c77dd9e90-1/llvm-project/libcxx-ci/libcxx/test/std/numerics/numbers/illformed.verify.cpp:15): static assertion failed{{.*}}A program that instantiates a primary template of a mathematical constant variable template is ill-formed.
>   error: 'error' diagnostics seen but not expected:
>     File /home/libcxx-builder/.buildkite-agent/builds/2b1c77dd9e90-1/llvm-project/libcxx-ci/build/generic-cxx2b/include/c++/v1/numbers Line 83: static_assert failed due to requirement '__false<int>' "A program that instantiates a primary template of a mathematical constant variable template is ill-formed."
>   2 errors generated.
>    
>
> IMO this is reasonable output, in fact it's pretty much as good as we could have had. So I'm not taking any action item regarding improving the output of our CI, but I'm entirely open to suggestions if somebody has some.

I don't have a concrete suggestion for the CI output itself -- the failure was clear in retrospect, this was more a matter of not being familiar with the testing practices used.

> In D129048#3669568 <https://reviews.llvm.org/D129048#3669568>, @aaron.ballman wrote:
>
>> At a high-level, I think it boils down to familiarity. If we can get the libc++ CI to run as part of precommit CI (assuming precommit CI can be made to run with reliable results, which is a bit of a question mark),
>
> It is pretty reliable. However, I don't think it'll ever become reasonable to run the full libc++ CI on every Clang patch due to resource limitations.

The resource limitations are certainly a pragmatic thing for us to be concerned with, but I hope we're able to find some happy middle ground.

>> It would have also helped to catch the cause for the initial revert where everyone thought the patch was ready to land.
>
> 100% agreed, however this is blocked on the resource limitations mentioned above. I guess one thing we could do here is trigger a simple bootstrapping build of Clang and run the libc++ tests in a single configuration even on changes to `clang/`. That might catch most issues and perhaps that would be small enough to be scalable. I'll experiment with that after the release.

Thanks, that would be really beneficial. It's not just diagnostic wording changes that cause Clang developers problems with libc++ reverts; I've also seen C++20 work reverted because it broke libc++ without anyone in Clang knowing about it until after it landed (most recently, it's with the deferred concept checking work done by @erichkeane).

>> Another thing that would help would be to get the libc++ test bots into the LLVM lab (https://lab.llvm.org/buildbot/#/builders)
>
> Libc++ CI is pre-commit, the lab is post-commit only AFAICT.

Correct, but right now we have no community testing for libc++ whatsoever from the perspective of Clang folks. We only find out about failures when libc++ folks start reverting or pointing them out.

> I also don't know whether there's a way to bridge between BuildKite and BuildBot, and what that would look like. So while I share the desire to have a single place to look for all LLVM wide CI, I'm not sure it is possible given the variety of technologies used.

I don't know enough about the technology to know if it's easy or hard, unfortunately.

>> and ensure that they're sending failure emails to everyone on the blame list including people who land a patch on behalf of others.
>
> 100% agreed here -- this is one problem with our current setup.
>
>> It looks like we have one builder for libc++, but it doesn't appear to be working recently: https://lab.llvm.org/buildbot/#/builders/156.
>
> Yeah, this one needs to be removed, it's a remnant of when we used to have libc++ CI on BuildBot.
>
> Okay, so to summarize:
>
> 1. I'll experiment with a simple job that runs the libc++ pre-commit CI on every patch to Clang. We'll see if that scales.

Thanks, that would be awesome if it works out!

> 2. I'll look into sending blame emails from the ongoing (every 3h) job that runs the whole libc++ CI. This is basically equivalent to the buildbot functionality and it's arguably something that we're missing here. I have no idea how to do that, though, but it must be possible.

Thank you!!

> 3. Usually, for these types of failures, we actually notice and fix it on our end without requesting a revert. For this kind of change, my opinion is that the burden of fixing the code should have been on us, kind of like when the LLDB data formatters break because we made a totally legal change in libc++. Nobody jumped in to fix it on the libc++ side this time because (I assume) we're racing for the release, but normally I or someone else would have chimed in to fix this without annoying Clang.

FWIW, I've convinced myself that I agree with you here that the burden probably should have been on libc++ maintainers in this case. libc++ almost feels more like a downstream consumer of Clang in terms of testing needs, so I think when tests break because Clang diagnostic wording or behavior changes in a standards conforming way, libc++ folks should address it, but if Clang changes in a way that's not standards conforming, then Clang folks should address it. (Roughly speaking.) That said, I absolutely think we all need to continue to collaborate closely with one another as best we can when issues arise, and I really appreciate the discussion on how we can do that!

For this particular issue, I'd like @Codesbyusman to continue to try to fix the libc++ testing issues (it's good experience), but if that takes significantly longer (say, more than 8 hours of his effort), perhaps @ldionne or someone else from libc++ will have a moment to step in to help? (For reference, Usman is a Google Summer of Code mentee and this was his first patch for Clang. I think this has been a really good way for him to see how open source collaborations work in practice but I also don't want him to get bogged down too much if I can help it. Normally I'd step in to help directly, but I've been in WG14 meetings all week and so I am pretty swamped at the moment.)

> 4. That being said, let's try to spread the word that when the libc++ tests do fail on a Clang patch (which implies that `libcxx/` will have been touched until (1) is done), they must not be ignored.

Absolutely!

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129048