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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
