Closed Bug 591866 Opened 9 years ago Closed 9 years ago

channel-prefs.js should not be in omni.jar (it needs to be excluded from the update mar)

Categories

(Firefox Build System :: General, defect, blocker)

defect
Not set
blocker

Tracking

(blocking2.0 beta5+)

VERIFIED FIXED
mozilla2.0b5
Tracking Status
blocking2.0 --- beta5+

People

(Reporter: nthomas, Assigned: mwu)

References

Details

Attachments

(2 files)

Two problems
1, the update channel is always read from defaults/pref/channel-prefs.js and not the profile. Moving the file into omni.jar makes it harder for QA to do update testing (because they need to set it to 'betatest' or 'releasetest')
2, modifying omni.jar will cause partial updates to fail

mwu suggests moving channel-prefs.js back out of omni.jar. I don't think we query that all that often so hopefully the impact is tiny.
The alternative is to copy the value of app.update.url and create app.update.url.override with it, then replace %CHANNEL% with betatest or releasetest. I think this is a bit less transparent than the current technique because the channel part of the URL is usually off the right hand edge of about:config.
I think the more important problem is that this makes the update channel part of the update when it really should not be.
Summary: channel-prefs.js in omni.jar makes update testing hard → channel-prefs.js should not be in omni.jar
Yes, absolutely.
Any reason to not just change the default pref programatically? I do this for the channel in numerous mochitest-chrome and xpcshell tests.
So, this is entirely about manual testing since all mochitest-chrome, xpcshell, and mozmill tests don't modify the channel pref file?

I don't see how having the channel be part of the update is really a problem except when it concerns manual update testing where the channel needs to be changed for some reason. For clarity, under what circumstances is it necessary to change the channel?

Another option is a separate file could be used like what was done for the build's locale.
Summary: channel-prefs.js should not be in omni.jar → channel-prefs.js should not be in omni.jar (makes manual testing more difficult)
Suppose you're on the beta channel with 4.0b5. The mar file produced for 4.0RC1 has omni.jar containing a channel-prefs.js setting 'release'. You are now on a different channel.
As I understand it, the update channel depends on whether the user initially installed a nightly or a release. If the user installs a nightly, the user should continue to get nightly updates even if the nightly channel puts out a release build (with the release update channel).
The update needs to be able to exclude this file from all updates... makes sense. So either channel-prefs.js needs to be outside of omni.jar or the channel could be stored in a file like the locale is for app update.

