Closed Bug 1130854 Opened 5 years ago Closed 5 years ago

Package Lightning with Thunderbird

Categories

(Thunderbird :: Build Config, defect)

38 Branch
defect
Not set

Tracking

(thunderbird38 fixed, thunderbird39 fixed)

RESOLVED FIXED
Thunderbird 40.0
Tracking Status
thunderbird38 --- fixed
thunderbird39 --- fixed

People

(Reporter: Fallen, Assigned: Fallen)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 3 obsolete files)

This bug is meant to package Lightning in mail/installer/package-manifest.in.

Current issues include bug 910660, and the startup cache breaking because both ical.js and libical have a calIDateTime interface.
Not only c-c and c-a, but also for betas and releases now. For nightly builds, I think we should package the files into Contents/Resources/extensions, while for betas and releases we will do Contents/Resources/distribution/extensions.
Summary: Package Lightning with Thunderbird for c-c and c-a builds → Package Lightning with Thunderbird
Attached patch Fix - v1 (obsolete) β€” β€” Splinter Review
This should take care, given the patch for bug 910660 is applied.
Attachment #8571021 - Flags: review?(Pidgeot18)
One thing we might want to add is augmenting the version number of Lightning, at least on official beta/release builds to be something like 4.0packaged. This would let Thunderbird use the packaged Lightning version, but upgrade to the official version via AMO on the next update check.

Another thing left to do is make sure l10n repacks on beta/release are doing the right thing.
Depends on: 1142261
Attached patch Fix - v2 (obsolete) β€” β€” Splinter Review
Ok, updated patch. This should be fairly final, I'll handle repacks and such in a separate bug.
Attachment #8571021 - Attachment is obsolete: true
Attachment #8571021 - Flags: review?(Pidgeot18)
Attachment #8576269 - Flags: review?(Pidgeot18)
Depends on: 1143163
Comment on attachment 8576269 [details] [diff] [review]
Fix - v2

Review of attachment 8576269 [details] [diff] [review]:
-----------------------------------------------------------------

For some reason, I've been having trouble importing this patch to check a local build (see bug 1144518).

This may have some impacts on buildbot steps--I'd like to see a green try push before this lands.

Arguably, you may also want to default lightning to being enabled when we build Thunderbird.

::: calendar/base/backend/install.rdf
@@ +1,4 @@
> +<?xml version="1.0"?>
> +<!--
> +  This is a dummy file to convince the packager that the components/
> +  subdirectory is not a separate base. Its needed because the components/

Nit: It's

::: mail/installer/package-manifest.in
@@ +854,5 @@
>  @RESPATH@/components/@DLL_PREFIX@mozgnome@DLL_SUFFIX@
>  #endif
> +
> +[calendar]
> +#ifdef NIGHTLY_BUILD

This section should be keyed off of Lightning being enabled in the Thunderbird build.
Attachment #8576269 - Flags: review?(Pidgeot18) → review+
Depends on: 1147207
OS: Mac OS X → All
Hardware: x86 → All
Version: 30 → 38
Depends on: 1153192
Depends on: 1153327
Pushed to comm-central changeset 566579330863
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 39.0
argh sorry, that was just a try push.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: Thunderbird 39.0 → ---
Status: REOPENED → ASSIGNED
Attached patch Fix - v3 (obsolete) β€” β€” Splinter Review
Comments taken into account and some additions to make sure it will actually work at the end.

I haven't made --enable-calendar the default, first of all since I didn't find out how to do this on first sight, and because I don't want to put too much into this bug.
Attachment #8576269 - Attachment is obsolete: true
Attachment #8590952 - Flags: review?(Pidgeot18)
Comment on attachment 8590952 [details] [diff] [review]
Fix - v3

Taking back review for now, still errors with unification *sigh*. We might just have to dump this in once there is a green try build and see how it goes for the beta.
Attachment #8590952 - Flags: review?(Pidgeot18)
Depends on: 1153790
Attached patch Part1: Basic Packaging β€” β€” Splinter Review
Attachment #8590952 - Attachment is obsolete: true
Attachment #8591859 - Flags: review?(Pidgeot18)
We moved to using em:realTagetPlatform to have a platform value to unify on but make it platform independent in case ical.js is enabled. ssitter had some doubts in that bug and I think he was right, there is no need to do that right now. As a nice side effect, with my patch from bug 1153790 the target platform is automatically merged by the packager.

