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

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[:Gavin Sharp [email: [e-mail]]]

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).

Comment 1

[(not currently active) Ted Mielczarek]

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?

Comment 2

[Cameron McCormack (:heycam)]

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.

Comment 3

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

FWIW, we have the same problem with --enable-official-branding, and iirc dbaron builds with it to make sure we don't break it.

Comment 4

[Gregory Szorc [:gps]]

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.

Comment 5

[Gregory Szorc [:gps]]

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.

Comment 6

[Justin Wood (:Callek)]

(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)

Comment 7

[Boris Zbarsky [:bzbarsky]]

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....

Comment 8

[(not currently active) Ted Mielczarek]

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).

Comment 9

[Boris Zbarsky [:bzbarsky]]

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?

Comment 10

[Cameron McCormack (:heycam)]

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?

Comment 11

[Cameron McCormack (:heycam)]

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.)

Comment 12

[Boris Zbarsky [:bzbarsky]]

Not sure what the Seamonkey folks want....

Do we want to enable stuff in b2g?

Comment 13

[Boris Zbarsky [:bzbarsky]]

Actually, my preference is that all.js should just have the Firefox behavior.  Other apps can override prefs as needed.

Comment 14

[Cameron McCormack (:heycam)]

OK.  In that case the name RELEASE_BUILD sounds all right to me.

Comment 15

[Cameron McCormack (:heycam)]

Attachment: patch

Set macro RELEASE_BUILD while preprocessing pref js files when browser/config/version.txt does not contain "a".

Comment 16

[Mike Hommey [:glandium]]

(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.

Comment 17

[Boris Zbarsky [:bzbarsky]]

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....

Comment 18

[Cameron McCormack (:heycam)]

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?

Comment 19

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 20

[Cameron McCormack (:heycam)]

(To be clear, I thought I was just restating your comment 13 and comment 17, which should be what the patch currently does.)

Comment 21

[Justin Wood (:Callek)]

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.

Comment 22

[Justin Wood (:Callek)]

(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.

Comment 23

[Cameron McCormack (:heycam)]

(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 24

[:Gavin Sharp [email: [e-mail]]]

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.

Comment 25

[Justin Wood (:Callek)]

(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.

Comment 26

[Cameron McCormack (:heycam)]

(In reply to :Gavin Sharp (use [e-mail] 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?

Comment 27

[Cameron McCormack (:heycam)]

(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.)

Comment 28

[:Gavin Sharp [email: [e-mail]]]

(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.

Comment 29

[Cameron McCormack (:heycam)]

See question at end of comment 26.

Comment 30

[(not currently active) Ted Mielczarek]

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.

Comment 31

[Cameron McCormack (:heycam)]

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

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

Comment 32

[Cameron McCormack (:heycam)]

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.

Comment 33

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/7286dac15291

Comment 34

[Alex Keybl [:akeybl]]

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!

Comment 35

[Cameron McCormack (:heycam)]

https://hg.mozilla.org/releases/mozilla-beta/rev/a0dbfb9d776b

Comment 36

[Cameron McCormack (:heycam)]

Ah that should go on Aurora, not Beta:

https://hg.mozilla.org/releases/mozilla-beta/rev/1ed1cd3f4bd9 (backout)
https://hg.mozilla.org/releases/mozilla-aurora/rev/9841e712dda6

Comment 37

[Cameron McCormack (:heycam)]

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.

Comment 38

[:Gavin Sharp [email: [e-mail]]]

Backout link for Aurora:
https://hg.mozilla.org/releases/mozilla-aurora/rev/66f742b26f32

Comment 39

[Cameron McCormack (:heycam)]

Attachment: patch v2

Set GRE_MILESTONE in configure.  Tested locally on an inbound and beta branch, seems to do the right thing.

Comment 40

[:Gavin Sharp [email: [e-mail]]]

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.

Comment 41

[Cameron McCormack (:heycam)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/948ae77e36d6

Comment 42

[Cameron McCormack (:heycam)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d3a280f6c049

Comment 43

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/01f69299eba0

Comment 44

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/948ae77e36d6