[clang] [llvm] Enable LLDB tests in Linux pre-merge CI (PR #94208)
Aaron Ballman via cfe-commits
cfe-commits at lists.llvm.org
Tue Jun 4 11:12:41 PDT 2024
AaronBallman wrote:
> I would not agree with this specific wording. We have in the past and will in the future revert commits that break existing tests elsewhere in the LLVM project. If a patch to Clang breaks an existing test in LLDB, it's up to the author of the patch to work with the LLDB project members to find path forward so the patch can be merged without introducing a breakage on the bots. The specifics of that will depend on the situation (it could be supporting the feature in LLDB, changing the patch to Clang, updating an overconstrained test, XFAILing a test if there's a really good reason to do so, ...), but it's generally not okay to land a patch that breaks existing tests in the LLVM project.
It's good that we have this discussion then, thank you! FWIW, this scenario is the kind of one I am talking about:
> I have had clang patches reverted only because they broke lldb tests on pure diagnostics / type printing improvements.
I have also had this exact experience and this is the scenario we need to avoid. We (lldb + clang folks) had discussed this previously (I can try to dig up the link if that's helpful) and the eventual decision we came to was that conforming changes to Clang which break lldb do not always qualify for an automatic revert because a bot went red. e.g., Clang can change the wording of a diagnostic and a monorepo "downstream" (like lldb or libc++) may need to react to address the issue without reverting the Clang change. That said, there's definitely an expectation that the Clang developer will let the lldb (or libc++, etc) folks know about the changes and try to avoid disruption as much as possible.
To hopefully help focus the need (not exhaustive lists):
Revert is appropriate when:
* lldb testing exposes a crash or assert in Clang
* the change in Clang is semantically incorrect
A revert is not appropriate when:
* lldb tests fail because AST printing has changed in Clang and the printed output is valid
* lldb tests fail because the wording of a diagnostic has changed in Clang
Examples of gray area are:
* Clang changes the layout of an object and lldb tests start failing
* Clang changes and an lldb test fails but it's not clear why the test is now failing
The key takeaway is: lldb bots going red because of changes in Clang *does not* qualify for an automatic revert in all circumstances, but definitely does qualify in others. To feel comfortable adding lldb to Clang's precommit CI, we need to ensure that it's clear to contributors (including folks new to the community) which failures qualify as "your patch cannot be merged" and which failures qualify as "please let lldb folks know they need to react to these changes".
https://github.com/llvm/llvm-project/pull/94208
More information about the cfe-commits
mailing list