Simplify creating funnelcake stub installers

RESOLVED FIXED in Firefox 27

Status

()

Firefox
Installer
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: rstrong, Assigned: rstrong)

Tracking

unspecified
Firefox 28
x86_64
Windows 7
Points:
---

Firefox Tracking Flags

(firefox25 wontfix, firefox26 wontfix, firefox27 fixed, firefox28 fixed)

Details

(Whiteboard: [qa-])

Attachments

(2 attachments, 4 obsolete attachments)

Patch coming up
Created attachment 8335624 [details] [diff] [review]
patch rev1

Nick, this makes it so only one change is necessary to create funnelcake stub installers.
Assignee: nobody → robert.bugzilla
Status: NEW → ASSIGNED
Attachment #8335624 - Flags: review?(netzen)
Attachment #8335624 - Flags: feedback?(nthomas)
Comment on attachment 8335624 [details] [diff] [review]
patch rev1

Review of attachment 8335624 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me from a code sanity standpoint.
Attachment #8335624 - Flags: review?(netzen) → review+
I've tested it locally as well.
Created attachment 8335735 [details] [diff] [review]
patch rev2 - removed redundancy in comment - checked in

Carrying forward r+
Attachment #8335624 - Attachment is obsolete: true
Attachment #8335624 - Flags: feedback?(nthomas)
Attachment #8335735 - Flags: review+
Attachment #8335735 - Flags: feedback?(nthomas)
Pushed to fx-team
https://hg.mozilla.org/integration/fx-team/rev/f37f1fcd9823

Nick, this is mainly a heads up for you that there is a simpler method for creating stub installers for funnelcake. Thanks!
https://hg.mozilla.org/mozilla-central/rev/f37f1fcd9823
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 28
Great idea, but I think this doesn't work quite as you intended. In the normal flow, the full installer is requested with product=firefox-latest-f28, rather than product=firefox-latest.....&f=NN.
Thanks Nick, I missed that was changed as well. I'll add that as well.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #8335735 - Attachment description: patch rev2 - removed redundancy in comment → patch rev2 - removed redundancy in comment - checked in
Attachment #8335735 - Flags: feedback?(nthomas) → checkin+
Created attachment 8336353 [details] [diff] [review]
followup patch rev1

I tested this using 28 for the funnelcake experiment being run at this time and all was well as well as verified it with wireshark.

/?os=win&lang=en-US&product=firefox-latest-f28&f=28
Attachment #8336353 - Flags: review?(netzen)
Attachment #8336353 - Flags: feedback?(nthomas)
Comment on attachment 8336353 [details] [diff] [review]
followup patch rev1

meh, forgot to update the comments
Attachment #8336353 - Attachment is obsolete: true
Attachment #8336353 - Flags: review?(netzen)
Attachment #8336353 - Flags: feedback?(nthomas)
Created attachment 8336359 [details] [diff] [review]
followup patch rev2 - updated comment
Attachment #8336359 - Flags: review?(netzen)
Attachment #8336359 - Flags: feedback?(nthomas)
Created attachment 8336361 [details] [diff] [review]
followup patch rev3

*rushing too much*
Attachment #8336359 - Attachment is obsolete: true
Attachment #8336359 - Flags: review?(netzen)
Attachment #8336359 - Flags: feedback?(nthomas)
Attachment #8336361 - Flags: review?(netzen)
Attachment #8336361 - Flags: feedback?(nthomas)
Comment on attachment 8336361 [details] [diff] [review]
followup patch rev3

>diff --git a/browser/installer/windows/nsis/defines.nsi.in b/browser/installer/windows/nsis/defines.nsi.in
>+!define URLStubDownloadAppend    "-f${FunnelcakeVersion}&f=${FunnelcakeVersion}"

What is the '&f=${FunnelcakeVersion}' part for ? AFAIK download.m.o doesn't respond to it all, and it's not used for metrics. Otherwise looks good.
Attachment #8336361 - Flags: feedback?(nthomas) → feedback+
Created attachment 8336428 [details] [diff] [review]
followup patch rev4 checked in

(In reply to Nick Thomas [:nthomas] from comment #13)
> Comment on attachment 8336361 [details] [diff] [review]
> followup patch rev3
> 
> >diff --git a/browser/installer/windows/nsis/defines.nsi.in b/browser/installer/windows/nsis/defines.nsi.in
> >+!define URLStubDownloadAppend    "-f${FunnelcakeVersion}&f=${FunnelcakeVersion}"
> 
> What is the '&f=${FunnelcakeVersion}' part for ? AFAIK download.m.o doesn't
> respond to it all, and it's not used for metrics. Otherwise looks good.
I thought I saw that in the patches in bug 933847. :(

Brian, I know the use of URLStubDownloadAppend and StubURLVersionAppend could be combined into a single define but I'd like to keep them separate since it is more flexible that way.
Attachment #8336361 - Attachment is obsolete: true
Attachment #8336361 - Flags: review?(netzen)
Attachment #8336428 - Flags: review?(netzen)
Attachment #8336428 - Flags: review?(netzen) → review+
Followup patch pushed to fx-team
https://hg.mozilla.org/integration/fx-team/rev/4e9f43beb31f
and also fixed a copy paste error in a comment
https://hg.mozilla.org/integration/fx-team/rev/1180c417baff
https://hg.mozilla.org/mozilla-central/rev/4e9f43beb31f
https://hg.mozilla.org/mozilla-central/rev/1180c417baff
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED
Attachment #8336428 - Attachment description: followup patch rev4 → followup patch rev4 checked in
Attachment #8336428 - Flags: checkin+
Comment on attachment 8336428 [details] [diff] [review]
followup patch rev4 checked in

[Approval Request Comment]
Bug caused by (feature/regressing bug #): None
User impact if declined: None for users though there is impact. It has been stated that the stub installer is decoupled from the Firefox train model as justification to fast track changes. As I see it this also makes it just as ok to uplift this patch the stub installer. It is also changed manually to perform studies directly on the release channel which is error prone. This simplifies the changes we make on release for funnelcake studies and thereby lessens the likelihood of the changes breaking the stub installer while also making all of the changes needed with one change so that none of the changes are overlooked as happened with the latest study.
Testing completed (on m-c, etc.): Manually tested locally
Risk to taking this patch (and alternatives if risky): Minimal
String or IDL/UUID changes made by this patch: None
Attachment #8336428 - Flags: approval-mozilla-aurora?

Updated

4 years ago
Attachment #8336428 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Pushed to mozilla-aurora
https://hg.mozilla.org/releases/mozilla-aurora/rev/45db33cf269b
status-firefox25: --- → wontfix
status-firefox26: --- → wontfix
status-firefox27: --- → fixed
status-firefox28: --- → fixed
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.