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