EWS should set cq- flag on the patch when that patch fails to build. This would help in two ways: #1: making sure commit-queue doesn't commit a patch if it fails to build webkit on any configuration (e.g.: ios, win, wpe, gtk, watch, tv etc.) #2: Bugzilla would send an email notification when patch fails to build (notification about patch being marked as cq-). Also, if a patch is already marked cq-, subsequent attempt to mark it cq- would be ignored by Bugzilla. So, if a patch fails to build on multiple configuration, bugzilla will set cq- only once and generate only one email notification.
Created attachment 403989 [details] Patch
Sample run: https://ews-build.webkit-uat.org/#/builders/35/builds/2573
Created attachment 403993 [details] Patch
Updated the patch to include the case of patch introducing layout-test failure. EWS should cq- patch in that case as well.
<rdar://problem/65852947>
Comment on attachment 403993 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=403993&action=review > Tools/BuildSlaveSupport/ews-build/steps.py:1941 > + self.build.addStepsAfterCurrentStep([SetCommitQueueMinusFlagOnPatch()]) To clarify, this only does anything if a patch is in commit queue and tests fail before it lands, right?
(In reply to Aakash Jain from comment #4) > Updated the patch to include the case of patch introducing layout-test failure. EWS should cq- patch in that case as well. Since we have various flaky layout tests, they can causing false positives as well (causing cq- flag on the bug unnecessarily).
(In reply to Jonathan Bedard from comment #6) > To clarify, this only does anything if a patch is in commit queue and tests fail before it lands, right? This marks the patch cq- irrespective of whether the patch is in commit-queue or not. If patch is in commit-queue, and patch is marked cq-, commit-queue would not land it. Engineers can always override it by setting cq+ flag again.
Created attachment 408249 [details] Patch
Committed r266741: <https://trac.webkit.org/changeset/266741> All reviewed patches have been landed. Closing bug and clearing flags on attachment 408249 [details].
Would be better if this was refined so that it will not add a "commit-queue-" to an obsolete patch. Doing that generates notification mail that's not valuable and has no other practical effect. I’ve been getting that mail.
(In reply to Darin Adler from comment #11) > Would be better if this was refined so that it will not add a "commit-queue-" to an obsolete patch. That's a good point. I will make that change. Also, what should be the behavior in these two scenarios (should the cq- flag be set by ews or not): 1) when the patch is already marked r- 2) when corresponding bug is already resolved (patch already landed either through commit-queue or manually).
(In reply to Aakash Jain from comment #12) > Also, what should be the behavior in these two scenarios (should the cq- > flag be set by ews or not): > 1) when the patch is already marked r- > > 2) when corresponding bug is already resolved (patch already landed either > through commit-queue or manually). In both these cases I don’t see a lot of value adding commit-queue-. I suppose that if commit-queue? or commit-queue+ was set, I might think it’s OK to *change* it to commit-queue-. But it’s not great to add a “commit-queue-“ when nothing indicates we intend to commit the patch and no one was saying anything about the commit queue at all.