Bug 65171 – CSS Regions build bot should build with "--css-regions" and "--css-exclusion" flags

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Alexandru Chiculita]

Add support for regions and exclusions optional features in build bot config file.

[Alexandru Chiculita]

Created attachment 101984 [details]
Patch

[Tony Chang]

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.

[Alexandru Chiculita]

(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.

[Adam Roben (:aroben)]

Comment on attachment 101984 [details]
Patch

I agree with Tony's comments. Other than that this looks fine.

[Alexandru Chiculita]

Created attachment 102034 [details]
Patch

Moved the check to the parse time.

[Adam Roben (:aroben)]

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:...")

[Alexandru Chiculita]

Created attachment 102037 [details]
Patch

It isn't special. I just removed the check.

[Tony Chang]

+wms for master restart

[Adam Roben (:aroben)]

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.

[Adam Roben (:aroben)]

(In reply to comment #8)
> +wms for master restart

FYI, this isn't needed anymore. The build master restarts itself automatically.

[Adam Roben (:aroben)]

(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.

[Alexandru Chiculita]

(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.

[WebKit Review Bot]

Comment on attachment 102037 [details]
Patch

Clearing flags on attachment: 102037

Committed r91782: <http://trac.webkit.org/changeset/91782>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.