478229 - Abstract core factory classes in buildbotcustom for use in other products

Status: RESOLVED FIXED

(Release Engineering :: General, defect, P2)

Description

[Robert Kaiser]

Currently, e.g. the MercurialBuildFactory hardcodes "firefox" in some places. We should try to make those core classes abstract enough so that we can share them between different products, including Firefox, Fennec, SeaMonkey, Thunderbird, and Sunbird.

Comment 1

[[e-mail] (:bhearsum)]

Attachment: remove firefox specifics where possible, and move them to a subclass where they are necessary

This isn't ready for a real review as it's not been tested but I want to make sure that overall, this is what you guys are looking for. MozillaBuildFactory and BaseRepackFactory have had their references to 'firefox' and 'browser' removed. I've also moved the checkout of code and configuration into separate functions - to make it easier for you guys to simple subclass and override those methods.

Before I test it, how does it look?

Comment 2

[Robert Kaiser]

Comment on attachment 363157 [details] [diff] [review]
remove firefox specifics where possible, and move them to a subclass where they are necessary

Most of this sounds good from looking over the patch without doing any testing or such.

>-            builds['firefox.dmg'] = 'Firefox %s.dmg' % longAppVersion
>-            self.env['ZIP_IN'] = WithProperties('%(srcdir)s/firefox.dmg')
>+            filename = '%s.dmg' % self.project
>+            builds[filename] = '%s %s.dmg' % (self.project.capitalize(),
>+                                              longAppVersion)
>+            self.env['ZIP_IN'] = WithProperties('%(srcdir)s/' + filename)

I see potential problems here:
1) SeaMonkey isn't using "pretty" package names for releases.
2) If we did, self.project.capitalize() would be "Seamonkey" and not "SeaMonkey", which is what be used in those names.

Doesn't 2) potentially affect Firefox as well with product codenames used for Alphas (did we have e.g. "BonEcho" or "GranParadiso" there)?

Of course, for not using "pretty" package names, we probably can go with subclassing here.

Comment 3

[[e-mail] (:bhearsum)]

Good point. What if we did something like:
class ReleaseBuildFactory(...):
  def __init__(self, ..., brandName=None, **kwargs):
    ...
    if brandName:
      self.brandName = brandName
    else:
      self.brandName = self.project.capitalize()

This way you don't need to duplicate any of the messy downloadBuilds() logic.

As for BonEcho/GranParadiso - that's a good point. We haven't shipped an alpha with this framework, so it hasn't been an issue yet.

Comment 4

[Robert Kaiser]

(In reply to comment #3)
> This way you don't need to duplicate any of the messy downloadBuilds() logic.

Looks good at least for the prettynames-case. For the non-prettynames case that SeaMonkey probably will continue to use even for releases, we still need to subclass or so, but that's doable, esp. given everyone else seems to be on prettynames for releases these days.

> As for BonEcho/GranParadiso - that's a good point. We haven't shipped an alpha
> with this framework, so it hasn't been an issue yet.

Right, and with Shiretoko it also wouldn't have bitten you :)

Comment 5

[[e-mail] (:bhearsum)]

Yeah, the non-prettynames case is obviously different.

I'll do some testing on my patch this week.

Comment 6

[[e-mail] (:bhearsum)]

Attachment: remove firefox specific factory stuff, take 2

This patch has been tested well in staging. It ran overnight without any issues. I also ran en-US release builds through it, which were totally fine.

Comment 7

[[e-mail] (:bhearsum)]

Attachment: simple master side followup

This patch makes app_name mandatory for branches, and adds it for Tracemonkey. I've switched the hardcoded 'firefox' for the l10n builds with branch['product_name']. Mostly, this just makes sure the necessary parameters get passed to the right factories.

Comment 8

[[e-mail] (:bhearsum)]

Comment on attachment 365207 [details] [diff] [review]
remove firefox specific factory stuff, take 2

Sigh, I forgot to check the l10n build logs. I busted the nightly l10n builds with this patch.

Comment 9

[[e-mail] (:bhearsum)]

Comment on attachment 365207 [details] [diff] [review]
remove firefox specific factory stuff, take 2

This one is actually fine, turns out I screwed up the config.py patch. Fixed one incoming.

Comment 10

[[e-mail] (:bhearsum)]

Attachment: use 'browser' for app_name, not 'firefox'

The bustage I saw earlier was like this:
configure: error: --enable-application value not recognized (firefox/build.mk does not exist).

Which made me notice 'app_name' for 1.9.1/m-c being wrong. We tried to '--enable-application=firefox', which doesn't work. This patch should fix the bustage. I also included the production patch here.

Comment 11

[Robert Kaiser]

Comment on attachment 365207 [details] [diff] [review]
remove firefox specific factory stuff, take 2

I couldn't spot any obvious problems on code inspection, of course only really using all this for other products will tell if there are any still hidden somewhere ;-)

Thanks for looking into this, it will greatly ease using the same definitions for all our products!

Comment 12

[[e-mail] (:bhearsum)]

Comment on attachment 365207 [details] [diff] [review]
remove firefox specific factory stuff, take 2

changeset:   219:4538cae1c559

Comment 13

[[e-mail] (:bhearsum)]

Comment on attachment 365226 [details] [diff] [review]
use 'browser' for app_name, not 'firefox'

changeset:   1009:deda897e7628

Comment 14

[[e-mail] (:bhearsum)]

Buildbot master config'ed for this.