Bug 151239 – This is why I love or hate the commit queue

• Component: Tools / Tests
• Status: NEW

[Geoffrey Garen]

Let's document our commit queue experiment results here.

[Sergio Villar Senin]

I always use webkit-patch land directly but decided to join the experiment and started to use webkit-patch land-safely.

The very first thing I dislike is the fact that I need to specify the reviewer with "-r". The command should get it from the bug the same as webkit-patch land does.

[Ryosuke Niwa]

(In reply to comment #1)
> I always use webkit-patch land directly but decided to join the experiment
> and started to use webkit-patch land-safely.
> 
> The very first thing I dislike is the fact that I need to specify the
> reviewer with "-r". The command should get it from the bug the same as
> webkit-patch land does.

Are you using webkit-patch land-safely?  That should automatically get the reviewer name from the bug.

[Carlos Alberto Lopez Perez]

One thing that I dislike about the commit queue is that you have to wait a long time (more than 20 minutes) to see your patch landed.

And that (at least for me) is an inconvenient because I want to check all the bots after my patch lands to ensure I didn't broke something.

The bots already take a long time in processing the patch once it lands. If I have to add another 20-60 minutes just because of wanting to use the commit-queue, that makes the whole thing not practical.

What is the rationale of making the commit queue take so long to land a patch once it is marked cq+? IMHO it should take as few as possible (less than 5 minutes in the worst case).

[Michael Catanzaro]

The point of the commit-queue is to check for you that you haven't broken anything. If you can't trust commit-queue to do that, then there's no point to using commit-queue, right?

[Ryosuke Niwa]

The commit queue right now applies and builds your patch.

[Martin Robinson]

Perhaps the issue is that as of right now, the GTK+ commit-queue does not run any tests.

[Martin Robinson]

(In reply to comment #6)
> Perhaps the issue is that as of right now, the GTK+ commit-queue does not
> run any tests.

Er. I meant the EWS here, of course. It's also true that things can break between EWS time and commit time. A commit-queue that prevents commits that breaks tests could be quite nice, but right now our tests are so fragile it could be an issue.

[Carlos Alberto Lopez Perez]

(In reply to comment #5)
> The commit queue right now applies and builds your patch.

I guess it only does that for the Mac port.

Will it be possible that it also builds the patch for the GTK/EFL ports?
We can setup infrastructure for the GTK port if needed.

[Carlos Alberto Lopez Perez]

(In reply to comment #7)
> (In reply to comment #6)
> > Perhaps the issue is that as of right now, the GTK+ commit-queue does not
> > run any tests.
> 
> Er. I meant the EWS here, of course. It's also true that things can break
> between EWS time and commit time. A commit-queue that prevents commits that
> breaks tests could be quite nice, but right now our tests are so fragile it
> could be an issue.

If it only prevented the build of the GTK port from being broken, that would be already an incredible improvement.

[youenn fablet]

land-safely option does not like bug 151190 patch.
This patch removes some empty files which may cause some trouble, as noted in bug 29684.

[Benjamin Poulain]

I had to land some stuff manually today. We need cq to be faster.

I had 3 patches with dependencies with each other. After 1.5 hour, the first was till not landed by cq.

[youenn fablet]

> What is the rationale of making the commit queue take so long to land a
> patch once it is marked cq+? IMHO it should take as few as possible (less
> than 5 minutes in the worst case).

Agreed, it seems EWS bots that do build and test tasks are as fast as CQ bots.
What prevents CQ bots from being faster?

[Ryosuke Niwa]

Commit queue applies your changes and builds the entire WebKit before landing a patch.

[Alexey Proskuryakov]

We should upgrade commit queue hardware. I'll look into that, but it won't be immediate.

[Alex Christensen]

https://bugs.webkit.org/show_bug.cgi?id=151418 failed in the commit queue because "Geoff Garen" is not in the list of reviewers.  "Geoffrey Garen" is.  At least the commit queue failed quickly.

[Alex Christensen]

https://bugs.webkit.org/show_bug.cgi?id=151418 finally passed the commit queue, but broke the GTK build and broke API tests.

[Csaba Osztrogonác]

(In reply to comment #16)
> https://bugs.webkit.org/show_bug.cgi?id=151418 finally passed the commit
> queue, but broke the GTK build and broke API tests.

CQ can break the build, because folks want this behaviour:
- https://lists.webkit.org/pipermail/webkit-dev/2014-January/026068.html
- https://bugs.webkit.org/show_bug.cgi?id=127079

I don't agree with this, but I have only one vote. :(

Previously a build failure (red EWS bubble) set cq- flag immediately
to avoid breaking builds. But it didn't help if an eager reviewer
set r+/cq+ immediately after the patch is uploaded, leaving no
chance to the EWS bot to run. I remember, it happened many times.

[Ryosuke Niwa]

I didn't use commit queue because none of my changes to Websites/perf.webkit.org affect WebKit behaviors, and running layout tests is useless.

[Michael Catanzaro]

I'm back to only using the commit queue from the flag in Bugzilla, and only as a shortcut so I don't have to 'webkit-patch apply-from-bug' then 'webkit-patch land'.

The commit-queue seems borderline useless to me. It doesn't run tests, except on Mac (where it is useful to catch test failures), but it's extremely rare that I break Mac tests. It doesn't catch build breaks, except on Mac, but I already have to watch EWS to make sure I don't break Windows and EFL, so this is useless. If I'm making a change I think might break tests, I have to watch the test board since I know commit-queue probably won't catch it.

commit-queue would be useful if I could rely on it to catch test failures and build breakage.

We could hypothetically add a new flag, say cq++:

* cq+, to be used normally, says "commit unless it breaks a build or test on any port." Then this would be useful as it would replace the need to check EWS for failures.
* cq++ says "commit even if it breaks a non-Mac port," to be used only after cq+ has failed. When cq++ gets set, relevant port maintainers get CCed to the bug automatically.

The cost of cq++ to Apple developers would be that it takes twice as long to commit via commit-queue if the patch breaks another port, but it would make commit queue useful for the rest of us.

[youenn fablet]

The commit queue these days takes time due to some flakiness tests.
This makes it sometimes very slow.

Instead of running tests which the EWS already did/can already do, I am wondering whether the commit queue should not run different checks, like running binding tests and unit tests.

[Carlos Alberto Lopez Perez]

I think the commit queue should not run any test at all. But just require that all the EWS bots on that patch have finished on green status. Or that no EWS bot have finished in red status (to avoid blocking the patch if some EWS is down)

And if some developer wants to commit the patch anyway (knowing that their patch broke some EWS bot) they can always commit directly via svn.

That way commit queue will be very fast (no test to run, so it can commit in a matter of seconds) and the coverage for the quality of the patch that is being committed will be better than now, because the EWS bots run tests on more ports than just the Mac one.

[Ryosuke Niwa]

Yeah, waiting for all EWS and only building on commit queue would speed things up considerably.

[Antonio Gomes]

(In reply to comment #22)
> Yeah, waiting for all EWS and only building on commit queue would speed
> things up considerably.

I have decided to use CQ+ check in bugzilla more these days, and I agree that if EWK is all green (main builds are fine and tests pass on Mac), CQ+ should only build and commit the patch.

Maybe a thread in webkit-dev ML would allow us to actually move forward and get consensus about CQ behavior.

[Alexey Proskuryakov]

FWIW, I'm looking into using faster hardware for the CQ.