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

Thu Jul 21 15:55:13 PDT 2022

ldionne added a comment.

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.

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:

  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.

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.

> 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.

> 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. 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.

> 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.
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.
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.
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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129048