Closed Bug 817723 Opened 7 years ago Closed 7 years ago

Only build the stub installer when building x86 and the update channel equals nightly, aurora, beta, and release

Categories

(Firefox :: Installer, defect)

x86_64
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 20
Tracking Status
firefox19 --- fixed
firefox20 --- fixed

People

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

References

Details

Attachments

(3 files, 4 obsolete files)

There have been a few complaints about the stub installer only downloading the nightly channel vs. nightly-elm, etc. and since it would be a pita to maintain entries in bouncer for all of the repos we should just enable building the stub installer when the update channel equals nightly on mozilla-central. We might want to do something similar for aurora, beta, and release but that isn't as much of an issue so this bug is only about m-c.
Note: for try builds we should be able to check in a change to confvars.sh to enable building the stub installer on m-c.
Comment on attachment 687873 [details] [diff] [review]
patch rev1 - untested

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

Here's a snippet from channel checking we use in updater/Makefile.in for determining which cert to include inside the .rc file.

ifneq (,$(filter beta release esr,$(MOZ_UPDATE_CHANNEL)))
RCFLAGS += -DMAR_SIGNING_RELEASE_BETA=1
else
ifneq (,$(filter nightly aurora nightly-elm nightly-profiling nightly-oak,$(MOZ_UPDATE_CHANNEL)))
RCFLAGS += -DMAR_SIGNING_AURORA_NIGHTLY=1
endif
endif
(In reply to Brian R. Bondy [:bbondy] from comment #3)
> Comment on attachment 687873 [details] [diff] [review]
> patch rev1 - untested
> 
> Review of attachment 687873 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Here's a snippet from channel checking we use in updater/Makefile.in for
> determining which cert to include inside the .rc file.
> 
> ifneq (,$(filter beta release esr,$(MOZ_UPDATE_CHANNEL)))
> RCFLAGS += -DMAR_SIGNING_RELEASE_BETA=1
> else
> ifneq (,$(filter nightly aurora nightly-elm nightly-profiling
> nightly-oak,$(MOZ_UPDATE_CHANNEL)))
> RCFLAGS += -DMAR_SIGNING_AURORA_NIGHTLY=1
> endif
> endif
Since confvars.sh doesn't get merged I was just going to go with the simple test on m-c. Is there any reason to go with the more inclusive test?
that's fine, was just including it in case it was helpful.
Attached patch patch rev2 (obsolete) — Splinter Review
Successful push to try and verified that the stub doesn't build on both win 32 and 64
https://tbpl.mozilla.org/?tree=Try&rev=a628da3c719d&noignore=1

Verifying locally that everything builds fine with the update channel set to nightly before requesting review.
Attachment #687873 - Attachment is obsolete: true
Comment on attachment 688136 [details] [diff] [review]
patch rev2

Brian, could you sanity check the confvars.sh changes.

btw: I had to move MOZ_UPDATE_CHANNEL prior to confvars.sh in configure.in and I went with moving it to the existing location in js/src/configure.in
Attachment #688136 - Flags: feedback?(netzen)
catlee, juanb, jsmith: just a heads up about this upcoming change. If / when it lands, please let anyone else know that should know.
Attached patch patch rev3 (obsolete) — Splinter Review
Attachment #688136 - Attachment is obsolete: true
Attachment #688136 - Flags: feedback?(netzen)
Attachment #688157 - Flags: review?(khuey)
Attachment #688157 - Flags: feedback?(netzen)
Comment on attachment 688157 [details] [diff] [review]
patch rev3

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

Looks good to me as long as there are no objections from releng and as long as it passes try tests.
Attachment #688157 - Flags: feedback?(netzen) → feedback+
Please also include aurora, beta, release and esr117 here so that releng doesn't have to build stubs for these channels manually. Or perhaps building when official branding is enabled is sufficient (although that misses aurora).
(In reply to Brian R. Bondy [:bbondy] from comment #10)
> Comment on attachment 688157 [details] [diff] [review]
> patch rev3
> 
> Review of attachment 688157 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good to me as long as there are no objections from releng and as long
> as it passes try tests.
It passes the l10n tests... any other tests you think it should pass?
https://tbpl.mozilla.org/?tree=Try&rev=a628da3c719d&noignore=1
(In reply to Chris AtLee [:catlee] from comment #11)
> Please also include aurora, beta, release and esr117 here so that releng
> doesn't have to build stubs for these channels manually. Or perhaps building
> when official branding is enabled is sufficient (although that misses
> aurora).
catlee, this is in confvars.sh on m-c which doesn't build aurora, beta, release or esr117 without manual intervention. To build any of these channel's stub installer on m-c the nightly stub installer will first need to be built. Then branding files and channel name will need to be updated in the nightly directory as has been done previously to build these channels.
(In reply to Robert Strong [:rstrong] (do not email) from comment #13)
> (In reply to Chris AtLee [:catlee] from comment #11)
> > Please also include aurora, beta, release and esr117 here so that releng
> > doesn't have to build stubs for these channels manually. Or perhaps building
> > when official branding is enabled is sufficient (although that misses
> > aurora).
> catlee, this is in confvars.sh on m-c which doesn't build aurora, beta,
> release or esr117 without manual intervention. To build any of these
> channel's stub installer on m-c the nightly stub installer will first need
> to be built. Then branding files and channel name will need to be updated in
> the nightly directory as has been done previously to build these channels.

As this code rides the trains, it's important that the stub is getting built properly by automation. If you want to update the merge day checklist to update this file on every uplift, that would be fine by me. We should not be planning on doing manual builds for beta/release stub installers in the long term.
(In reply to Robert Strong [:rstrong] (do not email) from comment #12)
> (In reply to Brian R. Bondy [:bbondy] from comment #10)
> > Comment on attachment 688157 [details] [diff] [review]
> > patch rev3
> > 
> > Review of attachment 688157 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Looks good to me as long as there are no objections from releng and as long
> > as it passes try tests.
> It passes the l10n tests... any other tests you think it should pass?
> https://tbpl.mozilla.org/?tree=Try&rev=a628da3c719d&noignore=1

Nope.  I was more concerned with something on releng side that would maybe look for the stub exe to sign it and then fail.  But maybe that code just looks for any exe and signs it, or maybe this is already covered by the confar variable. I don't think tbpl builds get signed anyway.  If you want to try it out though you can push to oak and do a nightly build there (via https://secure.pub.build.mozilla.org/buildapi/self-serve/oak ). It is a lvl3 repo so other exes would be signed there.  You'd know better than me though if that's not needed.
(In reply to Chris AtLee [:catlee] from comment #14)
> (In reply to Robert Strong [:rstrong] (do not email) from comment #13)
> > (In reply to Chris AtLee [:catlee] from comment #11)
> > > Please also include aurora, beta, release and esr117 here so that releng
> > > doesn't have to build stubs for these channels manually. Or perhaps building
> > > when official branding is enabled is sufficient (although that misses
> > > aurora).
> > catlee, this is in confvars.sh on m-c which doesn't build aurora, beta,
> > release or esr117 without manual intervention. To build any of these
> > channel's stub installer on m-c the nightly stub installer will first need
> > to be built. Then branding files and channel name will need to be updated in
> > the nightly directory as has been done previously to build these channels.
> 
> As this code rides the trains, it's important that the stub is getting built
> properly by automation. If you want to update the merge day checklist to
> update this file on every uplift, that would be fine by me. We should not be
> planning on doing manual builds for beta/release stub installers in the long
> term.
Unless I am badly mistaken, confvars.sh doesn't ride the trains (confvars.sh had to be updated manually in aurora, beta, and release and this is how we make ACCEPTED_MAR_CHANNEL_IDS and MAR_CHANNEL_ID train repo specifc... also, see comment #0) and if this is actually the case all other train repos will continue to build the stub installer unconditionally with this patch.
(In reply to comment #16)
> (In reply to Chris AtLee [:catlee] from comment #14)
> > (In reply to Robert Strong [:rstrong] (do not email) from comment #13)
> > > (In reply to Chris AtLee [:catlee] from comment #11)
> > > > Please also include aurora, beta, release and esr117 here so that releng
> > > > doesn't have to build stubs for these channels manually. Or perhaps building
> > > > when official branding is enabled is sufficient (although that misses
> > > > aurora).
> > > catlee, this is in confvars.sh on m-c which doesn't build aurora, beta,
> > > release or esr117 without manual intervention. To build any of these
> > > channel's stub installer on m-c the nightly stub installer will first need
> > > to be built. Then branding files and channel name will need to be updated in
> > > the nightly directory as has been done previously to build these channels.
> > 
> > As this code rides the trains, it's important that the stub is getting built
> > properly by automation. If you want to update the merge day checklist to
> > update this file on every uplift, that would be fine by me. We should not be
> > planning on doing manual builds for beta/release stub installers in the long
> > term.
> Unless I am badly mistaken, confvars.sh doesn't ride the trains.

AFAIK, it does.
> > As this code rides the trains, it's important that the stub is getting built
> > properly by automation. If you want to update the merge day checklist to
> > update this file on every uplift, that would be fine by me. We should not be
> > planning on doing manual builds for beta/release stub installers in the long
> > term.
> Unless I am badly mistaken, confvars.sh doesn't ride the trains (confvars.sh
> had to be updated manually in aurora, beta, and release and this is how we
> make ACCEPTED_MAR_CHANNEL_IDS and MAR_CHANNEL_ID train repo specifc... also,
> see comment #0) and if this is actually the case all other train repos will
> continue to build the stub installer unconditionally with this patch.

It does ride the trains, but is modified after each uplift. So if you want to add another step to the merge checklist to update this block of code, that's fine with releng as I indicated previously, and you should make sure relman is aware and OK with this change.
Thanks for the info and sounds like a pita. ;)

I think I'll go with just adding them but what about release-test and friends?
Attachment #688157 - Flags: review?(khuey)
Attached patch patch rev4 (obsolete) — Splinter Review
catlee, will this suffice?
Attachment #688157 - Attachment is obsolete: true
Attachment #688399 - Flags: feedback?(catlee)
Attachment #688399 - Flags: review?(khuey)
Summary: Only build the stub installer when the update channel equals nightly → Only build the stub installer when the update channel equals nightly, aurora, beta, and release
Comment on attachment 688399 [details] [diff] [review]
patch rev4

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

Looks good to me.

Builds are never built with "X-test" as the channel, so beta-test/release-test aren't a concern.

We'll need to update this file if we ever want to have stub installer support for ESR.
Attachment #688399 - Flags: feedback?(catlee) → feedback+
Summary: Only build the stub installer when the update channel equals nightly, aurora, beta, and release → Only build the stub installer when building x86 and the update channel equals nightly, aurora, beta, and release
btw: this likely just needs a clobber but I want to verify that is the case first.
Carrying forward r+
Attachment #688399 - Attachment is obsolete: true
Attachment #690267 - Flags: review+
Comment on attachment 690268 [details] [diff] [review]
patch rev5 - configure changes

Kyle, the move of MOZ_UPDATER broke the build so I split out the configure.in changes into a separate patch and just moved MOZ_UPDATE_CHANNEL to above where confvars.sh is called. The try runs look good. Thanks!
https://tbpl.mozilla.org/?tree=Try&rev=b2186845498b
Attachment #690268 - Flags: review?(khuey)
https://hg.mozilla.org/mozilla-central/rev/4528be937f5c
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 20
Comment on attachment 690624 [details] [diff] [review]
combined patch as checked in

Approving for Aurora, as this blocks the wonderful merge help found in bug 818333. The risk here is very manageable, given the testing that occurs over the aurora/beta merge.
Attachment #690624 - Flags: approval-mozilla-aurora+
Backed out but this one is safe to land on aurora
You need to log in before you can comment on or make changes to this bug.