CSS Regions build bot should archive and upload output files

[Alexandru Chiculita]

Currently CSS Regions build bot does not store the output results. It should archive and upload the output to the buildbot master.

[Alexandru Chiculita]

Make sure "features" are also taken into account when computing the file name of the archive folder. That way trunk snow leopard and css regions snow leopard will not overlap.

[Alexandru Chiculita]

Created attachment 102108 [details]
Patch

[Adam Roben (:aroben)]

Comment on attachment 102108 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=102108&action=review

> Tools/BuildSlaveSupport/build.webkit.org-config/master.cfg:132
> +def determineExtraFeatures(build):

Can this be a member of the UploadBuiltProduct class?

> Tools/BuildSlaveSupport/build.webkit.org-config/master.cfg:134
> +    if (build.hasProperty("features")):

No need for the parentheses here.

I think using an early return would be clearer:

if not build.hasProperty('features'):
    return ''

Ditto for the len(features) check below.

> Tools/BuildSlaveSupport/build.webkit.org-config/master.cfg:141
> +

Missing an extra newline here.

> Tools/BuildSlaveSupport/build.webkit.org-config/master.cfg:623
>          if triggers:
>              self.addStep(ArchiveBuiltProduct)
>              self.addStep(UploadBuiltProduct)
> -            self.addStep(trigger.Trigger, schedulerNames=triggers)
> +            if triggers.count("upload-only") != len(triggers):
> +                self.addStep(trigger.Trigger, schedulerNames=triggers)

Is it OK to be passing "upload-only", which isn't a real trigger, to the schedulerNames parameter? Maybe it would be better to have some other way of specifying in config.json that you want the build uploaded. Then here you could do "if triggers or thatOtherThing:"

[Alexandru Chiculita]

Created attachment 102233 [details]
Patch

Thanks for review! 

Added "upload" parameter that when set to "true" will upload the contents.

[Adam Roben (:aroben)]

Comment on attachment 102233 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=102233&action=review

This is looking really close!

> Tools/BuildSlaveSupport/build.webkit.org-config/config.json:247
> +                      "upload": "true", "slavenames": ["adobe-mac-slave1"]

You can use a literal true value here. No need for the string. That will make the master.cfg logic simpler.

> Tools/BuildSlaveSupport/build.webkit.org-config/master.cfg:-131
> -

You should leave this extra line in place to match standard Python style.

> Tools/BuildSlaveSupport/build.webkit.org-config/master.cfg:133
> +    masterdest = WithProperties("archives/%(fullPlatform)s-%(architecture)s-%(configuration)s%(extraFeatures)s/%(got_revision)s.zip", extraFeatures=lambda build:UploadBuiltProduct.determineExtraFeatures(build))

I'm surprised the lambda is needed.

> Tools/BuildSlaveSupport/build.webkit.org-config/master.cfg:741
> +            "upload": builder.pop("upload", None) == "true"

False would probably be a better default, especially once you start using a real True value instead of a string.

[Alexandru Chiculita]

Comment on attachment 102233 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=102233&action=review

Thanks for the review.

>> Tools/BuildSlaveSupport/build.webkit.org-config/config.json:247
>> +                      "upload": "true", "slavenames": ["adobe-mac-slave1"]
> 
> You can use a literal true value here. No need for the string. That will make the master.cfg logic simpler.

Ok.

>> Tools/BuildSlaveSupport/build.webkit.org-config/master.cfg:-131
>> -
> 
> You should leave this extra line in place to match standard Python style.

Ok.

>> Tools/BuildSlaveSupport/build.webkit.org-config/master.cfg:133
>> +    masterdest = WithProperties("archives/%(fullPlatform)s-%(architecture)s-%(configuration)s%(extraFeatures)s/%(got_revision)s.zip", extraFeatures=lambda build:UploadBuiltProduct.determineExtraFeatures(build))
> 
> I'm surprised the lambda is needed.

determineExtraFeatures will only be defined after this initialization. I'm new to python, so I might be wrong. The options that I found to be working are:
a. put the method at the module level 
b. use the lambda
c. move the method before masterdest, but I think it looks better to have the properties grouped at the top

>> Tools/BuildSlaveSupport/build.webkit.org-config/master.cfg:741
>> +            "upload": builder.pop("upload", None) == "true"
> 
> False would probably be a better default, especially once you start using a real True value instead of a string.

Ok.

[Adam Roben (:aroben)]

Comment on attachment 102233 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=102233&action=review

>>> Tools/BuildSlaveSupport/build.webkit.org-config/master.cfg:133
>>> +    masterdest = WithProperties("archives/%(fullPlatform)s-%(architecture)s-%(configuration)s%(extraFeatures)s/%(got_revision)s.zip", extraFeatures=lambda build:UploadBuiltProduct.determineExtraFeatures(build))
>> 
>> I'm surprised the lambda is needed.
> 
> determineExtraFeatures will only be defined after this initialization. I'm new to python, so I might be wrong. The options that I found to be working are:
> a. put the method at the module level 
> b. use the lambda
> c. move the method before masterdest, but I think it looks better to have the properties grouped at the top

The lambda seems OK. But you should rename the parameter to "properties", since what is passed is a Properties instance, not a Build instance.

> Tools/BuildSlaveSupport/build.webkit.org-config/master.cfg:140
> +    def determineExtraFeatures(build):

Same comment here about renaming "build" to "properties".

[Alexandru Chiculita]

Created attachment 102644 [details]
Patch

[Adam Roben (:aroben)]

Comment on attachment 102644 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=102644&action=review

Just a couple of cosmetic issues this time. Looks great!

> Tools/BuildSlaveSupport/build.webkit.org-config/config.json:262
>                        "platform": "mac-snowleopard", "configuration": "release", "architectures": ["x86_64"], "features": ["css-regions", "css-exclusions"],
> -                      "slavenames": ["adobe-mac-slave1"]
> +                      "upload": true, "slavenames": ["adobe-mac-slave1"]

I'd recommend putting "upload": true on its own line above "slavenames".

> Tools/BuildSlaveSupport/build.webkit.org-config/master.cfg:135
> +                                extraFeatures=lambda properties:UploadBuiltProduct.determineExtraFeatures(properties))

Missing a space after the colon.

[Alexandru Chiculita]

Created attachment 102647 [details]
Patch

Done.

[WebKit Review Bot]

Comment on attachment 102647 [details]
Patch

Clearing flags on attachment: 102647

Committed r92191: <http://trac.webkit.org/changeset/92191>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.