Last Comment Bug 1128070 - Fix packaging for timezones extension
: Fix packaging for timezones extension
Status: RESOLVED FIXED
:
Product: Calendar
Classification: Client Software
Component: General (show other bugs)
: unspecified
: All All
-- normal (vote)
: 4.0.0.1
Assigned To: Geoff Lankow (:darktrojan)
:
:
Mentors:
Depends on:
Blocks: 1121415
  Show dependency treegraph
 
Reported: 2015-01-30 14:04 PST by Geoff Lankow (:darktrojan)
Modified: 2015-02-22 02:09 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
1128070-1.diff (4.11 KB, patch)
2015-01-30 14:06 PST, Geoff Lankow (:darktrojan)
philipp: review-
Details | Diff | Splinter Review
1128070-2.diff (5.35 KB, patch)
2015-01-31 15:46 PST, Geoff Lankow (:darktrojan)
philipp: review-
Details | Diff | Splinter Review
1128070-3.diff (4.44 KB, patch)
2015-02-08 05:56 PST, Philipp Kewisch [:Fallen]
no flags Details | Diff | Splinter Review
1128070-4.diff (5.66 KB, patch)
2015-02-11 15:20 PST, Geoff Lankow (:darktrojan)
philipp: review+
Pidgeot18: review+
Details | Diff | Splinter Review

Description User image Geoff Lankow (:darktrojan) 2015-01-30 14:04:09 PST
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.
Comment 1 User image Geoff Lankow (:darktrojan) 2015-01-30 14:06:38 PST
Created attachment 8557344 [details] [diff] [review]
1128070-1.diff
Comment 2 User image Geoff Lankow (:darktrojan) 2015-01-30 14:09:41 PST
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 User image Philipp Kewisch [:Fallen] 2015-01-31 04:38:16 PST
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.
Comment 4 User image Geoff Lankow (:darktrojan) 2015-01-31 15:45:33 PST
(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.
Comment 5 User image Geoff Lankow (:darktrojan) 2015-01-31 15:46:25 PST
Created attachment 8557541 [details] [diff] [review]
1128070-2.diff
Comment 6 User image Philipp Kewisch [:Fallen] 2015-02-01 09:23:17 PST
*** Bug 1128293 has been marked as a duplicate of this bug. ***
Comment 7 User image Philipp Kewisch [:Fallen] 2015-02-08 05:54:50 PST
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.
Comment 8 User image Philipp Kewisch [:Fallen] 2015-02-08 05:56:29 PST
Created attachment 8561008 [details] [diff] [review]
1128070-3.diff
Comment 9 User image Geoff Lankow (:darktrojan) 2015-02-11 15:20:31 PST
Created attachment 8563081 [details] [diff] [review]
1128070-4.diff

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 :-)
Comment 10 User image Philipp Kewisch [:Fallen] 2015-02-12 02:19:42 PST
Comment on attachment 8563081 [details] [diff] [review]
1128070-4.diff

Looks good, r=philipp. Thanks for the persistence :-)
Comment 11 User image Geoff Lankow (:darktrojan) 2015-02-13 02:28:46 PST
Comment on attachment 8563081 [details] [diff] [review]
1128070-4.diff

Mike, just need approval for the mail/ and suite/ changes.
Comment 12 User image Mike Hommey [:glandium] 2015-02-17 18:14:42 PST
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.

Note You need to log in before you can comment on or make changes to this bug.