1569539 - Calendar packaging clean-up

Status: RESOLVED FIXED

(Calendar :: Build Config, task)

Description

[Geoff Lankow (:darktrojan)]

I have several patches to do with the packaging of the calendar extensions. I'm going to use this bug for them all.

Comment 1

[Geoff Lankow (:darktrojan)]

Attachment: 1569539-calendar-buildfiles-1.diff

This patch removes a large amount of unused build config stuff. It also aligns the packaging of Lightning and the Google Provider so that they build the same way (as each other) in both artifact and full-build mode. Both are now exported as lightning.xpi and gdata-provider.xpi artifacts from a TaskCluster build.

Comment 2

[Geoff Lankow (:darktrojan)]

Attachment: 1569539-timezones-extension-1.diff

In this patch I remove the timezones extension. We don't use it any more, it's never been updated to manifest.json format, and as long as we keep up with the timezone data releases and uplift changes to ESR, there should be no need for it.

Comment 3

[Geoff Lankow (:darktrojan)]

(In reply to Geoff Lankow (:darktrojan) from comment #1)

Both are now exported as lightning.xpi and gdata-provider.xpi artifacts from a TaskCluster build.

I should've said: this is so that we can easily export them to ftp.mozilla.org (as in bug 1516816) and ATN (some day).

Comment 4

[Geoff Lankow (:darktrojan)]

Attachment: 1569539-sort-manifests-1.diff

This flattens the contents of the chrome directory somewhat, so that we don't have directories containing one subdirectories and nothing else. I believe that's a relic from when Lightning was unpacked on installation and contained JAR files instead of directories.

By my count this reduces the number of directories in Lightning by 10.

Since I was changing just about every line in the manifests, I sorted them, by the second column as I think that makes more sense than sorting by the first.

Comment 5

[Geoff Lankow (:darktrojan)]

Attachment: 1569539-sort-manifests-2.diff

Oops, broke a test.

Comment 6

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 9081195 [details] [diff] [review]
1569539-calendar-buildfiles-1.diff

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

::: calendar/providers/gdata/jar.mn
@@ +43,5 @@
>      skin/browserRequest.css                      (content/browserRequest.css)
> +
> +
> +gdata-provider-@AB_CD@.jar:
> +relativesrcdir comm/calendar/locales:

Glad this works in the jar.mn now!

Comment 7

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 9081196 [details] [diff] [review]
1569539-timezones-extension-1.diff

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

The timezones extension is a good idea, it is unfortunate that we never followed through. What I am concerned about is old versions of Thunderbird, it would be great to give them the latest versions of the timezone database without requiring an upgrade. Would you consider instead of removing it filing a bug to update it to the new format and maybe ship it as a system extension? We'd need some easy way to publish it (not sure how updates to system extensions work), but this way we could keep timezones up to date without requiring users to upgrade Thunderbird versions.

I'm going to r+ this anyway given the code you are removing no longer works, but I think it would be pretty great if the followup bug would be fixed :)

Comment 8

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 9081477 [details] [diff] [review]
1569539-sort-manifests-2.diff

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

r+ with this comment considered:

::: calendar/base/modules/calExtract.jsm
@@ -47,5 @@
>      }
> -
> -    if (!this.checkBundle(fallbackLocale)) {
> -        this.bundleUrl = this.packagedUrl;
> -        cal.WARN("Your installed Lightning only includes a single locale, extracting event info from other languages is likely inaccurate. You can install Lightning from addons.mozilla.org manually for multiple locale support.");

Is this no longer relevant? Are single-locale Thunderbird packaging all other languages, even for Lightning?

Comment 9

[Magnus Melin [:mkmelin]]

(In reply to Philipp Kewisch [:Fallen] [:📆] from comment #7)

The timezones extension is a good idea, it is unfortunate that we never
followed through. What I am concerned about is old versions of Thunderbird,
it would be great to give them the latest versions of the timezone database
without requiring an upgrade.

But those versions of Thunderbird is not supported, right? Or which are you referring to?

Comment 10

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Mostly, the future. If we are updating ESR now that is fine, but at some point there will be a next ESR and people might not be ready to upgrade. Having timezones available would be great then.

Comment 11

[Magnus Melin [:mkmelin]]

Not upgrading is not an option (with some months delay). After that it's EOL, unsafe and should in no way be supported.

Comment 12

[Geoff Lankow (:darktrojan)]

I think that, given we can ship future timezone changes now there should be no need for the extension, even for people lagging behind on updates. If a major problem emerges, the mechanism is still in place and we can reconstruct the extension and ship it.

Comment 13

[Jorg K (CEST = GMT+2)]

Hmm, no checkin-needed here yet and it's unclear whether some comments will be addressed.

Comment 14

[Geoff Lankow (:darktrojan)]

Attachment: 1569539-sort-manifests-3.diff

Same thing without the bitrot.

(In reply to Philipp Kewisch [:Fallen] [:📆] from comment #8)

Is this no longer relevant? Are single-locale Thunderbird packaging all
other languages, even for Lightning?

The warning itself may be useful still, so I've reinstated it. The line of code is useless as this.packagedUrl would be the same as this.bundleUrl, and has been wrong for some time. (We don't have JARs inside the package at all any more.)

Comment 15

[Geoff Lankow (:darktrojan)]

Try runs with this and bug 1561530. This needs to land second, or there'll be serious bitrot.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/a6f9ee9aa3cf
Remove outdated build config code for calendar extensions; r=Fallen
https://hg.mozilla.org/comm-central/rev/22d2296fed5d
Remove the calendar timezones extension; r=Fallen
https://hg.mozilla.org/comm-central/rev/f5d10ede4410
Flatten chrome folders in Lightning and GData packages; r=Fallen

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/4c9f754fb46d
Remove the calendar timezones extension from suite. rs=bustage-fix

Comment 18

[Frank-Rainer Grahl (:frg)]

Geoff,
you must not block on us but a heads up next time would be nice. You bascally tied Lightning to Thunderbird by removing the lightning version and not taking into account that it is not defined in suite by default. There was a reason that two paths were in the removed files to account for that.

0:25.57 File "d:\seamonkey\comm-central\python\mozbuild\mozbuild\preprocessor.py", line 783, in filter_substitution
0:25.57 return self.varsubst.sub(repl, aLine)
0:25.57 File "d:\seamonkey\comm-central\python\mozbuild\mozbuild\preprocessor.py", line 781, in repl
0:25.57 raise Preprocessor.Error(self, 'UNDEFINED_VAR', varname)
0:25.59 mozbuild.preprocessor.Error: ('$SRCDIR\comm\calendar\lightning\manifest.json', 10, 'UNDEFINED_VAR', 'THUNDERBIRD_VERSIO
_DISPLAY')
0:25.59 mozmake.EXE[4]: *** [d:/seamonkey/comm-central/config/rules.mk:1197: ../../../dist/bin/extensions/{e2fda1a4-762b-4020-b5ad-

Comment 19

[Frank-Rainer Grahl (:frg)]

Attachment: 1569539-suite.patch

This fixes the build for suite. Also skips the upload and moz-mill tests. Does the extension manager check against the app or gecko version? If app I would need to change it to the SeaMonkey version.

Comment 20

[Frank-Rainer Grahl (:frg)]

Attachment: Capture.PNG

FYI a local TB build creates empty directories with only a chrome/.mkdir.done in behind. The ones without the curly brackets. Not caused by the patch and seems harmless but probably not 100% right either.

Comment 21

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 9084089 [details] [diff] [review]
1569539-suite.patch

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

I wonder if it makes sense to just define THUNDERBIRD_VERSION (and SUITE_VERSION, etc) toplevel for all comm projects?  Anyway, here some comments:

::: calendar/lightning/Makefile.in
@@ +10,5 @@
>  final_parent = $(ABS_DIST)/bin/distribution/extensions
>  endif
>  
> +ifdef MOZ_SUITE
> +THUNDERBIRD_VERSION := $(shell cat $(commtopsrcdir)/mail/config/version.txt)

Not sure about these, I think it can also go in moz.build as EXPORTS?

@@ +13,5 @@
> +ifdef MOZ_SUITE
> +THUNDERBIRD_VERSION := $(shell cat $(commtopsrcdir)/mail/config/version.txt)
> +DEFINES += -DTHUNDERBIRD_VERSION='$(THUNDERBIRD_VERSION)'
> +THUNDERBIRD_VERSION_DISPLAY := $(shell cat $(commtopsrcdir)/mail/config/version_display.txt)
> +DEFINES += -DTHUNDERBIRD_VERSION_DISPLAY='$(THUNDERBIRD_VERSION_DISPLAY)'

Can these defines go into moz.build?

@@ +14,5 @@
> +THUNDERBIRD_VERSION := $(shell cat $(commtopsrcdir)/mail/config/version.txt)
> +DEFINES += -DTHUNDERBIRD_VERSION='$(THUNDERBIRD_VERSION)'
> +THUNDERBIRD_VERSION_DISPLAY := $(shell cat $(commtopsrcdir)/mail/config/version_display.txt)
> +DEFINES += -DTHUNDERBIRD_VERSION_DISPLAY='$(THUNDERBIRD_VERSION_DISPLAY)'
> +THUNDERBIRD_MAXVERSION := $(shell echo $(THUNDERBIRD_VERSION) | sed 's|\(^[0-9]*\)\.\([0-9]*\).*|\1|' ).*

This seems a bit fragile. The version I found in moz.build looked slightly less fragile, maybe go with that instead?

Comment 22

[Frank-Rainer Grahl (:frg)]

This seems a bit fragile. The version I found in moz.build looked slightly less fragile, maybe go with that instead?
That is basically part of the the old version which got removed. If you feel uneasy about it I can do a moz.build patch based on MOZILLA_VERSION for suite.

Did someone set TB to 69 for a test and see if this still works with the gecko version at 70? I didn't find which version the extension manager uses for comparision.

Comment 23

[Geoff Lankow (:darktrojan)]

Sorry Frank, I didn't mean to bust you.

I wonder if the tidiest fix might be to take this chunk from mail/moz.configure and add it to suite/moz.configure. (Similar to what Philipp said, but different.)

The extension manager works off the app version, but app version and gecko version are always aligned in Thunderbird anyway.

Comment 24

[Geoff Lankow (:darktrojan)]

Attachment: 1569539-suite-lightning-1.diff

I think this should work for both. It should also be easy to add version_parts[1] to major_version if that was wanted.

Comment 25

[Frank-Rainer Grahl (:frg)]

Comment on attachment 9084184 [details] [diff] [review]
1569539-suite-lightning-1.diff

This works but moz.configure needs to be changed. 
The "build_env.topsrcdir, 'mail'" needs to be changed to "build_env.topsrcdir, 'comm/mail'" 

Results in:
>  "version": "70.0a1",
>...
>   "id": "{e2fda1a4-762b-4020-b5ad-a41df1933103}",
>   "strict_min_version": "2.67a1",
>   "strict_max_version": "2.*"

Which is good enough for us. 

Thanks

Comment 26

[Frank-Rainer Grahl (:frg)]

Comment on attachment 9084184 [details] [diff] [review]
1569539-suite-lightning-1.diff

Any update here? Would be happy to see this go in.

Comment 27

[Jorg K (CEST = GMT+2)]

1569539-suite-lightning-1.diff needs review?

Comment 28

[Geoff Lankow (:darktrojan)]

Attachment: 1569539-suite-lightning-2.diff

Comment 29

[Paul Morris [:pmorris]]

Comment on attachment 9085903 [details] [diff] [review]
1569539-suite-lightning-2.diff

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

r+ Not an area that I'm familiar with, but the changes look ok.

Comment 30

[Frank-Rainer Grahl (:frg)]

Comment on attachment 9085903 [details] [diff] [review]
1569539-suite-lightning-2.diff

Thanks and r+

Btw. in mail/installer/removed-files.in you remove the old unpacked add-on dir conditionally depending on the current build. 
> # Remove unpacked lightning extension.
> #ifdef NIGHTLY_BUILD

Shouldn't you just do it in both locations? If someone installs a Beta over a Nightly or vice versa the directory might not get removed.

Comment 31

[Jorg K (CEST = GMT+2)]

Comment on attachment 9085903 [details] [diff] [review]
1569539-suite-lightning-2.diff

I've never seen that flag. We use checkin-needed. However, how can I check it in with an open question in comment #30?

Comment 32

[Frank-Rainer Grahl (:frg)]

I've never seen that flag.
Me neither so I thought I try it out :)

We use checkin-needed.
Thought it might be overkill with only one patch missing.

The question is unrelated to the patch to be checked in. Just stumbled over it for a SeaMonkey follow up here.

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/3b969aeca740
Follow-up: Allow SeaMonkey version numbers in Lightning manifests. r=pmorris,frg