Bug 65200

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.

Created attachment 102108 [details] Patch

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

Created attachment 102233 [details] Patch Thanks for review! Added "upload" parameter that when set to "true" will upload the contents.

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.

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.

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

Created attachment 102644 [details] Patch

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.

Created attachment 102647 [details] Patch Done.

Comment on attachment 102647 [details] Patch Clearing flags on attachment: 102647 Committed r92191: <http://trac.webkit.org/changeset/92191>

All reviewed patches have been landed. Closing bug.