853071 - Create new build-time defines for channel-specific features

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Gregory Szorc [:gps]]

The build system doesn't currently have a unified primitive for answering questions like "am I on a pre-release or release-like channel?" As a result, our management of Nightly/Aurora-only features is haphazard. In some cases, I even believe we rely on reverting prefs when trees are uplifted! This is fragile and prone to accidents.

I think it is desirable for the build system to offer a well-defined convention to express when code or features should be enabled.

Complicating matters are features that need to distinguish between local and official builds (e.g. crash reporter), official channels vs no channels (e.g. inbound doesn't use a channel IIRC).

Here is a rough proposal. Names could be better. Let's bike shed on the substance. Some of these variables may already exist or exist under different names.

PRERELEASE_BUILD - Bool that is true only on Nightly and Aurora-like builds. This is not tied to channels. Therefore, it is global across local and official builds/channels.

RELEASE_BUILD - Opposite of PRERELEASE_BUILD.

DISTRIBUTED_BUILD - Bool true on builds meant for distribution or consumption by others (e.g. non-local builds).

LOCAL_BUILD - Opposite of DISTRIBUTED_BUILD. Only on local, non-official builds.

OFFICIAL_CHANNEL - Bool true when the build is part of an official distribution channel (like Nightly, Aurora, Beta, or Release).

What else do we need to key off?

Comment 1

[Mike Hommey [:glandium]]

Do we really need 5 boolean values? Can't we use something like, for example, BUILD_TYPE = release distributed local ?

Comment 2

[Alex Keybl [:akeybl]]

See https://bug803276.bugzilla.mozilla.org/attachment.cgi?id=679743 for some sort of precedent.

Comment 3

[Gregory Szorc [:gps]]

(In reply to Mike Hommey [:glandium] from comment #1)
> Do we really need 5 boolean values? Can't we use something like, for
> example, BUILD_TYPE = release distributed local ?

I'd be fine with a field set. Obviously make has $(filter). Python is Python. Can we handle that in the preprocessor?

Comment 4

[Mike Hommey [:glandium]]

(In reply to Gregory Szorc [:gps] from comment #3)
> I'd be fine with a field set. Obviously make has $(filter). Python is
> Python. Can we handle that in the preprocessor?

Not the C preprocessor

Comment 5

[(not currently active) Ted Mielczarek]

That feels like a lot of granularity without a real purpose. Why don't we just extend the existing RELEASE_BUILD mechanism to be usable as a makefile variable and global AC_DEFINE?

We already have MOZILLA_OFFICIAL which roughly serves as "something built by Mozilla build machines", it's what we condition enabling crash reporting on.

Comment 6

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

gps, bsmedberg, johnath, akeybl, lsblakk and I met today to discuss this. The plan we agreed upon:

- add an ONLY_EARLY_BETAS (name bikeshedding welcome) variable that is defined until the 2nd or 3rd beta (exact point where it reverts to false to be managed explicitly by rel-mgmt). There should be a fail safe mechanism to ensure that it is not defined for release builds if all of rel-mgmt gets hit by a bus. The idea is that this will be used infrequently, and we should enforce that at merge time (and possibly with a pre-commit hook). 
- add a NIGHTLY_BUILD variable, only defined for pre-Aurora builds (i.e. a stricter condition than #ifndef RELEASE_BUILD, which includes Aurora)

I think that these should be global DEFINES, rather than the  PREF_PPFLAGS-only nature of RELEASE_BUILD, and that we should switch RELEASE_BUILD to match (as Ted suggests in comment 5).

Comment 7

[[e-mail] (:bhearsum)]

Thanks for the summary!

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #6)
> gps, bsmedberg, johnath, akeybl, lsblakk and I met today to discuss this.
> The plan we agreed upon:
> 
> - add an ONLY_EARLY_BETAS (name bikeshedding welcome) variable that is
> defined until the 2nd or 3rd beta (exact point where it reverts to false to
> be managed explicitly by rel-mgmt).

Just to make sure I understand correctly: rel-mgmt will make a commit when they deem appropriate that flips the flag off (presumably, a change to the mozconfig).

> There should be a fail safe mechanism to
> ensure that it is not defined for release builds if all of rel-mgmt gets hit
> by a bus.

Were there any ideas floated about how to do this?

Comment 8

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

(In reply to Ben Hearsum [:bhearsum] from comment #7)
> Just to make sure I understand correctly: rel-mgmt will make a commit when
> they deem appropriate that flips the flag off (presumably, a change to the
> mozconfig).

Yeah, something like that.

> > There should be a fail safe mechanism to
> > ensure that it is not defined for release builds if all of rel-mgmt gets hit
> > by a bus.
> 
> Were there any ideas floated about how to do this?

Not really. Something that might work is separating the beta and release mozconfigs in the tree (and somehow otherwise ensuring that the only difference is the lack of this magic flag.

Comment 9

[Mike Hommey [:glandium]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #8)
> (In reply to Ben Hearsum [:bhearsum] from comment #7)
> > Just to make sure I understand correctly: rel-mgmt will make a commit when
> > they deem appropriate that flips the flag off (presumably, a change to the
> > mozconfig).
> 
> Yeah, something like that.

This would be better in confvars.sh, so that it's also caught by non tbpl builds.

Comment 10

[Alex Keybl [:akeybl]]

Who wants this bug? :)

Comment 11

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

(In reply to Mike Hommey [:glandium] from comment #9)
> This would be better in confvars.sh, so that it's also caught by non tbpl
> builds.

confvars is app-specific and I think we want this to be global, so that's probably not ideal. But I agree that we want to avoid mozconfigs - we can do something similar with a global confvars.sh-like file, I guess.

Comment 12

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

Attachment: patch

This patch:
- adds a defines.sh at the top level, that just gets sourced into configure

- adds EARLY_BETA_OR_EARLIER to that file, since this patch will land on trunk. The idea is that this is the "switch" that rel-mgmt will manually flip back to "EARLY_BETA_OR_EARLIER=" once we are past the point of "early beta".

- modifies configure to AC_SUBST/AC_DEFINE EARLY_BETA_OR_EARLIER, if it is set in defines.sh, but not if BUILDING_RELEASE is set.

- creates "beta" mozconfigs next to the existing "release" mozconfigs. They are identical to the "release" mozconfigs. The "release" mozconfigs get an added "export BUILDING_RELEASE=1", in order to distinguish our build of releases vs. our builds of beta, for the purposes of safe fallback behavior for EARLY_BETA_OR_EARLIER. This will require that rel-eng switch to using different configs for beta vs. release.

- moves the existing RELEASE_BUILD logic to configure (from rules.mk), and makes it a global AC_SUBST/AC_DEFINE rather than only affecting PREF_PPFLAGS

- adjusts some services/healthreport code to make use of the global RELEASE_BUILD define rather than rolling its own

- adds a NIGHTLY_BUILD AC_SUBST/AC_DEFINE that is set when GRE_MILESTONE contains "a1"

f?bhearsum for the requirement this would impose of using different mozconfigs for release vs. beta.

f?gps for everything else.

I've only tested this lightly, by manually hacking my mozconfig/milestone.txt. I think it generally works but may have missed an edge case.

Comment 13

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

I forgot to qref to include the addition of defines.sh, but its contents are just:

# Define indicating that this build is prior to one of the early betas. To be
# unset mid-way through the beta cycle.
EARLY_BETA_OR_EARLIER=1

Comment 14

[Gregory Szorc [:gps]]

Comment on attachment 741610 [details] [diff] [review]
patch

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

On the right track methinks.

::: configure.in
@@ +3921,5 @@
> +# - otherwise, we're building Release/Beta (define RELEASE_BUILD)
> +case "$GRE_MILESTONE" in
> +  *a1*)
> +      NIGHTLY_BUILD=1
> +      AC_DEFINE(NIGHTLY_BUILD)

Note that AC_DEFINE doesn't make it into moz.build and Makefiles: just mozilla-config.h and the preprocessor. AC_SUBST covers everything.

@@ +4387,5 @@
>    AC_MSG_RESULT([no])
>  fi
>  
> +# Allow influencing configure with a build-defines.sh script.
> +. "${srcdir}/defines.sh"

I don't like this living in top-level. Files in top-level (especially executable files) just confuse new developers and make it seem like something important for a large audience.

Let's move this to /build.

@@ +4397,5 @@
> +  EARLY_BETA_OR_EARLIER=
> +elif test "$EARLY_BETA_OR_EARLIER"; then
> +  AC_DEFINE(EARLY_BETA_OR_EARLIER)
> +fi
> +AC_SUBST(EARLY_BETA_OR_EARLIER)

Why AC_DEFINE for one but AC_SUBST for the other?

Also, if it's undefined, it should be acceptable to omit the AC_DEFINE/AC_SUBST.

Comment 15

[[e-mail] (:bhearsum)]

Comment on attachment 741610 [details] [diff] [review]
patch

We'll have a few small adjustments to make, but nothing that makes this plan unworkable.

Comment 16

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

Attachment: patch v2

Adjusted the location of defines.sh, but left the other stuff as-is per discussion on IRC.

Comment 17

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

Ben: am I good to land this? The "safeguard" code won't work until the dependent bugs are fixed, but the mozconfig changes shouldn't break anything, right?

Comment 18

[[e-mail] (:bhearsum)]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #17)
> Ben: am I good to land this? The "safeguard" code won't work until the
> dependent bugs are fixed, but the mozconfig changes shouldn't break
> anything, right?

Sorry for the delayed response -- you're fine to land as far as I'm concerned. The RelEng bugs are about pre-release checks, which only affect Beta and higher.

Comment 19

[Alex Keybl [:akeybl]]

RelEng believes FF23 is a better target for this build-time feature.

Comment 20

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/23bb737193ed

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/23bb737193ed