Closed Bug 1569539 Opened 4 months ago Closed 4 months ago

Calendar packaging clean-up

Categories

(Calendar :: Build Config, task)

task
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: darktrojan, Assigned: darktrojan)

References

Details

Attachments

(5 files, 4 obsolete files)

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

Component: General → Build Config

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.

Attachment #9081195 - Flags: review?(philipp)

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.

Attachment #9081196 - Flags: review?(philipp)

(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).

Attached patch 1569539-sort-manifests-1.diff (obsolete) — Splinter Review

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.

Attachment #9081199 - Flags: review?(philipp)
Attached patch 1569539-sort-manifests-2.diff (obsolete) — Splinter Review

Oops, broke a test.

Attachment #9081199 - Attachment is obsolete: true
Attachment #9081199 - Flags: review?(philipp)
Attachment #9081477 - Flags: review?(philipp)
Blocks: 1516816
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!
Attachment #9081195 - Flags: review?(philipp) → review+
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 :)
Attachment #9081196 - Flags: review?(philipp) → review+
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?
Attachment #9081477 - Flags: review?(philipp) → review+

(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?

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.

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

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.

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

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

Attachment #9081477 - Attachment is obsolete: true
Attachment #9083590 - Flags: review+

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

Keywords: checkin-needed

Pushed by mozilla@jorgk.com:
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

Status: ASSIGNED → RESOLVED
Closed: 4 months ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 70
Pushed by frgrahl@gmx.net:
https://hg.mozilla.org/comm-central/rev/4c9f754fb46d
Remove the calendar timezones extension from suite. rs=bustage-fix

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-

Flags: needinfo?(geoff)
Attached patch 1569539-suite.patch (obsolete) — Splinter Review

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.

Attachment #9084089 - Flags: review?(philipp)
Attached image 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 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?
Attachment #9084089 - Flags: review?(philipp) → review+

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.

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.

Flags: needinfo?(geoff)
Attached patch 1569539-suite-lightning-1.diff (obsolete) — Splinter Review

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

Attachment #9084184 - Flags: feedback?(frgrahl)
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
Attachment #9084184 - Flags: feedback?(frgrahl) → feedback+
Comment on attachment 9084184 [details] [diff] [review]
1569539-suite-lightning-1.diff

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

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

Flags: needinfo?(geoff)
Attachment #9084184 - Attachment is obsolete: true
Flags: needinfo?(geoff)
Attachment #9085903 - Flags: review?(paul)
Attachment #9085903 - Flags: review?(frgrahl)
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.
Attachment #9085903 - Flags: review?(paul) → review+
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.
Attachment #9085903 - Flags: review?(frgrahl) → review+
Attachment #9084089 - Attachment is obsolete: true
Attachment #9085903 - Flags: checkin?(jorgk)
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?
Attachment #9085903 - Flags: checkin?(jorgk)

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.

Keywords: checkin-needed
Duplicate of this bug: 1563902

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

Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.