Bug 73482 – [Chromium] sheriffbot roll-chromium-deps shouldn't create a bug with None as the new revision.

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

[David Levin]

Like this one: https://bugs.webkit.org/show_bug.cgi?id=73481

[Adam Barth]

Assigned for tracking.

[Ami Fischman]

Another "None" patch created by sheriffbot: https://bugs.webkit.org/attachment.cgi?id=136060&action=edit

[Ami Fischman]

*** Bug 84443 has been marked as a duplicate of this bug. ***

[Ryosuke Niwa]

webkit-patch roll-chromium-deps works fine. Not sure if sheriffbot is failing to fetch the URL or something else is happening there :\

[Alan Cutter]

Sheriffbot passes None to UpdateChromiumDEPS via UNIX pipes causing None to be converted to "None".
Ideally argument validation should occur in UpdateChromiumDEPS rather than Sheriffbot.

[Alan Cutter]

Created attachment 181559 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 181559 [details]
Patch

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

> Tools/Scripts/webkitpy/tool/steps/updatechromiumdeps.py:76
>          new_chromium_revision = state["chromium_revision"]

Why don’t we just convert it to int here inside try & except instead of checking the validity here and then converting it into int in _validate_revisions?

[Adam Barth]

Comment on attachment 181559 [details]
Patch

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

>> Tools/Scripts/webkitpy/tool/steps/updatechromiumdeps.py:76
>>          new_chromium_revision = state["chromium_revision"]
> 
> Why don’t we just convert it to int here inside try & except instead of checking the validity here and then converting it into int in _validate_revisions?

Yeah, other places we use the ValueException (or something like that) to handle these cases.

[Alan Cutter]

Created attachment 181637 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 181637 [details]
Patch

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

> Tools/Scripts/webkitpy/tool/steps/updatechromiumdeps.py:54
> +        except Exception, e:
> +            return None

Just except ValueException since we shouldn’t be getting other kinds of exceptions.

[Alan Cutter]

Created attachment 181642 [details]
Patch

[Alan Cutter]

(In reply to comment #10)
> (From update of attachment 181637 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=181637&action=review
> 
> > Tools/Scripts/webkitpy/tool/steps/updatechromiumdeps.py:54
> > +        except Exception, e:
> > +            return None
> 
> Just except ValueException since we shouldn’t be getting other kinds of exceptions.

Ah thanks for picking that up, that was sloppy of me.
Fixed in repatch.

[Ryosuke Niwa]

Comment on attachment 181642 [details]
Patch

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

> Tools/Scripts/webkitpy/tool/steps/updatechromiumdeps.py:79
> +        if new_chromium_revision == None:

It’s probably sufficient to check "not new_chromium_revision" since revision 0 doesn’t exist nor is useful.

[Alan Cutter]

(In reply to comment #13)
> (From update of attachment 181642 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=181642&action=review
> 
> > Tools/Scripts/webkitpy/tool/steps/updatechromiumdeps.py:79
> > +        if new_chromium_revision == None:
> 
> It’s probably sufficient to check "not new_chromium_revision" since revision 0 doesn’t exist nor is useful.

Agreed in hindsight.
Changed in repatch.

[Alan Cutter]

Created attachment 181645 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 181645 [details]
Patch

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

> Tools/Scripts/webkitpy/tool/steps/updatechromiumdeps.py:79
> +        if new_chromium_revision:

Now you’re missing "not"!

[Alan Cutter]

(In reply to comment #16)
> (From update of attachment 181645 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=181645&action=review
> 
> > Tools/Scripts/webkitpy/tool/steps/updatechromiumdeps.py:79
> > +        if new_chromium_revision:
> 
> Now you’re missing "not"!

Ouch! Been a long day...

[Alan Cutter]

Created attachment 181646 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 181646 [details]
Patch

Should we be logging to the user when their revision fails to parse and we end up using the LKGR instead?

[Alan Cutter]

(In reply to comment #19)
> (From update of attachment 181646 [details])
> Should we be logging to the user when their revision fails to parse and we end up using the LKGR instead?

I was wondering that and figured it would be more appropriate to address in a separate bug.
Alternatively it should fail completely if an invalid input is given and have a special input for using the LKGR (eg. "LKGR").

[Eric Seidel (no email)]

Comment on attachment 181646 [details]
Patch

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

Otherwise this is good to go.

> Tools/Scripts/webkitpy/tool/steps/updatechromiumdeps.py:48
> +    @staticmethod
> +    def _parse_revision_name(revision):

Please write a test for this.

[Alan Cutter]

Created attachment 185444 [details]
Patch

[Alan Cutter]

Created attachment 185447 [details]
Patch

[Alan Cutter]

(In reply to comment #23)
> Created an attachment (id=185447) [details]
> Patch

Changed the RollChromiumDEPS command to reject invalid revision numbers and use "LKGR" explicitly to use the last known good revision.
Added error handling in the UpdateChromiumDEPS step and logic in the Sheriffbot to handle those errors.

[Eric Seidel (no email)]

Shouldn't it just fail if "None" is the LKGR?  Explicit is OK, but seems unrelated.

[Alan Cutter]

(In reply to comment #25)
> Shouldn't it just fail if "None" is the LKGR?  Explicit is OK, but seems unrelated.

I don't think the LKGR being None was the cause of the issue, rather Sheriffbot was passing None to the webkit-patch post-chromium-deps-roll command via a pipe when no valid revision was given by the user. The None was converted to the string "None" and no longer evaluates as Boolean False when UpdateChromiumDEPS checks it. UpdateChromiumDEPS then uses "None" instead of fetching the LKGR.
The problem was UpdateChromiumDEPS wasn't validating its input properly.

Should we keep the behaviour of using LKGR if no revision is specified but fail on any other invalid input (like "None")?

[WebKit Review Bot]

Comment on attachment 185447 [details]
Patch

Clearing flags on attachment: 185447

Committed r144940: <http://trac.webkit.org/changeset/144940>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.