Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Fix packaging for timezones extension

RESOLVED FIXED in 4.0.0.1

Status

Calendar
General
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: darktrojan, Assigned: darktrojan)

Tracking

unspecified
4.0.0.1

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

3 years ago
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

3 years ago
Created attachment 8557344 [details] [diff] [review]
1128070-1.diff
Assignee: nobody → geoff
Status: NEW → ASSIGNED
Attachment #8557344 - Flags: review?(philipp)
(Assignee)

Comment 2

3 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 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

3 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

3 years ago
Created attachment 8557541 [details] [diff] [review]
1128070-2.diff
Attachment #8557344 - Attachment is obsolete: true
Attachment #8557541 - Flags: review?(philipp)
Duplicate of this bug: 1128293
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-
Created attachment 8561008 [details] [diff] [review]
1128070-3.diff
Attachment #8557541 - Attachment is obsolete: true
Attachment #8561008 - Flags: feedback?(geoff)
(Assignee)

Comment 9

3 years ago
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 :-)
Attachment #8561008 - Attachment is obsolete: true
Attachment #8561008 - Flags: feedback?(geoff)
Attachment #8563081 - Flags: review?(philipp)
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

3 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 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)
Attachment #8563081 - Flags: review?(Pidgeot18) → review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 13

2 years ago
https://hg.mozilla.org/comm-central/rev/ab1b861da0f2

Updated

2 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 2 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.