Mikhail Goncharov via llvm-dev
2020-Jun-04 10:09 UTC
[llvm-dev] pre-merge checks are switching to buildkite build system
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 Kind regards, Mikhail On Wed, Jun 3, 2020 at 3:40 PM MyDeveloper Day <mydeveloperday at gmail.com> wrote:> 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. > > > > > On Wed, Jun 3, 2020 at 1:40 PM Mikhail Goncharov via llvm-dev < > llvm-dev at lists.llvm.org> wrote: > >> 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/ >> >> Kind regards, >> Mikhail >> _______________________________________________ >> LLVM Developers mailing list >> llvm-dev at lists.llvm.org >> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev >> >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20200604/b5032d11/attachment.html>
MyDeveloper Day via llvm-dev
2020-Jun-05 10:38 UTC
[llvm-dev] pre-merge checks are switching to buildkite build system
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. On Thu, Jun 4, 2020 at 11:09 AM Mikhail Goncharov <goncharov at google.com> wrote:> 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 > > Kind regards, > Mikhail > > > On Wed, Jun 3, 2020 at 3:40 PM MyDeveloper Day <mydeveloperday at gmail.com> > wrote: > >> 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. >> >> >> >> >> On Wed, Jun 3, 2020 at 1:40 PM Mikhail Goncharov via llvm-dev < >> llvm-dev at lists.llvm.org> wrote: >> >>> 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/ >>> >>> Kind regards, >>> Mikhail >>> _______________________________________________ >>> LLVM Developers mailing list >>> llvm-dev at lists.llvm.org >>> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev >>> >>-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20200605/13354661/attachment.html>
Mehdi AMINI via llvm-dev
2020-Jun-05 20:33 UTC
[llvm-dev] pre-merge checks are switching to buildkite build system
On Fri, Jun 5, 2020 at 3:38 AM MyDeveloper Day via llvm-dev < llvm-dev at lists.llvm.org> wrote:> 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.> > I hope this kind of approach seems sensible and reasonable in order to > prevent false negatives from the pre-merge checking. > > MyDeveloeprDay. > > > > > On Thu, Jun 4, 2020 at 11:09 AM Mikhail Goncharov <goncharov at google.com> > wrote: > >> 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 >> >> Kind regards, >> Mikhail >> >> >> On Wed, Jun 3, 2020 at 3:40 PM MyDeveloper Day <mydeveloperday at gmail.com> >> wrote: >> >>> 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. >>> >>> >>> >>> >>> On Wed, Jun 3, 2020 at 1:40 PM Mikhail Goncharov via llvm-dev < >>> llvm-dev at lists.llvm.org> wrote: >>> >>>> 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/ >>>> >>>> Kind regards, >>>> Mikhail >>>> _______________________________________________ >>>> LLVM Developers mailing list >>>> llvm-dev at lists.llvm.org >>>> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev >>>> >>> _______________________________________________ > LLVM Developers mailing list > llvm-dev at lists.llvm.org > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20200605/2e8b3732/attachment.html>