add global "RELEASE_BUILD" #define that depends on the build version number

RESOLVED FIXED in Firefox 19

Status

defect
RESOLVED FIXED
7 years ago
a year ago

People

(Reporter: Gavin, Assigned: heycam)

Tracking

unspecified
mozilla20
Dependency tree / graph

Firefox Tracking Flags

(firefox19 fixed, firefox20 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

The use case is wanting to enable features on Nightly/Aurora, but not Beta/Release. The current hacks for this involve checking update channels, which is not optimal because it doesn't really work for people building locally without update channels defined.

(see bug 803276 comment 4)

Instead, I propose basing the #define on whether the version number in version.txt includes an "a", since that is updated on merge day and is more reliable across build configs. I had originally proposed using the name of the current repository, but I think that wouldn't work since people can name their repositories whatever they link, and "remotes" can similarly be arbitrary (for people cloning m-c locally to "m-c", for example).
Assignee

Updated

7 years ago
Blocks: 814530
The only wrinkle here is winding up with code that doesn't actually compile #ifdef RELEASE_BUILD, and not finding out until it hits Beta.

Is there a particular reason we'd want to do this automatically, and not just land patches to disable individual features that we think aren't release-ready?
Assignee

Comment 2

7 years ago
At best, somebody remembers to do this and flips the pref just before or after the Aurora -> Beta uplift, resulting in either the feature incorrectly being disabled for a day (on Aurora) or incorrectly enabled for a day (on Beta).  Last uplift, I forgot, and so @supports was enabled for a good week on Beta.

I would be OK with release people doing this pref flipping as part of the uplift, but I feel like something automated would be better.

We could have this macro only be defined for the all.js preprocessing, sticking it in PREF_PPFLAGS for example.
FWIW, we have the same problem with --enable-official-branding, and iirc dbaron builds with it to make sure we don't break it.
Are you aware we already have separate mozconfigs for some configurations, including the nightly and release channels? We could add mozconfigs for the beta and aurora channels (assuming they don't exist already) and feature selection could all be controlled through configure arguments. You would only need to adjust the mozconfigs when the channel-dependent feature is added to the tree, not when it rides the trains.

A benefit of this is that features could be selectively toggled independent of the channel. I worry that #defines too tightly couple the release process with feature activation. i.e. I'd rather have multiple individual knobs that have direct control over individual feature behavior than one giant knob (version/channel type) that doesn't offer granular control.
Justin tells me RelEng doesn't like multiple mozconfigs.

One thing worth considering is exposing channel and/or version numbers to mozconfigs. mozconfigs are just shell scripts after all.
(In reply to Gregory Szorc [:gps] from comment #5)
> Justin tells me RelEng doesn't like multiple mozconfigs.

To be extra clear, I meant that as a "*I*" don't like it, speaking as a releng person. Not meant to blanket-statement that all of Releng share my opinion (but I would not be surprised if others in releng did)
I think from my point of view the following hold:

1)  Doing a build on aurora or beta using a default mozconfig (or whatever mozconfig a developer is using) should get you something that's as similar as possible to official aurora/beta builds.

2)  Trusting a dozen people to all independently remember to pref off several things manually when going to beta is an exercise in fail, so we need some sort of centralized solution where a developer who lands a "not ready to ship to the web" feature would just flag it that way _once_ and we wouldn't ship it until it got unflagged.

Doing the last paragraph of comment 2 seems like a good compromise to me if people are really worried about build issues if the macro is available everywhere....

One question I have: who makes the call on this and when?  If this is not happening before the next uplift, we need to plan accordingly....
I'm fine with this plan, I just wanted to make sure we had thought about the potential for issues. Sounds like it's likely to be the least-bad option. We should define exactly what we want it to be able to control. #ifdef'ing code or default preferences should be fine. Trying to use this to control defaults for --enable flags in configure is trickier just because of the ordering problem (need to know what app you're building to get the version number).
Yeah, I think in practice we're not doing --enable flags in configure much to control web-facing features because it's a pain to add that sort of thing.  So controlling prefs and code #ifdefs is sufficient for most of the things I know about that we want to turn off.

If we sort of agree on the plan... who's doing it?
Assignee

Comment 10

7 years ago
I'll take it.  I assume we only want to disable the feature prefs if we're building Firefox, and in all other cases we want the features to be enabled?
Assignee: nobody → cam
Status: NEW → ASSIGNED
Assignee

Comment 11

7 years ago
I think the macro name should talk about feature disabling rather than whether we're in a release build; the former sounds more accurate if we take into account non-browser MOZ_BUILD_APP values.

#ifdef UNSTABLE_FEATURES_DISABLED
pref(..., false);
#else
pref(..., true);
#endif

where UNSTABLE_FEATURES_DISABLED is defined if MOZ_BUILD_APP == "browser" and FIREFOX_VERSION =~ /a/?  (Not sure about "UNSTABLE" though.)
Not sure what the Seamonkey folks want....

Do we want to enable stuff in b2g?
Actually, my preference is that all.js should just have the Firefox behavior.  Other apps can override prefs as needed.
Assignee

Comment 14

7 years ago
OK.  In that case the name RELEASE_BUILD sounds all right to me.
Assignee

Comment 15

7 years ago
Posted patch patch (obsolete) — Splinter Review
Set macro RELEASE_BUILD while preprocessing pref js files when browser/config/version.txt does not contain "a".
Attachment #691121 - Flags: review?(ted)
(In reply to Boris Zbarsky (:bz) from comment #13)
> Actually, my preference is that all.js should just have the Firefox
> behavior.  Other apps can override prefs as needed.

I really don't think this is a good idea. With something like that you can be sure most apps based on Gecko will have these features enabled when they really shouldn't. That should be the exact opposite.
Hmm.  I was assuming we'd just base this on Gecko version, so apps built on a release Gecko would not have the feature enabled....
Assignee

Comment 18

7 years ago
For SeaMonkey for example we want to have the same pref settings as Firefox.  But say Thunderbird, it probably don't matter.  Shouldn't it be safer to keep the immature features disabled in other applications using Gecko unless they opt in to them?
> Shouldn't it be safer to keep the immature features disabled in other applications using
> Gecko unless they opt in to them?

I would be fine with that too, yes.
Assignee

Comment 20

7 years ago
(To be clear, I thought I was just restating your comment 13 and comment 17, which should be what the patch currently does.)
Attachment #691121 - Flags: review?(ted) → review+
Comment on attachment 691121 [details] [diff] [review]
patch

>diff --git a/config/rules.mk b/config/rules.mk
>--- a/config/rules.mk
>+++ b/config/rules.mk

>+ifeq (,$(findstring a,$(FIREFOX_VERSION)))

Nit: wouldn't  MOZ_APP_VERSION be more appropriate here, since its used outside of browser/ and is not specific to Firefox.
(In reply to Boris Zbarsky (:bz) from comment #12)
> Not sure what the Seamonkey folks want....

For sure we want the same web-features as Firefox on a by-default basis. So if web-facing features are disabled by default in a certain build of Firefox, SeaMonkey wants to mirror that [ideally without needing to override anything in comm-* directly]

(In reply to Cameron McCormack (:heycam) from comment #20)
> (To be clear, I thought I was just restating your comment 13 and comment 17,
> which should be what the patch currently does.)

And for clarity, indeed the patch does this the way I would like.
Assignee

Comment 23

7 years ago
(In reply to Justin Wood (:Callek) from comment #21)
> >+ifeq (,$(findstring a,$(FIREFOX_VERSION)))
> 
> Nit: wouldn't  MOZ_APP_VERSION be more appropriate here, since its used
> outside of browser/ and is not specific to Firefox.

Will they always be the same (maybe in practice they are)?  I guessed that it would be slightly safer choosing FIREFOX_VERSION, since I can see in configure.in that it grabs it from browser/config/version.txt, whereas MOZ_APP_VERSION could be set to anything by an application's confvars.sh.
Comment on attachment 691121 [details] [diff] [review]
patch

My intent was for the define to be global, and not limited to prefs files, so that it could be used elsewhere as well (e.g. in code). Any reason not to do that?

You could use GRE_MILESTONE (the value from milestone.txt) rather than FIREFOX_VERSION to make this suitably app-agnostic. I guess that'd involve moving the definition of GRE_MILESTONE to configure.
(In reply to Cameron McCormack (:heycam) from comment #23)
> (In reply to Justin Wood (:Callek) from comment #21)
> > >+ifeq (,$(findstring a,$(FIREFOX_VERSION)))
> > 
> > Nit: wouldn't  MOZ_APP_VERSION be more appropriate here, since its used
> > outside of browser/ and is not specific to Firefox.
> 
> Will they always be the same (maybe in practice they are)?  I guessed that
> it would be slightly safer choosing FIREFOX_VERSION, since I can see in
> configure.in that it grabs it from browser/config/version.txt, whereas
> MOZ_APP_VERSION could be set to anything by an application's confvars.sh.

Point was that also in-theory an [non Firefox] app may want to cut a real beta, or a final off a non-beta-branch, and if they set MOZ_APP_VERSION appropriately they'd get the same behavior we *intend* with this patch.

And in practice for Firefox/SeaMonkey it would end up acting the same as you do here, but withing directly tying it to FIREFOX*.

its even a better choice than GRE_MILESTONE - I think. Though, imho, this whole line of comments is just us bikeshedding and - while a valid desire to have the shed look nice, (rather than hand a paint brush to a 3 year old) what I care about is that the shed is built, not that it is pretty. So I'll defer to you/gavin on final choice here.
Blocks: 818400
Assignee

Comment 26

7 years ago
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #24)
> My intent was for the define to be global, and not limited to prefs files,
> so that it could be used elsewhere as well (e.g. in code). Any reason not to
> do that?

The direct problem we were having was with pref flipping.  Now I guess we might have some feature that is not easily controllable via a pref and so instead you would want to put it behind a macro.  I think the risk of things breaking during the uplift process is lower if it's just used to control prefs.  At least we'll avoid compilation errors.

Ted (given your wrinkle in comment 1), WDYT?
Assignee

Comment 27

7 years ago
(In reply to Justin Wood (:Callek) from comment #25)
> Point was that also in-theory an [non Firefox] app may want to cut a real
> beta, or a final off a non-beta-branch, and if they set MOZ_APP_VERSION
> appropriately they'd get the same behavior we *intend* with this patch.

Not to belabour the point (well, maybe a bit):

But would this be say cutting a release of the app from e.g. an Aurora Gecko branch?  Because I think there we don't want the features to be enabled.  Although TBH SeaMonkey is probably the only real app where we definitely want to avoid these features becoming enabled before they're ready.  If someone is making a non-browser app then it doesn't matter as much.

So if GRE_MILESTONE means "Gecko version", then that seems to me to be a better fit.  (I assume that FIREFOX_VERSION is always going to be the same as GRE_MILESTONE.)
(In reply to Cameron McCormack (:heycam) from comment #27)
> So if GRE_MILESTONE means "Gecko version", then that seems to me to be a
> better fit.  (I assume that FIREFOX_VERSION is always going to be the same
> as GRE_MILESTONE.)

Yes, let's use GRE_MILESTONE (this is intended for Gecko features, not app features). In practice FIREFOX_VERSION will always be the same as GRE_MILESTONE.
Status: ASSIGNED → NEW
Status: NEW → ASSIGNED
Assignee

Comment 29

7 years ago
See question at end of comment 26.
Flags: needinfo?(ted)
I'm inclined to say "stick with just a prefs.js #define for now". I'd rather have someone from release management make the call on whether they're ok with code changes taking effect via migration. If they don't have a problem with that then I don't either.
Flags: needinfo?(ted)
Assignee

Comment 31

7 years ago
OK, let's begin with just allowing in prefs.

https://hg.mozilla.org/integration/mozilla-inbound/rev/7286dac15291
Assignee

Comment 32

7 years ago
Comment on attachment 691121 [details] [diff] [review]
patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): N/A
User impact if declined: None
Testing completed (on m-c, etc.): Successful build on inbound
Risk to taking this patch (and alternatives if risky): Very low
String or UUID changes made by this patch: N/A

If we can land this on Aurora, then people with feature prefs to disable for release will only need to make that change one last time on Aurora before the next uplift.
Attachment #691121 - Flags: approval-mozilla-aurora?
Assignee

Updated

7 years ago
Whiteboard: [leave open]
Comment on attachment 691121 [details] [diff] [review]
patch

Can you throw up a post to dev.planning/dev.platform (or anywhere you see fit) so that other developers are aware of this functionality? Thanks for doing this!
Attachment #691121 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee

Comment 35

7 years ago
https://hg.mozilla.org/releases/mozilla-beta/rev/a0dbfb9d776b
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Whiteboard: [leave open]
Target Milestone: --- → mozilla20
Assignee

Comment 37

7 years ago
Backed out because the patch I landed didn't work:

https://hg.mozilla.org/integration/mozilla-inbound/rev/01f69299eba0

I moved my rules into build/Makefile.in, since that's where GRE_MILESTONE was available (it wasn't available in rules.mk by the time it was parsed), but of course that won't have any effect on other directories.

Per gavin's suggestion I'll set GRE_MILESTONE in configure.in so it's available earlier.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee

Comment 39

7 years ago
Posted patch patch v2Splinter Review
Set GRE_MILESTONE in configure.  Tested locally on an inbound and beta branch, seems to do the right thing.
Attachment #691121 - Attachment is obsolete: true
Attachment #694631 - Flags: review?(gavin.sharp)
Comment on attachment 694631 [details] [diff] [review]
patch v2

Looks good to me, but I'm not the best person to review changes to configure.
Attachment #694631 - Flags: review?(gavin.sharp) → review?(ted)
Attachment #694631 - Flags: review?(ted) → review+
Assignee

Comment 42

7 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/d3a280f6c049
Status: REOPENED → RESOLVED
Last Resolved: 7 years ago7 years ago
Resolution: --- → FIXED
Blocks: 934360

Updated

a year ago
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.