Bug 127561 – Teach Buildbot dashboard to parse alternative revision format

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Daniel Bates]

According to David Farler, the preferred convention for having Buildbot return the Internal and OpenSource revision numbers is to have the Buildbot property revision be a dictionary that contains these values as opposed to defining a separate property for each revision number. We should teach the Buildbot dashboard code to recognize when the Buildbot property revision is a dictionary and extract the Internal and OpenSource revisions numbers from it.

[Daniel Bates]

Created attachment 222111 [details]
Patch

[Daniel Bates]

(In reply to comment #0)
> According to David Farler, the preferred convention for having Buildbot return the Internal and OpenSource revision numbers is to have the Buildbot property revision be a dictionary that contains these values as opposed to defining a separate property for each revision number. We should teach the Buildbot dashboard code to recognize when the Buildbot property revision is a dictionary and extract the Internal and OpenSource revisions numbers from it.

*property got_revision

[Daniel Bates]

Created attachment 222112 [details]
Patch

Update ChangeLog entry

[Alexey Proskuryakov]

Comment on attachment 222112 [details]
Patch

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

> Tools/ChangeLog:9
> +        Add support for extracting the OpenSource and Internal revision numbers when Buildbot
> +        returns a dictionary for the value of property got_revision.

What is the reason that we get revision number from the property, not from sourceStamp? Are these ever different?

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Utilities.js:78
> +function objectIsString(object)

I'm not sure why this function is needed. This is parsed JSON, can we possibly have String objects in it?

[David Farler]

(In reply to comment #4)
> (From update of attachment 222112 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=222112&action=review
> 
> > Tools/ChangeLog:9
> > +        Add support for extracting the OpenSource and Internal revision numbers when Buildbot
> > +        returns a dictionary for the value of property got_revision.
> 
> What is the reason that we get revision number from the property, not from sourceStamp? Are these ever different?

A sourceStamp is not final - it is akin to a “plan” for what will be checked out. The got_revision reflects what is actually checked out. For example, if a sourceStamp has branch = ‘trunk’ but revision undefined, the got_revision will become the latest revision in trunk, etc. If a source stamp had an explicit revision in it, then that specific revision is checked out and would be reflected in got_revision also. Therefore, got_revision should reflect the final say in what revision a build has.

> 
> > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Utilities.js:78
> > +function objectIsString(object)
> 
> I'm not sure why this function is needed. This is parsed JSON, can we possibly have String objects in it?

I’ll let Dan speak to this one, but it is probably because the the got_revision is a flat string in the open source buildbots and not a dictionary. If you use codebases, the supported multi-repository buildbot feature, you get a string -> { repository, branch, revision} structure in the sourcestamp and/or got_revision.

[Alexey Proskuryakov]

Comment on attachment 222112 [details]
Patch

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

>>> Tools/ChangeLog:9
>>> +        returns a dictionary for the value of property got_revision.
>> 
>> What is the reason that we get revision number from the property, not from sourceStamp? Are these ever different?
> 
> A sourceStamp is not final - it is akin to a “plan” for what will be checked out. The got_revision reflects what is actually checked out. For example, if a sourceStamp has branch = ‘trunk’ but revision undefined, the got_revision will become the latest revision in trunk, etc. If a source stamp had an explicit revision in it, then that specific revision is checked out and would be reflected in got_revision also. Therefore, got_revision should reflect the final say in what revision a build has.

We keep having bugs because of undefined revision for iterations that are loaded while svn step is still in progress. Would using sourceStamp resolve all these bugs for good? Do we have any builders configured to have undefined revision in sourceStamp? Would we ever want to?

[Daniel Bates]

Comment on attachment 222112 [details]
Patch

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

>>> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Utilities.js:78
>>> +function objectIsString(object)
>> 
>> I'm not sure why this function is needed. This is parsed JSON, can we possibly have String objects in it?
> 
> I’ll let Dan speak to this one, but it is probably because the the got_revision is a flat string in the open source buildbots and not a dictionary. If you use codebases, the supported multi-repository buildbot feature, you get a string -> { repository, branch, revision} structure in the sourcestamp and/or got_revision.

I'll inline the typeof check in the caller and omit the instanceof check before landing this patch.

Additional remarks:

Neither this function nor the type check for class String in its implementation are necessary. It is sufficient to inline the type check and omit the instanceof comparison. I chose to extract this functionality into a function that identifies string literals and objects derived from class String because I thought such a convenience function may be useful for future use. Having said that, we can define such a function should the need arise in the future.

[Alexey Proskuryakov]

Comment on attachment 222112 [details]
Patch

r=me with this change, however I think that we should be more clear on why we are getting the revision from properties.

[Daniel Bates]

Committed r162846: <http://trac.webkit.org/changeset/162846>

[WebKit Commit Bot]

Re-opened since this is blocked by bug 127695

[Daniel Bates]

Created attachment 222345 [details]
Patch

Added helper function isMultiCodebaseGotRevisionProperty() to determine whether a property is a multi-codebase got_revision property.

Updated patch to only use look at the value of property got_revision for the Internal revision number when its the multi-codebase variant.

[Daniel Bates]

Created attachment 222464 [details]
Patch

[Daniel Bates]

Committed r162971: <http://trac.webkit.org/changeset/162971>