Bug 160739 – EWS should check if the patch is still valid before executing every major step

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

[Aakash Jain]

Most of the time, it's wasteful for EWS bots to keep building or testing once a patch has become invalid (e.g.: obsoleted or r-).
It would be much better to drop obsoleted patches quickly, and to move to ones that people are actually waiting for.

One step in this direction is for EWS to check if patch is still valid before executing every major step.

Also see <rdar://problem/27768813>

[Aakash Jain]

Created attachment 285733 [details]
Proposed patch

This does not stop the processing once the build/command has started, but checks for validation more frequently. For e.g.: if the patch fails to build, before attempting _build_without_patch, we would validate and stop.

Note: This fails unit tests because of the change in logic, I will fix it. I want to get feedback on the logic first.

[Alexey Proskuryakov]

Comment on attachment 285733 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=285733&action=review

> Tools/Scripts/webkitpy/tool/bot/patchanalysistask.py:88
> +        self.validate()

Do you have a list of steps that now validate, and they didn't before? It's not obvious to me if this change provides a noticeable improvement - the real benefit would come from the ability to interrupt builds and tests, which would be a much larger change.

[Aakash Jain]

This patch perform validation before any step which executes _run_command(). There are 8 such methods: _build_without_patch, _build_and_test_without_patch*(), _clean(), _update(), _apply(), _build(), _land() and _test().

Earlier we were doing validate before clean() and if validation pass, we run all the next steps. I think the major improvement in this patch would be to skip:  _build_without_patch, _build_and_test_without_patch and _test. 

For e.g.: _build_without_patch might take significant time, we can skip it for obsolete patches.

[Alexey Proskuryakov]

Comment on attachment 285733 [details]
Proposed patch

Makes sense, that's a non-trivial improvement.

There is some risk of reporting tools being broken by not running all the steps, we should look out for that.

[Aakash Jain]

Created attachment 285756 [details]
Updated patch

Simplified the patch and updated unit-tests. Please review again.

[WebKit Commit Bot]

Comment on attachment 285756 [details]
Updated patch

Clearing flags on attachment: 285756

Committed r204382: <http://trac.webkit.org/changeset/204382>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.