mwu, how difficult would it be to move channel-prefs.js out of omni.jar? What is your opinion of using a separate file specifically for this purpose?
Summary: channel-prefs.js should not be in omni.jar (makes manual testing more difficult) → channel-prefs.js should not be in omni.jar (it needs to be excluded from the update mar)
(In reply to comment #8)
> mwu, how difficult would it be to move channel-prefs.js out of omni.jar? What
> is your opinion of using a separate file specifically for this purpose?

It shouldn't be too hard. I'll try to put a patch together tomorrow.

Having a separate file isn't great but we can probably do some sort of lazy loading to minimize the impact.
If it weren't in a pref and instead in a separate file it would only be used by app update and hence would only be loaded when necessary. This would of course remove channel-prefs.js entirely. My main concern is that we should have a way to override the channel. To support xulrunner apps it would need to check both the gre and app dirs similar to how the locale file is checked as follows.
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/nsUpdateService.js#547
(In reply to comment #5)
> So, this is entirely about manual testing since all mochitest-chrome, xpcshell,
> and mozmill tests don't modify the channel pref file?

Mozmill modifies that file because changing the pref app.update.channel once Firefox is running doesn't have any effect. So it's a very important dependency for our tests right now. I'm really up for a better solution too.
Where does mozmill modify channel-prefs.js? I did a quick scan of the mozmill update scripts / shared module and didn't see it.
This blocks definitely our Mozmill testing. If it's not in beta5 and we don't have another easy way to switch the channel we cannot run automation.

Currently our nightly update tests cannot be run because of this issue.
Severity: normal → blocker
blocking2.0: --- → ?
Hardware: x86 → All
(In reply to comment #13)
> Where does mozmill modify channel-prefs.js? I did a quick scan of the mozmill
> update scripts / shared module and didn't see it.

It's part of the automation scripts:
http://hg.mozilla.org/qa/mozmill-automation/file/8710add372e4/libs/application.py#l97
I'm fairly certain you can modify the default preference to get the same affect with mozmill. I'll take a look at making it do this tomorrow. The only thing I am unsure of is whether your mozmill harness can pass the channel value to the test itself.

I'm tempted to just change app update so that it doesn't just check the default preference especially since we want to be able to change channels in the future. If there is a user preference for the channel that is different from the default preference we'll just download the complete mar. mozmill would need to set the default preference so it can download the partial of course.

Would this suffice?
(In reply to comment #16)
> I'm fairly certain you can modify the default preference to get the same affect
> with mozmill. I'll take a look at making it do this tomorrow. The only thing I
> am unsure of is whether your mozmill harness can pass the channel value to the
> test itself.

Yes, we can do that via module.persisted which we are already using to pass results back to the command line script.

> I'm tempted to just change app update so that it doesn't just check the default
> preference especially since we want to be able to change channels in the

Sounds good.

> future. If there is a user preference for the channel that is different from
> the default preference we'll just download the complete mar. mozmill would need
> to set the default preference so it can download the partial of course.

Why can't we have a user preference set to a specific channel and get both partial and complete updates? The update type is part of the update snippet and shouldn't cause us to have different handling in Mozmill.

How big is the chance to get this backported? Right now the automation scripts are not bind to a specific branch and we would have to add conditional checks or even put those under branch control again.
This needs to block RC so that we don't update beta users to the release channel, but I don't think it needs to block before that. Please correct me if I'm wrong.

Moving channel-prefs out of the omnijar seems like the low-risk solution here, can we just do that?
blocking2.0: ? → final+
(In reply to comment #18)
> This needs to block RC so that we don't update beta users to the release
> channel, but I don't think it needs to block before that. Please correct me if
> I'm wrong.

I just have to note that this blocks QA by running automated update tests for current nightly builds and release builds like beta5 -> beta6 if it isn't fixed for beta5. I don't have the time to investigate a workaround for Mozmill for an even temporary location. Getting this fixed as soon as possible would be highly appreciated.
(In reply to comment #18)
> This needs to block RC so that we don't update beta users to the release
> channel, but I don't think it needs to block before that. Please correct me if
> I'm wrong.
> 
> Moving channel-prefs out of the omnijar seems like the low-risk solution here,
> can we just do that?
Agreed.

mwu, please take approach. I'll look into removing the pref file after the branch for Firefox 4 in a separate bug.
Attachment #470525 - Flags: review?(benjamin)
Comment on attachment 470525 [details] [diff] [review]
Special case channel-prefs.js

You need to re-add this to package-manifest.in, right?
Attachment #470525 - Flags: review?(benjamin) → review+
(In reply to comment #22)
> Comment on attachment 470525 [details] [diff] [review]
> Special case channel-prefs.js
> 
> You need to re-add this to package-manifest.in, right?

It was never removed from package-manifest.in
Blocks b5 since if you update from a b5 with the prefs in the omnijar to b6 or anything without it you have no channel set and you'll just be on the default channel which iirc doesn't receive updates.
blocking2.0: final+ → beta5+
(In reply to comment #24)
> Blocks b5 since if you update from a b5 with the prefs in the omnijar to b6 or
> anything without it you have no channel set and you'll just be on the default
> channel which iirc doesn't receive updates.

Ah crap. That means this breaks updates our current nightly users, doesn't it?
(In reply to comment #25)
> (In reply to comment #24)
> > Blocks b5 since if you update from a b5 with the prefs in the omnijar to b6 or
> > anything without it you have no channel set and you'll just be on the default
> > channel which iirc doesn't receive updates.
> 
> Ah crap. That means this breaks updates our current nightly users, doesn't it?

For nightly users who installed fresh since omnijar, yes. For those from before I imagine they still have the channel-prefs laying around from before. Exciting
It breaks them... channel-prefs.js was removed via removed-files.
(In reply to comment #26)
> For nightly users who installed fresh since omnijar, yes. For those from before
> I imagine they still have the channel-prefs laying around from before. Exciting

Unfortunately channel-prefs.js was in removed-files.in.

The only solution to avoid breaking nightly users I can think of is to change tools/update-packaging/common.sh to add channel-prefs.js back to updates for a few days and then back it out.
nthomas, could you not exclude channel-prefs.js in the next nightly mar? From there the choices are to either continue not excluding it for a while or alway point to this nightly mar that contains the channel-prefs.js for all builds prior to this nightly.
specifically, nightly with a Firefox version of 4.0b5pre
I suggest having this patch in the tree for a few days to put channel-prefs.js back, but not in the branch for beta5.
Attachment #470560 - Flags: review?(robert.bugzilla)
Comment on attachment 470560 [details] [diff] [review]
Add channel-prefs.js back to the update temporarily

I'm fine with this approach but this needs to be reviewed by nthomas.
Attachment #470560 - Flags: review?(robert.bugzilla)
Attachment #470560 - Flags: review?(nrthomas)
Attachment #470560 - Flags: feedback+
Comment on attachment 470560 [details] [diff] [review]
Add channel-prefs.js back to the update temporarily

r+ for this to be in nightlies for a few days. Is it going to land after branching 4.0b5 ?
Attachment #470560 - Flags: review?(nrthomas) → review+
(In reply to comment #33)
> Comment on attachment 470560 [details] [diff] [review]
> Add channel-prefs.js back to the update temporarily
> 
> r+ for this to be in nightlies for a few days. Is it going to land after
> branching 4.0b5 ?
We can do that if beta5 branches soon/today. If not, we can back it out on the branch.
Wait - this confuses me. This bug blocks beta5, apparently, but we *don't* want the patch in the beta5 relbranch?
(In reply to comment #35)
> Wait - this confuses me. This bug blocks beta5, apparently, but we *don't* want
> the patch in the beta5 relbranch?

We want the first patch but not the second. The second patch is to restore channel-prefs.js for our nightly users.
OK, please land the first patch on trunk, then file a separate bug for the second patch. This bug is a blocker; only patches that fix that blocker are welcome.
http://hg.mozilla.org/mozilla-central/rev/66e15c60b178

Will open a second bug for the second patch.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Blocks: 592127
We really really cannot allow a nightly to happen without the second patch, or we'll strand all our nightly testers.
(In reply to comment #40)
> We really really cannot allow a nightly to happen without the second patch, or
> we'll strand all our nightly testers.

I opened bug 592127. I want to land after branching to avoid having to back out on the release branch, but it should get landed either way before tonight.
Thanks! The channel-prefs.js is present on all systems again and Mozmill tests can detect and modify it.
Status: RESOLVED → VERIFIED
Target Milestone: --- → mozilla2.0b5
At least one nightly user (me) appears to have been stranded by this; see bug 612600.
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.