| Summary: | Remove hardcoded "internal" from Buildbot.prototype._normalizeQueueInfo. | ||
|---|---|---|---|
| Product: | WebKit | Reporter: | Jason Marcell <jmarcell> |
| Component: | Tools / Tests | Assignee: | Jason Marcell <jmarcell> |
| Status: | RESOLVED FIXED | ||
| Severity: | Normal | CC: | adele, commit-queue, dbates, jmarcell |
| Priority: | P2 | ||
| Version: | 528+ (Nightly build) | ||
| Hardware: | Unspecified | ||
| OS: | Unspecified | ||
| See Also: | https://bugs.webkit.org/show_bug.cgi?id=147796 | ||
| Attachments: | |||
|
Description
Jason Marcell
2015-07-17 11:28:54 PDT
Created attachment 256978 [details]
Remove hardcoded "internal" from Buildbot.prototype._normalizeQueueInfo.
Comment on attachment 256978 [details] Remove hardcoded "internal" from Buildbot.prototype._normalizeQueueInfo. View in context: https://bugs.webkit.org/attachment.cgi?id=256978&action=review > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Buildbot.js:98 > + get defaultBranches() { Please move curly brace ('{'} to its own line. See the setter isAuthenticated() (below) for an example. > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/WebKitBuildbot.js:78 > + _getDefaultBranches: function() { Ditto. > Tools/ChangeLog:15 > + * BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Buildbot.js: > + (Buildbot.prototype._getDefaultBranches): > + base class method to return empty dictionary > + (Buildbot.prototype._normalizeQueueInfo): > + Calls _getDefaultBranches instead of hardcoded values > + * BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/WebKitBuildbot.js: > + (WebKitBuildbot.prototype._getDefaultBranches): > + overrides base class and provides a webkit-specific implementation that only cares about "opensource" This change log entry is out-of-date. Please update it. Also, please write complete sentences that start with a capital letter and end with a period. Comment on attachment 256978 [details] Remove hardcoded "internal" from Buildbot.prototype._normalizeQueueInfo. View in context: https://bugs.webkit.org/attachment.cgi?id=256978&action=review >> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/WebKitBuildbot.js:78 >> + _getDefaultBranches: function() { > > Ditto. "_getDefaultBranches: function()" => "get defaultBranches()" Thanks for your feedback, Dan. I'll be sending a new patch soon.
However, in my testing I found some assertions which this changes causes to fail, and which seem to contradict our assumption that combined queues should not have a dictionary of branches. This is in the constructor for BuildbotCombinedQueueView
BuildbotCombinedQueueView = function(queue)
{
for (var i = 1, end = queue.combinedQueues.length; i < end; ++i) {
console.assert(queue.combinedQueues[0].buildbot === queue.combinedQueues[i].buildbot);
console.assert(queue.combinedQueues[0].branch.openSource === queue.combinedQueues[i].branch.openSource);
console.assert(queue.combinedQueues[0].branch.internal === queue.combinedQueues[i].branch.internal);
}
I've just finished looking over the codebase for usages of .branch and I don't see any reason why we need to make this assertion.
Do you agree that we should remove these assertions?
Created attachment 257148 [details]
Remove hardcoded "internal" from Buildbot.prototype._normalizeQueueInfo.
Currently the "_normalizeQueueInfo" method is adding some hardcoded defaults for the queueInfo.branch data structure. I have been working with Dan Bates on a more generic solution in which the defaults may be specified instead by a Buildbot base class getter method (which returns an empty dictionary) that can be overridden by inheriting classes.
Comment on attachment 257148 [details] Remove hardcoded "internal" from Buildbot.prototype._normalizeQueueInfo. View in context: https://bugs.webkit.org/attachment.cgi?id=257148&action=review > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Buildbot.js:110 > + queueInfo.branch = queueInfo.combinedQueues ? null : queueInfo.branch || this.defaultBranches; Why can't a combined queue be on a branch? We don't have any now, but I don't see any intrinsic reason for this limitation. The operator precedence here seems non-obvious enough that I'd have appreciated parentheses. > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotCombinedQueueView.js:28 > + var all_branch = {}; "all_branch" is neither named according to WebKit style, nor grammatically correct. Comment on attachment 257148 [details] Remove hardcoded "internal" from Buildbot.prototype._normalizeQueueInfo. View in context: https://bugs.webkit.org/attachment.cgi?id=257148&action=review >> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Buildbot.js:110 >> + queueInfo.branch = queueInfo.combinedQueues ? null : queueInfo.branch || this.defaultBranches; > > Why can't a combined queue be on a branch? We don't have any now, but I don't see any intrinsic reason for this limitation. > > The operator precedence here seems non-obvious enough that I'd have appreciated parentheses. It seems that a combined queue is just a collection of sub-queues and that information pertaining to the repository/branch, etc. of the individual queues would be better kept at the sub-queue level. Especially in the case where the individual sub-queues have different repositories, what would it mean for the containing combined queue to have a repository associated with it? As Dan Bates mentioned to me, the parentheses are not necessary according to this: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Operator_Precedence However, I agree that a set of parentheses might be nice to emphasize which part is evaluated first for someone who does not have this reference handy and does not have the operator precedence memorized. >> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotCombinedQueueView.js:28 >> + var all_branch = {}; > > "all_branch" is neither named according to WebKit style, nor grammatically correct. I agree that this is not grammatically correct. This was not my initial choice for variable name, but I wanted to remain consistent with the choice of "branch" on the "BuildbotQueue" object (a data structure that contains repository to branch mappings). Would "all_branches" be a better choice? Created attachment 257198 [details]
Remove hardcoded "internal" from Buildbot.prototype._normalizeQueueInfo.
Dan Bates and Alexey Proskuryakov discussed the previous patch and concluded that combined queues should not have a repository/branch mapping associated with them. As such, this has been removed and we now assert that combined queues do not have a repository/branch mapping associated with them.
Also, with Dan Bates help, we arrived at a better way of reporting inconsistencies in the repository/branch mapping associations within a combined queue collection. We report the indices of conflicting sub-queues where the conflicting sub-queues have a mapping entry for the same repository but the branch associated with that repository is not the same.
Comment on attachment 257198 [details] Remove hardcoded "internal" from Buildbot.prototype._normalizeQueueInfo. View in context: https://bugs.webkit.org/attachment.cgi?id=257198&action=review > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Buildbot.js:101 > + get defaultBranches() > + { > + return {}; > + }, Do we need this? Looks like {} is always wrong. Comment on attachment 257198 [details] Remove hardcoded "internal" from Buildbot.prototype._normalizeQueueInfo. View in context: https://bugs.webkit.org/attachment.cgi?id=257198&action=review >> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Buildbot.js:101 >> + }, > > Do we need this? Looks like {} is always wrong. I was just providing a default implementation, but assuming we never instantiate Buildbot, but instead only ever instantiate a subclass of Buildbot, I agree that this is unneeded. I checked and did not see any instance of Buildbot being instantiated and I also tested removing this getter and everything seemed fine. Created attachment 257217 [details]
Remove hardcoded "internal" from Buildbot.prototype._normalizeQueueInfo.
Removing base class implementation of defaultBranches getter from "Buildbot" class.
Also removed braces from one-line "if" body.
Comment on attachment 257217 [details] Remove hardcoded "internal" from Buildbot.prototype._normalizeQueueInfo. View in context: https://bugs.webkit.org/attachment.cgi?id=257217&action=review Look good. > Tools/ChangeLog:18 > + Overrides the base class and provides a webkit-specific implementation that sets the queue.branches > + dictionary to have a single entry for "openSource" as the default set of branches. This change log entry is out of date. There is no base class implementation of defaultBranches. Nit: webkit => WebKit Comment on attachment 257217 [details] Remove hardcoded "internal" from Buildbot.prototype._normalizeQueueInfo. View in context: https://bugs.webkit.org/attachment.cgi?id=257217&action=review >> Tools/ChangeLog:18 >> + dictionary to have a single entry for "openSource" as the default set of branches. > > This change log entry is out of date. There is no base class implementation of defaultBranches. > > Nit: webkit => WebKit Also, as mentioned in an in-person discussion with Jason today, I suggest that we add a remark to the change log entry that explains that we intentionally omitted a base class implementation of the defaultBranches getter so as to cause a JavaScript TypeError when a sub-queue of a combined queue does not specify property branch and the derived Buildbot class does not implement defaultBranches so that a person can either update the definition of the sub-queue or implement defaultBranches in the derived Builedbot class they are using. (In reply to comment #0) > Currently the "_normalizeQueueInfo" method is adding some hardcoded defaults > for the queueInfo.branch data structure. I propose that the defaults be > specified instead by a Buildbot base class method (which returns an empty > dictionary) that can be overridden by inheriting classes. To clarify, Dan Bates made this proposal. Also, since the time of writing we have since removed the base class getter and now subclasses are required to provide their own implementation. Created attachment 257228 [details]
Remove hardcoded "internal" from Buildbot.prototype._normalizeQueueInfo.
Updating review info and ChangeLog comments.
Comment on attachment 257228 [details]
Remove hardcoded "internal" from Buildbot.prototype._normalizeQueueInfo.
We need to update the internal buildbot configuration before landing this patch.
I submitted a PFR for that. Comment on attachment 257228 [details] Remove hardcoded "internal" from Buildbot.prototype._normalizeQueueInfo. Clearing flags on attachment: 257228 Committed r187172: <http://trac.webkit.org/changeset/187172> All reviewed patches have been landed. Closing bug. |