| Summary: | [Chromium] sheriffbot roll-chromium-deps shouldn't create a bug with None as the new revision. | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | David Levin <levin> | ||||||||||||||||
| Component: | Tools / Tests | Assignee: | Adam Barth <abarth> | ||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||
| Severity: | Normal | CC: | abarth, alancutter, darin, dpranke, eric, levin, mnaganov, rniwa, webkit.review.bot | ||||||||||||||||
| Priority: | P2 | ||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||
| Attachments: |
|
||||||||||||||||||
|
Description
David Levin
2011-11-30 12:58:35 PST
Assigned for tracking. Another "None" patch created by sheriffbot: https://bugs.webkit.org/attachment.cgi?id=136060&action=edit *** Bug 84443 has been marked as a duplicate of this bug. *** webkit-patch roll-chromium-deps works fine. Not sure if sheriffbot is failing to fetch the URL or something else is happening there :\ 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. Created attachment 181559 [details]
Patch
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? 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. Created attachment 181637 [details]
Patch
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. Created attachment 181642 [details]
Patch
(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. 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. (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. Created attachment 181645 [details]
Patch
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"! (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... Created attachment 181646 [details]
Patch
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?
(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"). 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. Created attachment 185444 [details]
Patch
Created attachment 185447 [details]
Patch
(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. Shouldn't it just fail if "None" is the LKGR? Explicit is OK, but seems unrelated. (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")? Comment on attachment 185447 [details] Patch Clearing flags on attachment: 185447 Committed r144940: <http://trac.webkit.org/changeset/144940> All reviewed patches have been landed. Closing bug. |