Last Comment Bug 666518 - Stop making extensions optional in SeaMonkey's installer
: Stop making extensions optional in SeaMonkey's installer
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: Installer (show other bugs)
: SeaMonkey 2.2 Branch
: x86 All
: -- major (vote)
: seamonkey2.4
Assigned To: Justin Wood (:Callek)
:
Mentors:
Depends on:
Blocks: 666823 666824 666825 666836 660427 666834
  Show dependency treegraph
 
Reported: 2011-06-23 01:01 PDT by Justin Wood (:Callek)
Modified: 2011-08-03 22:55 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
+
fixed
fixed


Attachments
drop optional components (496 bytes, patch)
2011-06-23 01:01 PDT, Justin Wood (:Callek)
kairo: review+
kairo: approval‑comm‑aurora+
kairo: approval‑comm‑beta+
Details | Diff | Splinter Review

Description Justin Wood (:Callek) 2011-06-23 01:01:33 PDT
Created attachment 541306 [details] [diff] [review]
drop optional components

For the 2.2b1 run, we had some issues with extensions again, similar to what delayed 2.1

These issues is what Bug 660427 was meant to solve, but seems more stuff is wrong in this round of updates, and rather than trying to untangle and maintain that mess I feel simply dropping the optional/ aspect of this all is a better solution for SeaMonkey 2.2 and future. (We can revisit the optional/ aspect with regard to "install to profile" on a future date, but the "install to app dir from installer" I'd vote to keep unless Firefox/MoCo want to direct support that potential)

The issues:
* en-US complete mar didn't notice the files that were meant to be optional, because it only bothered with |core/| files...
* repacks again didn't catch |optional/| on unpack and stuffed the langpacks to |core/|

I'm attaching a l10n-free patch, that does not attempt any dead-code removal. If this gets r+ I'll check this in, and backout Bug 660427 from trunk m-c. and Spin 2.2b2 without that patch.

This turned out to be simple patch to do, and avoids UX issues in the installer due to http://mxr.mozilla.org/comm-central/source/suite/installer/windows/nsis/custom.nsi#38

We can do a followup bug for the l10n and dead-code-removal parts of this.

By dropping this all codepaths simply put the extensions in |core/| as expected.

Also of note is the code at http://mxr.mozilla.org/comm-central/source/mozilla/extensions/venkman/locales/Makefile.in#111 (to EOF) and similarly for chatzilla seem to no longer be being hit at all, so no need to worry about that atm.

Review for KaiRo as he knows the RelEng reasons behind this quite well. Feedback to mcsmurf for he is the current module owner of the installer so should get a glance, and for record Ratty was OK with this plan after I advocated it and explained it.

For future followup we can re-instate the optional nature here, but without specifically having "optional" files. And instead restrict the ability for the application to push the extensions to profile.
Comment 1 Robert Kaiser 2011-06-23 04:13:49 PDT
(In reply to comment #0)
> We can do a followup bug for the l10n and dead-code-removal parts of this.

Please do (I'll look at the actual review later today).

> Also of note is the code at
> http://mxr.mozilla.org/comm-central/source/mozilla/extensions/venkman/
> locales/Makefile.in#111 (to EOF) and similarly for chatzilla seem to no
> longer be being hit at all, so no need to worry about that atm.

It is hit and must work, but only for an actual repack step in a locale other than en-US (and one that actually has a venkman L10n - try de for testing).
Comment 2 Robert Kaiser 2011-06-23 04:15:10 PDT
As a remark, I can only review from code inspection, I can't build currently as building is b0rken with a Linux 3.0 kernel due to some NSPR stuff (IIRC) that is forked by kernel version.
Comment 3 Justin Wood (:Callek) 2011-06-23 13:07:49 PDT
(In reply to comment #1)
> (In reply to comment #0)
> > Also of note is the code at
> > http://mxr.mozilla.org/comm-central/source/mozilla/extensions/venkman/
> > locales/Makefile.in#111 (to EOF) and similarly for chatzilla seem to no
> > longer be being hit at all, so no need to worry about that atm.
> 
> It is hit and must work, but only for an actual repack step in a locale
> other than en-US (and one that actually has a venkman L10n - try de for
> testing).

We don't seem to when I check de. We *do* hit repackage-zip-@AB_CD@. I tried searching the repack logs for |repackage-win32|.

For ref, our release repack run # 72 was de.

We *used to* but don't seem to now. [looks like we *should* of course if we wanted to keep optional working]
Comment 4 Robert Kaiser 2011-06-23 15:17:39 PDT
Comment on attachment 541306 [details] [diff] [review]
drop optional components

That all said, if this helps the whole issue, let's do it. What's the impact of this on the installer UI? I fear it's not a good idea to ship broken UI...
Comment 5 Robert Kaiser 2011-06-23 15:18:06 PDT
(In reply to comment #3)
> We don't seem to when I check de. We *do* hit repackage-zip-@AB_CD@. I tried
> searching the repack logs for |repackage-win32|.

Hah, good catch, actually. http://hg.mozilla.org/comm-central/rev/e58c1b036d73 removed this as part of bug 586848 and made everything go through repackage-zip apparently.

Followups on ChatZilla and venkman should be filed to get that dead code removed. Actually, I wonder if followup are also in order to make toolkit installer and build system get rid of the optional extension mechanism completely, as apparently it doesn't really work in a usable way nowadays and it's complicated code that would be helpful to have cleaned up.

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