I haven't switched the Lightning postflight code to use the packager yet, I'd rather do this separately because I have something that works now and I'd rather not fiddle with it.
This is that patch that actually makes unification work. With the packaging enabled we need to unify twice, once for xpi-stage/lightning and once for the packaged Lightning. Of course I could copy files over, but since the packager produces a different layout than in xpi-stage/lightning I didn't want to take any chances. I also needed to move the order in the mozconfig so that the Lightning unification is called before Thunderbird's.
Attachment #8591864 - Flags: review?(Pidgeot18)
Attachment #8591860 - Flags: review?(Pidgeot18)
Green try run, admittedly before splitting into three parts:

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=da24c38fc3b5
Part 4 is needed because the unifier doesn't like .jar files and glandium has valid arguments against unifying them in bug 1153790.
Attachment #8592183 - Flags: review?(Pidgeot18)
Comment on attachment 8591859 [details] [diff] [review]
Part1: Basic Packaging

Review of attachment 8591859 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/locales/Makefile.in
@@ +228,5 @@
> +ifdef NIGHTLY_BUILD
> +MOZ_PKG_EXTRAL10N += extensions/{e2fda1a4-762b-4020-b5ad-a41df1933103}=$(DIST)/xpi-stage/lightning-$(AB_CD)
> +else
> +MOZ_PKG_EXTRAL10N += distribution/extensions/{e2fda1a4-762b-4020-b5ad-a41df1933103}=$(DIST)/xpi-stage/lightning-$(AB_CD)
> +endif

I'm nervous about passing parameters into l10n.mk after that file is included into the Makefile. It will certainly break if anyone uses := anywhere to define the variable.
Attachment #8591859 - Flags: review?(Pidgeot18) → review+
Attachment #8591860 - Flags: review?(Pidgeot18) → review+
Comment on attachment 8591864 [details] [diff] [review]
Part3: Make mac unification work

Review of attachment 8591864 [details] [diff] [review]:
-----------------------------------------------------------------

::: calendar/lightning/build/universal.mk
@@ +46,5 @@
> +endef
> +
> +define unify_lightning_repackage
> +cd $(DIST_UNI)/$1/$2 && $(ZIP) -qr ../$(XPI_PKGNAME).xpi *
> +endef

Why do you need to call this for dist/xpi-stage/lightning but not the Contents/Resources/ path? If you only need to do it once, there's no need to put this in a define.
Attachment #8592183 - Flags: review?(Pidgeot18) → review+
(In reply to Joshua Cranmer [:jcranmer] from comment #17)
> Why do you need to call this for dist/xpi-stage/lightning but not the
> Contents/Resources/ path? If you only need to do it once, there's no need to
> put this in a define.

The Lightning in Contents/Resources is packaged by the Thunderbird packager, while the other one needs to be packaged by us. I put it in a define to keep the actual target clean, I thought it would be nice to just call macros from the target and do the work separately.
(In reply to Joshua Cranmer [:jcranmer] from comment #16)
> > +MOZ_PKG_EXTRAL10N += distribution/extensions/{e2fda1a4-762b-4020-b5ad-a41df1933103}=$(DIST)/xpi-stage/lightning-$(AB_CD)
> I'm nervous about passing parameters into l10n.mk after that file is
> included into the Makefile. It will certainly break if anyone uses :=
> anywhere to define the variable.

Wouldn't it break if := is used even if l10n.mk is included afterwards? Anyway, I can move the include down to the bottom if you like.
Attachment #8591864 - Flags: review?(Pidgeot18) → review+
Target Milestone: --- → Thunderbird 40.0
Depends on: 1159447
I think this caused Bug 1160805.
Blocks: 516026
You need to log in before you can comment on or make changes to this bug.