Codesbyusman added a comment. In D129048#3671568 <https://reviews.llvm.org/D129048#3671568>, @aaron.ballman wrote:
> 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.) Yes for me as a complete be beginner was great to see how really open source works. I am working on the libc++ and used regex for the passing of test of previous versions. That is almost complete. >> 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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
