Closed
Bug 594858
Opened 14 years ago
Closed 14 years ago
Do not ship Feedback XPI with Firefox 4 final
Categories
(Firefox Build System :: General, defect)
Firefox Build System
General
Tracking
(blocking2.0 betaN+)
RESOLVED
FIXED
mozilla4.0b10
Tracking | Status | |
---|---|---|
blocking2.0 | --- | betaN+ |
People
(Reporter: mossop, Assigned: mossop)
References
Details
(Keywords: qawanted, Whiteboard: [hardblocker])
Attachments
(2 files, 2 obsolete files)
(deleted),
patch
|
bhearsum
:
feedback+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
robert.strong.bugs
:
review+
|
Details | Diff | Splinter Review |
Also decide if we want to add it to removed files to delete it from users who update.
Assignee | ||
Updated•14 years ago
|
blocking2.0: --- → final+
Comment 1•14 years ago
|
||
Morphing slightly:
- we do NOT want to ship the Feedback XPI with Firefox 4 final
- we DO want to keep the Feedback XPI running for Firefox 4 users who joined our beta program (ie: we don't want to remove it from users who update)
Summary: Turn off Feedback for Firefox 4 final → Do not ship Feedback XPI with Firefox 4 final
Assignee | ||
Comment 2•14 years ago
|
||
So there are no surprises, the consequences of the current way we package Feedback are that if we leave it there for users who upgrade then they still won't have the option to uninstall it through the add-ons manager, they will be able to disable it.
Comment 3•14 years ago
|
||
Yup, that's fine.
Comment 4•14 years ago
|
||
Mossop, adding feedback is keyed off update channel set to beta ? So that it won't be packaged when we switch to release for 4.0rc1 build1 ?
Assignee | ||
Comment 5•14 years ago
|
||
That is probably true, we'll still need to turn this off proper so we don't end up shipping it with the 4.0.x spins right?
Comment 6•14 years ago
|
||
You mean with the 4.0.x versions that have the beta channel set? I actually think we *want* the Feedback XPI included with those.
Comment 7•14 years ago
|
||
From code inspection (danger, will robinson!) we only package testpilot at build time when the update channel is beta
http://mxr.mozilla.org/mozilla-central/source/browser/installer/package-manifest.in#412
http://mxr.mozilla.org/mozilla-central/source/browser/app/profile/extensions/Makefile.in#49
So when we switch from beta to release at first 4.0rc1 build the exe/dmg/tar.bz's from won't contain testpilot. Historically, the update channel has changed then so that all the RCs are genuine candidates for final, without need for further recompile. That's all for new installs.
For existing installs, if we don't add the testpilot files to removed-files.in then _complete_ updates to 4.0rc1 (or later) will not remove it. But I expect a partial update between the last beta and 4.0rc1 will remove them, because it calculates a list of files that have been removed between the two mar files. Is that a problem, or has the extension already been copied to the profile ?
Assignee | ||
Comment 8•14 years ago
|
||
(In reply to comment #7)
> From code inspection (danger, will robinson!) we only package testpilot at
> build time when the update channel is beta
>
> http://mxr.mozilla.org/mozilla-central/source/browser/installer/package-manifest.in#412
>
> http://mxr.mozilla.org/mozilla-central/source/browser/app/profile/extensions/Makefile.in#49
> So when we switch from beta to release at first 4.0rc1 build the
> exe/dmg/tar.bz's from won't contain testpilot. Historically, the update channel
> has changed then so that all the RCs are genuine candidates for final, without
> need for further recompile. That's all for new installs.
So sounds like there is nothing we need to do here to stop this shipping in final. If we want to make it ship in the 4.0.x RCs then we'd need to take action but that doesn't sound desirable.
> For existing installs, if we don't add the testpilot files to removed-files.in
> then _complete_ updates to 4.0rc1 (or later) will not remove it. But I expect a
> partial update between the last beta and 4.0rc1 will remove them, because it
> calculates a list of files that have been removed between the two mar files. Is
> that a problem, or has the extension already been copied to the profile ?
I do not believe that partial mars remove files between builds do they?
Comment 9•14 years ago
|
||
Assignee | ||
Comment 10•14 years ago
|
||
So I guess the only way to leave it in lace would be to only produce complete mars for beta -> rc/final updates?
Updated•14 years ago
|
Assignee: nobody → dtownsend
Updated•14 years ago
|
Whiteboard: [hardblocker]
Updated•14 years ago
|
Whiteboard: [hardblocker] → [hardblocker][hardblocker]
Updated•14 years ago
|
Whiteboard: [hardblocker][hardblocker] → [hardblocker]
Assignee | ||
Comment 12•14 years ago
|
||
Tentatively over to Nick then, who I'm sure will reassign to the right person or tell me if I'm smoking crack.
Assignee: dtownsend → nrthomas
Comment 13•14 years ago
|
||
It doesn't sound like there's any changes I need to make in the Feedback code for this bug, right? I just wanted to double-check that. Please let me know ASAP if there are.
Comment 14•14 years ago
|
||
Don't think so, but what does Mossop think ?
Comment 15•14 years ago
|
||
Ben, how about a temporary change in m-c like this ? (untested)
We'd have to move the UPDATE_PACKAGING_RXX tag to this, and revert it afterwards, but this should get us automatically-generated partials that don't remove Feedback at the bN -> RC1 transition.
Attachment #503403 -
Flags: feedback?(bhearsum)
Comment 16•14 years ago
|
||
Seems OK to me. Am I right in thinking that 4.0[rcN|.N] -> 4.0[rcN|.N] will work because the from version doesn't have Test Pilot, and thus make_incremental_updates.py doesn't think to remove it?
Updated•14 years ago
|
Attachment #503403 -
Flags: feedback?(bhearsum) → feedback+
Assignee | ||
Comment 17•14 years ago
|
||
Just to flip flop a little. If we take bug 474289 before the next beta then we can actually fix this by just adding feedback to the removed-files list as normal. Going to try to make that happen.
Assignee | ||
Comment 18•14 years ago
|
||
Taking to do the repackaging magic now that bug 474289 is fixed
Assignee: nrthomas → dtownsend
Assignee | ||
Comment 19•14 years ago
|
||
The current plan requires that we have the repackaging changes in place by the last beta, unfortunately I've realised some snags which might make both options we've thought of so far unusable
blocking2.0: final+ → betaN+
Assignee | ||
Comment 20•14 years ago
|
||
Here is the problem. In order to save time and hassle and still have Feedback be localised we placed all of the locale files in with the main browser locales, so Feedback's strings end up in en-US.jar.
If we were to follow the original plan here and just stop shipping feedback then users upgrading from beta to RC would have the feedback UI and code but no strings which would be broken.
For the new plan to ship feedback as a distribution add-on we need to do some additional steps to make the localised strings be a part of the extension rather than having then end up in en-US.jar.
This patch is my first pass at doing that, it is probably hacky and I'm hoping Axel can make a better suggestion but it does appear to work, en-US builds have the extension in the right place and a l10n repack correctly replaces the strings in the extension with one problem, the language code on the locale line in chrome.manifest for the extension doesn't get updated. This is probably not a big deal as it will still load the strings, I verified this in a German repack.
Basic plan of attack is this:
1. Have the localised strings get put in a jar outside of the normal chrome directory (dist/xpi-stage/feedback was chosen).
2. Have the main en-US packaging code copy the additional strings and chrome.manifest from there into the extension's directory.
3. When the langpack build runs the new feedback strings end up in a slightly odd place, dist/xpi-stage/xpi-stage/feedback.
4. Before the repackage-zip step copy those into the langpack xpi directory
5. The repackage-zip step then copies them into the right place before packaging up the build.
Attachment #505615 -
Flags: feedback?(l10n)
Comment 21•14 years ago
|
||
I'd personally just go on and make the feedback l10n files be part of fx unconditionally. Beta channel or not. For the dashboard, they're not conditional anyway, so localizers will assume they have to do it, if we package them or not.
That should be a two line change in jar.mn, right?
Assignee | ||
Comment 22•14 years ago
|
||
Well I guess that is a simpler solution. This packages the feedback add-on as a packed xpi and adds the old location to removed-files.in as well as always packages the localisations.
Attachment #505668 -
Flags: review?(robert.bugzilla)
Comment 23•14 years ago
|
||
Comment on attachment 505668 [details] [diff] [review]
packaging patch rev 1
>diff --git a/browser/installer/removed-files.in b/browser/installer/removed-files.in
>--- a/browser/installer/removed-files.in
>+++ b/browser/installer/removed-files.in
>@@ -89,16 +89,133 @@ defaults/profile/extensions/{972ce4c6-7e
>...
>+extensions/testpilot@labs.mozilla.com/skin/all/badge-default.png
>+extensions/testpilot@labs.mozilla.com/skin/all/bg.jpg
>+extensions/testpilot@labs.mozilla.com/skin/all/css/screen-standalone.css
>+extensions/testpilot@labs.mozilla.com/skin/all/dino_32x32.png
>+extensions/testpilot@labs.mozilla.com/skin/all/images
You've included the images directory
Comment 24•14 years ago
|
||
Comment on attachment 505668 [details] [diff] [review]
packaging patch rev 1
>diff --git a/browser/installer/package-manifest.in b/browser/installer/package-manifest.in
>--- a/browser/installer/package-manifest.in
>+++ b/browser/installer/package-manifest.in
>@@ -416,17 +416,17 @@
>
> ; [Browser Chrome Files]
> @BINPATH@/chrome/browser@JAREXT@
> @BINPATH@/chrome/browser.manifest
> @BINPATH@/extensions/{972ce4c6-7e08-4474-a285-3208198ce6fd}/install.rdf
> @BINPATH@/extensions/{972ce4c6-7e08-4474-a285-3208198ce6fd}/icon.png
> @BINPATH@/extensions/{972ce4c6-7e08-4474-a285-3208198ce6fd}/preview.png
> #if MOZ_UPDATE_CHANNEL == beta
>-@BINPATH@/extensions/testpilot@labs.mozilla.com/*
>+@BINPATH@/distribution/extensions/testpilot@labs.mozilla.com/*
Shouldn't this should be testpilot@labs.mozilla.com.xpi?
Comment 25•14 years ago
|
||
Comment on attachment 505668 [details] [diff] [review]
packaging patch rev 1
>diff --git a/browser/app/profile/extensions/Makefile.in b/browser/app/profile/extensions/Makefile.in
>--- a/browser/app/profile/extensions/Makefile.in
>+++ b/browser/app/profile/extensions/Makefile.in
>@@ -35,27 +35,31 @@
>...
> ifeq (beta,$(MOZ_UPDATE_CHANNEL))
> EXTENSIONS = \
> testpilot@labs.mozilla.com \
> $(NULL)
>
>-define _INSTALL_EXTENSIONS
>-$(PYTHON) $(topsrcdir)/config/nsinstall.py $(wildcard $(srcdir)/$(dir)/*) $(DIST)/bin/extensions/$(dir)
>+define _INSTALL_EXTENSION
>+cd $(srcdir)/$(dir) && \
>+ $(ZIP) -r9D $(DISTROEXT)/$(dir).xpi *
What do you think of passing X to exclude extra file attributes as well?
Minusing to verify the fixes in an updated patch since if there are any problems they won't be caught until the beta is built.
Attachment #505668 -
Flags: review?(robert.bugzilla) → review-
Comment 26•14 years ago
|
||
Comment on attachment 505615 [details] [diff] [review]
Change packaging and mangle locales
I like the new patch, so feedback- on this one.
Attachment #505615 -
Flags: feedback?(l10n) → feedback-
Assignee | ||
Comment 27•14 years ago
|
||
Fixes the problems and the packaged add-on to removed-files for non-beta-channel builds so the RCs will not include it and will remove it from the app dir on upgrade from b10 or b11.
Ran tests by installing on windows over b9 with a nightly with the beta channel and then a nightly without the beta channel, in the first case feedback correctly got moved to the distro dir and installed as a profile add-on then in the second it got removed from the distro dir.
Attachment #505668 -
Attachment is obsolete: true
Attachment #505872 -
Flags: review?
Updated•14 years ago
|
Attachment #505872 -
Flags: review? → review+
Assignee | ||
Comment 28•14 years ago
|
||
Status: NEW → RESOLVED
Closed: 14 years ago
Flags: in-testsuite-
OS: Mac OS X → All
Hardware: x86 → All
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b10
Updated•14 years ago
|
Attachment #505615 -
Attachment is obsolete: true
Updated•6 years ago
|
Component: Build Config → General
Product: Firefox → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•