pre-merge checks are switching to buildkite build system

Hello friends,

We are switching the pre-merge test build system from Jenkins to Buildkite.
That will give authors and reviewers more transparency on what’s going on during the build process. For now only members of “pre-merge beta testing” [0] group are affected.

As usual, please tell us if something is off.

[0] https://reviews.llvm.org/project/view/78/

Mikhail

Firstly let me say that I love the pre-merge checks…but I recently saw something a little odd

A recent change I made to clang-format failed the pre-merge checks

https://results.llvm-merge-guard.org/BETA_amd64_debian_testing_clang8-1980/summary.html

This was because as part of the revision I clang-formatted one of the files with a build of clang-format that contained the fix I was making.

https://reviews.llvm.org/D80950

i.e. I was making a change to not just break between “XXX” << “XXX” just because it was 2 strings either side of “<<” and included as way of a demonstration the one other file in lib/Format that violated that rule (because we keep lib/Format 100% clang-format clean)

The failure from the pre-merge check was: (clang-format.patch)

diff --git clang/lib/Format/UnwrappedLineParser.cpp clang/lib/Format/UnwrappedLineParser.cpp
index 9c25e107d44…b8da2c23b55 100644
— clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -2744 +2744,2 @@ LLVM_ATTRIBUTE_UNUSED static void printDebugInfo(const UnwrappedLine &Line,

  • llvm::dbgs() << I->Tok->Tok.getName() << “[” << “T=” << I->Tok->getType()
  • llvm::dbgs() << I->Tok->Tok.getName() << “[”
  • << “T=” << I->Tok->getType()

Reading the documentation for the pre-merge checks it says this…

Linux

  1. Checkout of the branch (from apply patch)
  2. Guess which projects were modified, run Cmake for those.
  3. Build the binaries – ninja all
  4. Run the test suite – ninja check-all
  5. Run clang-format and clang-tidy on the diff.
  6. Upload build results to Phabricator

However could you clarify: if step v.

Run clang-format and clang-tidy on the diff.

Uses the clang-format/clang-tidy binaries either

a) built at step iii. or
b) if you use a pre-existing version?

If b) which version do you use?

a) last successful built
b) tip of existing committed master
c) last released version.

Many thank in advance

MyDeveloperDay.

Hi MyDeveloperDay,

We are using the released version of clang-format / clang-tidy (not necessarily the latest release). I think it makes sense to use most recent versions of the tools: https://github.com/google/llvm-premerge-checks/issues/196

Actually I’ve been thinking about this more, I wouldn’t expect the rest of the developers who are working in LLVM to be living at Trunk of the clang-format binary, using the last release say v10 at the time of writing seems reasonable.

However for those of us actually developing clang-format we are more than likely going to be using the latest and greatest and as such we’ll submit code (as I did here) which utilizes the latest changes/bug fixes.

It would be great if both worlds could exist, where we support the current tip of trunk for clang-format and the last version, for that to happen I feel the pre merge check would need to

  1. check using the last release of clang-format (v10 at time of writing)
  2. If 1) passes (then we are good)
  3. If 1) fails, then check the patch again with the last trunk version of clang-format (v11 at time of writing )
  4. if 3) passes then we are good
  5. if 3) also fails report the diffs from 1) as the failures

Otherwise I feel developers outside of clang-format will start getting clang-format warnings for a clang-format that they may not have access to, unless they build it themselves.

But also clang-format developers or those that like to use the latest clang-format will be able to ensure they are keeping clang-format area clean with the trunk version, if we don’t do this we can find that come a new release, there is a bunch of files that just start failing clang-format, and people don’t like a big clang-format only commits.

I hope this kind of approach seems sensible and reasonable in order to prevent false negatives from the pre-merge checking.

MyDeveloeprDay.

Actually I’ve been thinking about this more, I wouldn’t expect the rest of the developers who are working in LLVM to be living at Trunk of the clang-format binary, using the last release say v10 at the time of writing seems reasonable.

However for those of us actually developing clang-format we are more than likely going to be using the latest and greatest and as such we’ll submit code (as I did here) which utilizes the latest changes/bug fixes.

It would be great if both worlds could exist, where we support the current tip of trunk for clang-format and the last version, for that to happen I feel the pre merge check would need to

  1. check using the last release of clang-format (v10 at time of writing)
  2. If 1) passes (then we are good)
  3. If 1) fails, then check the patch again with the last trunk version of clang-format (v11 at time of writing )
  4. if 3) passes then we are good
  5. if 3) also fails report the diffs from 1) as the failures

Otherwise I feel developers outside of clang-format will start getting clang-format warnings for a clang-format that they may not have access to, unless they build it themselves.

If pre-merge is using the latest release, then why would developers need to build it themselves?

But also clang-format developers or those that like to use the latest clang-format will be able to ensure they are keeping clang-format area clean with the trunk version, if we don’t do this we can find that come a new release, there is a bunch of files that just start failing clang-format, and people don’t like a big clang-format only commits.

I think we’re only running clang-format on the diff, not on complete files anyway.

We have not noticed any significant issues with Buildkite migration and now ALL pre-merge checks for diff reviews are running on Buildkite!