Bug 65171

Created attachment 101984 [details] Patch

Comment on attachment 101984 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=101984&action=review This seems fine to me, but aroben might have some additional feedback. FWIW, on the flexbox bot, I just modified files in the checkout to have it compile with the necessary flags and to enable the layout tests (removed the SKIP line from test_expectations.txt). It's a bit fragile since I have to manually fix merge conflict, but that hasn't happened yet. > Tools/BuildSlaveSupport/build.webkit.org-config/master.cfg:108 > + if isinstance(features, list): Nit: I would remove this type check, I'm not sure it provides much safety other than hiding config.json errors. Also, it seems like any iterable would work.

(In reply to comment #2) > FWIW, on the flexbox bot, I just modified files in the checkout to have it compile with the necessary flags and to enable the layout tests (removed the SKIP line from test_expectations.txt). It's a bit fragile since I have to manually fix merge conflict, but that hasn't happened yet. I think we might better add support to overwrite/preprocess test_expectation. I will add a bug for that. > Nit: I would remove this type check, I'm not sure it provides much safety other than hiding config.json errors. Also, it seems like any iterable would work. Thanks. The value should come from the JSON Decoder, which will only generate dict or list iterable objects. I can move that check at the parse time and throw an exception.

Comment on attachment 101984 [details] Patch I agree with Tony's comments. Other than that this looks fine.

Created attachment 102034 [details] Patch Moved the check to the parse time.

Comment on attachment 102034 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=102034&action=review > Tools/BuildSlaveSupport/build.webkit.org-config/master.cfg:728 > + if isinstance(features, list) == False: Better WebKit style would be: if not isinstance(features, list): We don't have this kind of checking for other parts of config.json. Why is this attribute special? > Tools/BuildSlaveSupport/build.webkit.org-config/master.cfg:729 > + raise Exception, "Builder %r: 'features' parameter needs to be a list of flags that will be passed to build-webkit script. eg. features: ['css-regions']" % builder['name'] I think the more modern way to do this is using constructor syntax: raise Exception("Builder %r:...")

Created attachment 102037 [details] Patch It isn't special. I just removed the check.

+wms for master restart

Comment on attachment 102037 [details] Patch Actually, I don't think we can land this yet. Currently the CSSRegions bot is uploading its built frameworks to <http://build.webkit.org/archives/mac-snowleopard-x86_64-release/>. Presumably it's overwriting any builds that were built by Apple's bots and uploaded there, but so far that's harmless because the CSSRegions bot and Apple's bots are building the same code. With this change, the builds will now be different. We need to change the UploadBuiltProduct step to upload these builds to some other place.

(In reply to comment #8) > +wms for master restart FYI, this isn't needed anymore. The build master restarts itself automatically.

(In reply to comment #9) > (From update of attachment 102037 [details]) > Actually, I don't think we can land this yet. > > Currently the CSSRegions bot is uploading its built frameworks to <http://build.webkit.org/archives/mac-snowleopard-x86_64-release/>. Presumably it's overwriting any builds that were built by Apple's bots and uploaded there, but so far that's harmless because the CSSRegions bot and Apple's bots are building the same code. With this change, the builds will now be different. > > We need to change the UploadBuiltProduct step to upload these builds to some other place. Chiculita pointed out that I was incorrect. UploadBuiltProduct doesn't run at all if your builder has no triggers.

(In reply to comment #11) > Chiculita pointed out that I was incorrect. UploadBuiltProduct doesn't run at all if your builder has no triggers. Added https://bugs.webkit.org/show_bug.cgi?id=65200 to also archive and upload the CSS Regions build bot output files.

Comment on attachment 102037 [details] Patch Clearing flags on attachment: 102037 Committed r91782: <http://trac.webkit.org/changeset/91782>

All reviewed patches have been landed. Closing bug.