Summary: | master.cfg cleanup, pass BuildStep instances instead of BuildStep subclasses | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Csaba Osztrogonác <ossy> | ||||||||
Component: | Tools / Tests | Assignee: | Szilard Ledan <szledan> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | galpeter, lforschler, ossy, rniwa, tony, webkit.review.bot | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Bug Depends on: | 89120, 89215, 89564 | ||||||||||
Bug Blocks: | |||||||||||
Attachments: |
|
Description
Csaba Osztrogonác
2012-06-13 08:15:40 PDT
Created attachment 147319 [details]
Patch
Looks good to me... finally, those deprecation warnings will be gone :) Landed in http://trac.webkit.org/changeset/120352 Re-opened since this is blocked by 89120 It caused problems, because buildstep's constructors aren't in a good shape now, they must be refactored before we commit this change. See http://buildbot.net/buildbot/docs/0.8.6p1/manual/customization.html#writing-buildstep-constructors for details. I'm going to check and fix them one by one soon. I'm going to add smaller patches to "depends on" list, here is the first one - https://bugs.webkit.org/show_bug.cgi?id=89215 Unfortunately I can't test all of them, because I can't run locally for example Apple, Chromium, ... buildslaves. And unfortunately this kind of change isn't unittestable simple ... But I'm going to thinking how is it possible ... Or Péter, have you got any idea, how can we can make this kind of changes unittestable? Unit test helping this fix added in https://bugs.webkit.org/show_bug.cgi?id=89564 Created attachment 150564 [details]
patch
View in context: https://bugs.webkit.org/attachment.cgi?id=150564&action=review Just some minor things: > Tools/BuildSlaveSupport/build.webkit.org-config/master.cfg:222 > + kwargs["slavesrc"]=self.slavesrc > + kwargs["masterdest"]=self.masterdest > + kwargs["mode"]=0644 put spaces before and after the '=' in this cases. > Tools/BuildSlaveSupport/build.webkit.org-config/master.cfg:642 > + kwargs["slavesrc"]=self.slavesrc > + kwargs["masterdest"]=self.masterdest > + kwargs["mode"]=0644 ditto. > Tools/BuildSlaveSupport/build.webkit.org-config/master.cfg:652 > + kwargs["command"]="" ditto Comment on attachment 150564 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=150564&action=review > Tools/BuildSlaveSupport/build.webkit.org-config/master.cfg:220 > + kwargs["slavesrc"]=self.slavesrc One more thing: Please use ' instead of " similar to other parts of master.cfg. Created attachment 150572 [details]
patch
Thanks for your advice!
Comment on attachment 150572 [details]
patch
LGTM, r=me. But cq-, because I'd like to land it manually. If something goes wrong, I'll roll it out immediately.
Comment on attachment 150572 [details] patch Clearing flags on attachment: 150572 Committed r121760: <http://trac.webkit.org/changeset/121760> All reviewed patches have been landed. Closing bug. (In reply to comment #13) > (From update of attachment 150572 [details]) > Clearing flags on attachment: 150572 > > Committed r121760: <http://trac.webkit.org/changeset/121760> It seems master haven't restarted after this patch. Lucas, could you restart the master please? (Feel free to revert the patch if something doesn't work.) I just logged in to restart the master, but since we've moved to new hardware, my account hasn't been given permission. I've emailed the right people for 2 issues: 1. buildbot isnt' restarting after check-in 2. giving me permissions to manually restart. I've restarted the master. (In reply to comment #17) > I've restarted the master. Many thanks, we have better master.cfg day by day. :) |