Bug 65171 - CSS Regions build bot should build with "--css-regions" and "--css-exclusion" flags
Summary: CSS Regions build bot should build with "--css-regions" and "--css-exclusion"...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 57312
  Show dependency treegraph
 
Reported: 2011-07-26 04:31 PDT by Alexandru Chiculita
Modified: 2012-07-23 23:22 PDT (History)
5 users (show)

See Also:


Attachments
Patch (6.78 KB, patch)
2011-07-26 04:38 PDT, Alexandru Chiculita
aroben: review-
Details | Formatted Diff | Diff
Patch (6.99 KB, patch)
2011-07-26 11:40 PDT, Alexandru Chiculita
aroben: review-
Details | Formatted Diff | Diff
Patch (6.73 KB, patch)
2011-07-26 12:09 PDT, Alexandru Chiculita
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexandru Chiculita 2011-07-26 04:31:34 PDT
Add support for regions and exclusions optional features in build bot config file.
Comment 1 Alexandru Chiculita 2011-07-26 04:38:34 PDT
Created attachment 101984 [details]
Patch
Comment 2 Tony Chang 2011-07-26 09:55:28 PDT
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.
Comment 3 Alexandru Chiculita 2011-07-26 10:37:11 PDT
(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 4 Adam Roben (:aroben) 2011-07-26 10:44:29 PDT
Comment on attachment 101984 [details]
Patch

I agree with Tony's comments. Other than that this looks fine.
Comment 5 Alexandru Chiculita 2011-07-26 11:40:49 PDT
Created attachment 102034 [details]
Patch

Moved the check to the parse time.
Comment 6 Adam Roben (:aroben) 2011-07-26 12:05:58 PDT
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:...")
Comment 7 Alexandru Chiculita 2011-07-26 12:09:31 PDT
Created attachment 102037 [details]
Patch

It isn't special. I just removed the check.
Comment 8 Tony Chang 2011-07-26 12:18:52 PDT
+wms for master restart
Comment 9 Adam Roben (:aroben) 2011-07-26 12:19:03 PDT
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.
Comment 10 Adam Roben (:aroben) 2011-07-26 12:19:34 PDT
(In reply to comment #8)
> +wms for master restart

FYI, this isn't needed anymore. The build master restarts itself automatically.
Comment 11 Adam Roben (:aroben) 2011-07-26 12:22:02 PDT
(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.
Comment 12 Alexandru Chiculita 2011-07-26 12:26:09 PDT
(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 13 WebKit Review Bot 2011-07-26 14:17:27 PDT
Comment on attachment 102037 [details]
Patch

Clearing flags on attachment: 102037

Committed r91782: <http://trac.webkit.org/changeset/91782>
Comment 14 WebKit Review Bot 2011-07-26 14:17:32 PDT
All reviewed patches have been landed.  Closing bug.