Closed Bug 478229 Opened 13 years ago Closed 13 years ago

Abstract core factory classes in buildbotcustom for use in other products

Categories

(Release Engineering :: General, defect, P2)

defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: kairo, Assigned: bhearsum)

References

Details

Attachments

(2 files, 2 obsolete files)

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.
Status: NEW → ASSIGNED
Priority: -- → P2
Depends on: 479115
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?
Attachment #363157 - Flags: review?(gozer)
Attachment #363157 - Flags: review?(kairo)
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.
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.
(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 :)
Yeah, the non-prettynames case is obviously different.

I'll do some testing on my patch this week.
Attachment #363157 - Attachment is obsolete: true
Attachment #363157 - Flags: review?(kairo)
Attachment #363157 - Flags: review?(gozer)
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.
Attachment #365207 - Flags: review?(kairo)
Attachment #365207 - Flags: review?(catlee)
Attached patch simple master side followup (obsolete) — Splinter Review
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.
Attachment #365208 - Flags: review?(catlee)
Attachment #365207 - Flags: review?(catlee) → review+
Attachment #365208 - Flags: review?(catlee) → review+
Attachment #365207 - Attachment is obsolete: true
Attachment #365207 - Flags: review?(kairo)
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 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.
Attachment #365207 - Attachment is obsolete: false
Attachment #365207 - Flags: review?(kairo)
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.
Attachment #365208 - Attachment is obsolete: true
Attachment #365226 - Flags: review?(catlee)
Attachment #365226 - Flags: review?(catlee) → review+
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!
Attachment #365207 - Flags: review?(kairo) → review+
Comment on attachment 365207 [details] [diff] [review]
remove firefox specific factory stuff, take 2

changeset:   219:4538cae1c559
Attachment #365207 - Flags: checked‑in+ checked‑in+
Comment on attachment 365226 [details] [diff] [review]
use 'browser' for app_name, not 'firefox'

changeset:   1009:deda897e7628
Attachment #365226 - Flags: checked‑in+ checked‑in+
Buildbot master config'ed for this.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Blocks: 485820
Blocks: 486540
Product: mozilla.org → Release Engineering
You need to log in before you can comment on or make changes to this bug.