Closed
Bug 1128070
Opened 9 years ago
Closed 9 years ago
Fix packaging for timezones extension
Categories
(Calendar :: General, defect)
Calendar
General
Tracking
(Not tracked)
RESOLVED
FIXED
4.0.0.1
People
(Reporter: darktrojan, Assigned: darktrojan)
References
Details
Attachments
(1 file, 3 obsolete files)
5.66 KB,
patch
|
Fallen
:
review+
jcranmer
:
review+
|
Details | Diff | Splinter Review |
I broke building the timezones extension fixing bug 1121415. Then I realised it never gets built anyway. So in this bug I'm going to add calendar/timezones/ into the build, and fix it.
Assignee | ||
Comment 1•9 years ago
|
||
Assignee | ||
Comment 2•9 years ago
|
||
I think I should also change the install.rdf, and set the minVersion of Thunderbird and SeaMonkey to the current version, since no earlier versions will be compatible.
Comment 3•9 years ago
|
||
Comment on attachment 8557344 [details] [diff] [review] 1128070-1.diff Review of attachment 8557344 [details] [diff] [review]: ----------------------------------------------------------------- Did you want to just build it, or also upload it to the ftp server? You likely need to include lightning-packager.mk and read the instructions there, as well as some extra code in the Thunderbird makefiles to make it also upload the timezones extension. The only concern I have is that this will automatically install the timezones extension for folks doing their own builds. We also want to --enable-calendar for nightly builds soon and then the extension would become part of the nightly builds. We can certainly solve this later on, given we have to decide how and if we want to not include the Provider for Google Calendar (which will have the same issues), but I wanted to bring this up. What do you think? I agree we should change the minversion. r- mostly for the Makefile.in changes and given we need a new patch anyway to set the minVersion. ::: calendar/timezones/Makefile.in @@ +18,5 @@ > PREF_JS_EXPORTS = $(srcdir)/defaults/preferences.js > > ifndef DISABLE_LIGHTNING_INSTALL > # install as a global extension in dist/bin/extensions/ > INSTALL_EXTENSION_ID = calendar-timezones@mozilla.org Using this will put it into dist/bin/distribution/ and install it into the profile. I have an alternative in lightning-packager.mk that would install it into dist/bin/extensions/. @@ +33,5 @@ > -DTIMEZONES_VERSION=$(TIMEZONES_VERSION) \ > $(NULL) > > libs:: > + $(NSINSTALL) -m 0644 $(srcdir)/zones.json $(FINAL_TARGET)/timezones I think the new way to do this is to add FINAL_TARGET_FILES += [ 'zones.json' ] to the moz.build. Can you confirm that works? ::: calendar/timezones/version.py @@ +7,5 @@ > +json_file = os.path.join(os.path.dirname(os.path.realpath(__file__)), "zones.json") > + > +with open(json_file, "r") as fp: > + data = json.load(fp) > + print data["version"] Lots of spaces here, is that a tab? ::: mail/app.mozbuild @@ +21,5 @@ > if CONFIG['MOZ_CALENDAR']: > + DIRS += [ > + '/calendar/lightning', > + '/calendar/timezones' > + ] Its a minor change, but you need a suite and mail reviewer for these changes.
Attachment #8557344 -
Flags: review?(philipp) → review-
Assignee | ||
Comment 4•9 years ago
|
||
(In reply to Philipp Kewisch [:Fallen] from comment #3) > Did you want to just build it, or also upload it to the ftp server? You > likely need to include lightning-packager.mk and read the instructions > there, as well as some extra code in the Thunderbird makefiles to make it > also upload the timezones extension. Actually I don't even really want it to be built, given that will be exactly the same unless the timezones are updated or the version numbers change. But at the moment I *can't* build it, because the build system knows nothing about that directory and just ignores it. (I suspect that the introduction of mach caused this.) > The only concern I have is that this will automatically install the > timezones extension for folks doing their own builds. We also want to > --enable-calendar for nightly builds soon and then the extension would > become part of the nightly builds. We can certainly solve this later on, > given we have to decide how and if we want to not include the Provider for > Google Calendar (which will have the same issues), but I wanted to bring > this up. What do you think? I've taken that bit out of the Makefile. This extension shouldn't be installed unless needed. > @@ +33,5 @@ > > -DTIMEZONES_VERSION=$(TIMEZONES_VERSION) \ > > $(NULL) > > > > libs:: > > + $(NSINSTALL) -m 0644 $(srcdir)/zones.json $(FINAL_TARGET)/timezones > > I think the new way to do this is to add > > FINAL_TARGET_FILES += [ > 'zones.json' > ] > > to the moz.build. Can you confirm that works? I can confirm it doesn't. Nor can I make it work in moz.build, where it should, AFAICT.
Assignee | ||
Comment 5•9 years ago
|
||
Attachment #8557344 -
Attachment is obsolete: true
Attachment #8557541 -
Flags: review?(philipp)
Comment 7•9 years ago
|
||
Comment on attachment 8557541 [details] [diff] [review] 1128070-2.diff Review of attachment 8557541 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Geoff Lankow (:darktrojan) from comment #4) > (In reply to Philipp Kewisch [:Fallen] from comment #3) > > > The only concern I have is that this will automatically install the > > timezones extension for folks doing their own builds. We also want to > > --enable-calendar for nightly builds soon and then the extension would > > become part of the nightly builds. We can certainly solve this later on, > > given we have to decide how and if we want to not include the Provider for > > Google Calendar (which will have the same issues), but I wanted to bring > > this up. What do you think? > > I've taken that bit out of the Makefile. This extension shouldn't be > installed unless needed. The extension is still installed in to dist/bin/distribution/extenions when you have INSTALL_EXTENSION_ID set. If you want to prohibit installing it fully, you'd have to change the ifndef DISABLE_LIGHTNING_INSTALL to ifdef ENABLE_TIMEZONES_INSTALL. Also, this is the code that is required to put it into dist/bin/extensions instead: XPI_EM_ID = calendar-timezones@mozilla.org ifndef DISABLE_LIGHTNING_INSTALL # install as a global extension in dist/bin/extensions/ XPI_INSTALL_EXTENSION = $(XPI_EM_ID) endif # ...and at the end of the file include $(topsrcdir)/calendar/lightning/lightning-packager.mk > > FINAL_TARGET_FILES += [ > > 'zones.json' > > ] > > > > to the moz.build. Can you confirm that works? > > I can confirm it doesn't. Nor can I make it work in moz.build, where it > should, AFAICT. While mach build-backend will update the install manifests, this does not create the symlinks. Running mach build again created those links for me, so it should be fine. I'm attaching a debitrotted patch, the only thing left to do is to decide on ENABLE_TIMEZONES_INSTALL vs DISABLE_LIGHTNING_INSTALL.
Attachment #8557541 -
Flags: review?(philipp) → review-
Comment 8•9 years ago
|
||
Attachment #8557541 -
Attachment is obsolete: true
Attachment #8561008 -
Flags: feedback?(geoff)
Assignee | ||
Comment 9•9 years ago
|
||
This is basically the same patch as yours, but leaving the XPI behind in dist/xpi-stage instead of installing it anywhere. I figured out why mach wasn't doing what I wanted, and I'm going to blame mach :-)
Attachment #8561008 -
Attachment is obsolete: true
Attachment #8561008 -
Flags: feedback?(geoff)
Attachment #8563081 -
Flags: review?(philipp)
Comment 10•9 years ago
|
||
Comment on attachment 8563081 [details] [diff] [review] 1128070-4.diff Looks good, r=philipp. Thanks for the persistence :-)
Attachment #8563081 -
Flags: review?(philipp) → review+
Assignee | ||
Comment 11•9 years ago
|
||
Comment on attachment 8563081 [details] [diff] [review] 1128070-4.diff Mike, just need approval for the mail/ and suite/ changes.
Attachment #8563081 -
Flags: review?(mh+mozilla)
Comment 12•9 years ago
|
||
Comment on attachment 8563081 [details] [diff] [review] 1128070-4.diff Review of attachment 8563081 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Geoff Lankow (:darktrojan) from comment #11) > Comment on attachment 8563081 [details] [diff] [review] > 1128070-4.diff > > Mike, just need approval for the mail/ and suite/ changes. Jcranmer is better suited for that.
Attachment #8563081 -
Flags: review?(mh+mozilla) → review?(Pidgeot18)
Updated•9 years ago
|
Attachment #8563081 -
Flags: review?(Pidgeot18) → review+
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Updated•9 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 4.0
You need to log in
before you can comment on or make changes to this bug.
Description
•