Closed Bug 941291 Opened 6 years ago Closed 6 years ago

Simplify creating funnelcake stub installers

Categories

(Firefox :: Installer, defect)

x86_64
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 28
Tracking Status
firefox25 --- wontfix
firefox26 --- wontfix
firefox27 --- fixed
firefox28 --- fixed

People

(Reporter: robert.strong.bugs, Assigned: robert.strong.bugs)

Details

(Whiteboard: [qa-])

Attachments

(2 files, 4 obsolete files)

Attached patch patch rev1 (obsolete) — Splinter Review
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.
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
Closed: 6 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+
Attached patch followup patch rev1 (obsolete) — Splinter Review
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)
Attached patch followup patch rev3 (obsolete) — Splinter Review
*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+
(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+
https://hg.mozilla.org/mozilla-central/rev/4e9f43beb31f
https://hg.mozilla.org/mozilla-central/rev/1180c417baff
Status: REOPENED → RESOLVED
Closed: 6 years ago6 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?
Attachment #8336428 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.