Bug 74776 – Commit queue building & running tests on Chromium shouldn't land JSC or WK2 changes

• Status: RESOLVED
• Resolution: INVALID
• Component: Tools / Tests

[Ryosuke Niwa]

Commit queue landed a patch for https://bugs.webkit.org/show_bug.cgi?id=74767 but broke builds because it doesn't use JSC at all.

Ideally, we'll have a commit queue building on non-Chromium port to land JSC changes. It can at least not land JSC changes when running on Chromium port.

[Eric Seidel (no email)]

Do you have a suggested port to use?  Ideally the port would build on Linux and have a very high "uptime" in terms of building/tests-passing.  The linux part is because linux machines are easy to acquire/admin, and the buildability/testability part is for up-time of the queue.

It's possible we could just call build-javascriptcore of whatever port.

[Ryosuke Niwa]

(In reply to comment #1)
> Do you have a suggested port to use?  Ideally the port would build on Linux and have a very high "uptime" in terms of building/tests-passing.  The linux part is because linux machines are easy to acquire/admin, and the buildability/testability part is for up-time of the queue.
> 
> It's possible we could just call build-javascriptcore of whatever port.

How about Qt? Qt folks are very diligent in keeping bots green and they cycle fast.

[Adam Barth]

> How about Qt? Qt folks are very diligent in keeping bots green and they cycle fast.

Unfortunately, I can't figure out how to build Qt anymore.  I could try again, but it requires some bleeding edge libraries.  Maybe some Qt folks would be willing to help.

[Adam Barth]

By the way, the way the system is supposed to work is that the EWS bots examine the patch on the various different ports and set commit-queue- if the patch breaks them.  The best solution might be to run the tests on a JSC port EWS bot.

[Radar WebKit Bug Importer]

<rdar://problem/12992333>

[Ryosuke Niwa]

(In reply to comment #4)
> By the way, the way the system is supposed to work is that the EWS bots examine the patch on the various different ports and set commit-queue- if the patch breaks them.  The best solution might be to run the tests on a JSC port EWS bot.

Yeah. Maybe commit queue can be smart and wait until Mac EWS or Mac WK2 EWS bots finish.

[Adam Barth]

The current system doesn't wait on anything.  It's designed that way to encourage bot maintainers to make their bots fast.  If the commit-queue waited on an EWS bot, the maintainers of that bot would be slowing everyone else down if they let their bots slow down.

[Ryosuke Niwa]

(In reply to comment #7)
> The current system doesn't wait on anything.  It's designed that way to encourage bot maintainers to make their bots fast.  If the commit-queue waited on an EWS bot, the maintainers of that bot would be slowing everyone else down if they let their bots slow down.

On the other hand, it doesn't make any sense to build & run tests on chromium-linux before landing a JSC/WK2 specific change because chromium-linux doesn't even build JSC or WK2.

[Rafael Brandao]

At that time, I've figured out the breakage happened because the faulty patch was rolled out manually right before the build fix I've submitted landed via commit-queue, and then the fix would not make any sense. Not sure of how waiting for bots results would help in this case.

[Ryosuke Niwa]

(In reply to comment #9)
> At that time, I've figured out the breakage happened because the faulty patch was rolled out manually right before the build fix I've submitted landed via commit-queue, and then the fix would not make any sense. Not sure of how waiting for bots results would help in this case.

I don't think roll outs need to wait for EWS bots.

[Rafael Brandao]

(In reply to comment #10)
> (In reply to comment #9)
> > At that time, I've figured out the breakage happened because the faulty patch was rolled out manually right before the build fix I've submitted landed via commit-queue, and then the fix would not make any sense. Not sure of how waiting for bots results would help in this case.
> 
> I don't think roll outs need to wait for EWS bots.

Me neither. What happened was an unfortunate and unavoidable set of actions:
1. Patch A lands. Breaks some build system.
2. Patch B is evaluated for EWS to fix build A and it's correct. Waiting to land via commit-queue.
3. Patch C reverts A and it's landed faster (manually or maybe commit-queue prioritization of commits if any).
4. Patch B lands. But without A, it makes no sense and breaks everything.

So the best thing to do (tm) is to just poke the author of patch A and ask it to fix, or just poke on the original bug, rather than creating a separate bug to fix it. I'm still missing how 4th step could be avoided by any heuristic.

[Ryosuke Niwa]

(In reply to comment #11)
>
> Me neither. What happened was an unfortunate and unavoidable set of actions:
> 1. Patch A lands. Breaks some build system.
> 2. Patch B is evaluated for EWS to fix build A and it's correct. Waiting to land via commit-queue.
> 3. Patch C reverts A and it's landed faster (manually or maybe commit-queue prioritization of commits if any).
> 4. Patch B lands. But without A, it makes no sense and breaks everything.
> 
> So the best thing to do (tm) is to just poke the author of patch A and ask it to fix, or just poke on the original bug, rather than creating a separate bug to fix it. I'm still missing how 4th step could be avoided by any heuristic.

I don't think this problem is related to this bug.

[Csaba Osztrogonác]

(In reply to comment #6)
> (In reply to comment #4)
> > By the way, the way the system is supposed to work is that the EWS bots examine the patch on the various different ports and set commit-queue- if the patch breaks them.  The best solution might be to run the tests on a JSC port EWS bot.
> 
> Yeah. Maybe commit queue can be smart and wait until Mac EWS or Mac WK2 EWS bots finish.

I think reviewer shouldn't set r+ and cq+ flags _before_ all related EWS
bubbles become green. It is the easiest way to avoid build breakages. ;-)

[Csaba Osztrogonác]

(In reply to comment #11)

> Me neither. What happened was an unfortunate and unavoidable set of actions:
> 1. Patch A lands. Breaks some build system.
> 2. Patch B is evaluated for EWS to fix build A and it's correct. Waiting to land via commit-queue.
> 3. Patch C reverts A and it's landed faster (manually or maybe commit-queue prioritization of commits if any).
> 4. Patch B lands. But without A, it makes no sense and breaks everything.
> 
> So the best thing to do (tm) is to just poke the author of patch A and ask it to fix, or just poke on the original bug, rather than creating a separate bug to fix it. I'm still missing how 4th step could be avoided by any heuristic.

The easiest way to avoid this problems is the following: Add a comment to 
the bug that you are fixing the build soon and land the fix manually not via
(sometimes slow) commit queue. Buildfixes are very important, all delay is 
pain for developers. (But I agree with rniwa, it is unrelated to this bug.)

[Adam Barth]

> I think reviewer shouldn't set r+ and cq+ flags _before_ all related EWS
> bubbles become green. It is the easiest way to avoid build breakages. ;-)

I don't think anyone should have to wait for bots.  If the bots can't keep up, that means we need more bots.

[Adam Barth]

> On the other hand, it doesn't make any sense to build & run tests on chromium-linux before landing a JSC/WK2 specific change because chromium-linux doesn't even build JSC or WK2.

I run the commit-queue on chromium-linux because that's what most convenient for me.  If you'd like to take over running the commit-queue (i.e., providing the resources and administering the machines), then you'd could run them on whatever platform you like.

[Ryosuke Niwa]

(In reply to comment #16)
> > On the other hand, it doesn't make any sense to build & run tests on chromium-linux before landing a JSC/WK2 specific change because chromium-linux doesn't even build JSC or WK2.
> 
> I run the commit-queue on chromium-linux because that's what most convenient for me.  If you'd like to take over running the commit-queue (i.e., providing the resources and administering the machines), then you'd could run them on whatever platform you like.

That's not the problem I'm trying to solve. The problem is that the current system gives an illusion of JSC/WK2 specific patches being tested on commit-queue because it builds and run tests.

It's akin to the problem of only cr-linux and mac EWS bots running tests. Almost every new contributor assumes that all EWS bots run tests unless otherwise told.

[Ryosuke Niwa]

IMO, either not building chromium or not letting commit-queue be used to land JSC/WK2 changes would be better when landing JSC/WK2 specific changes.

[Adam Barth]

My position on this topic has been consistent since the beginning.  I went back and read the original announcement I wrote about the EWS: http://lists.webkit.org/pipermail/webkit-dev/2009-December/011016.html

---8<---
Like the style-queue, the EWS is purely advisory.  Contributors and
reviewers are free to ignore the warnings if they believe the warnings
are erroneous or they decide (for whatever reason) to break the build
in question.
--->8---

People shouldn't need to wait for bots.  If there's a JSC or a WK2 problem with a patch, then the EWS bot for that configuration should commit-queue- the patch automatically, stoping the patch from being landed.  If the EWS bot for that configuration doesn't run in time, that means you need to invest more resources in that configuration to make it run fast enough.  That doesn't mean we should slow down development to wait for the bot.

[Ryosuke Niwa]

(In reply to comment #19)
> ---8<---
> Like the style-queue, the EWS is purely advisory.  Contributors and
> reviewers are free to ignore the warnings if they believe the warnings
> are erroneous or they decide (for whatever reason) to break the build
> in question.
> --->8---
> 
> People shouldn't need to wait for bots.  If there's a JSC or a WK2 problem with a patch, then the EWS bot for that configuration should commit-queue- the patch automatically, stoping the patch from being landed.  If the EWS bot for that configuration doesn't run in time, that means you need to invest more resources in that configuration to make it run fast enough.  That doesn't mean we should slow down development to wait for the bot.

I'm sorry but I don't follow. On one hand, you say that we shouldn't wait for bots and EWS is only advisory. On the other hand, you say that bots should be able to cq- on time to recent commit-queue from landing patches. Either EWS is advisory, in which case, it shouldn't be required for commit-queue to the right thing, or it should be a required system for commit-queue to work properly.

If your position is that we can't fix this bug or this bug is invalid, then I'll make an announcement in webkit-dev instead saying that we shouldn't be using commit-queue to land JSC/WK2 specific patches and/or I'll propose to make changes so that cq+ is ignored on JSC/WK specific patches.

[Ryosuke Niwa]

s/to recent commit-queue from landing patches/to prevent commit-queue from landing patches/.

[Adam Barth]

> I'm sorry but I don't follow. On one hand, you say that we shouldn't wait for bots and EWS is only advisory. On the other hand, you say that bots should be able to cq- on time to recent commit-queue from landing patches.

There's "should" in that sentence.  That's how the system works today.

> Either EWS is advisory, in which case, it shouldn't be required for commit-queue to the right thing, or it should be a required system for commit-queue to work properly.

That's a false dichotomy.  People are perfectly capable of changing the bot-set commit-queue- into a commit-queue+ if that's the appropriate thing to do.

> If your position is that we can't fix this bug or this bug is invalid, then I'll make an announcement in webkit-dev instead saying that we shouldn't be using commit-queue to land JSC/WK2 specific patches and/or I'll propose to make changes so that cq+ is ignored on JSC/WK specific patches.

My position is that this bug is invalid.  You're welcome to start whatever threads you like on webkit-dev.  However, I don't support that change to how the commit-queue+ flag works.

[Adam Barth]

> There's "should" in that sentence.

I meant to write that there's *no* "should" in that sentence.