Closed Bug 853071 Opened 7 years ago Closed 7 years ago

Create new build-time defines for channel-specific features

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(firefox21 wontfix, firefox22- wontfix, firefox23+ fixed)

RESOLVED FIXED
mozilla23
Tracking Status
firefox21 --- wontfix
firefox22 - wontfix
firefox23 + fixed

People

(Reporter: gps, Assigned: Gavin)

References

Details

Attachments

(1 file, 1 obsolete file)

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?
Do we really need 5 boolean values? Can't we use something like, for example, BUILD_TYPE = release distributed local ?
(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?
(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
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.
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).
Thanks for the summary!

(In reply to :Gavin Sharp (use gavin@gavinsharp.com 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?
(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.
(In reply to :Gavin Sharp (use gavin@gavinsharp.com 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.
Who wants this bug? :)
Summary: Better handling of features across versions and channels → Create new build-time macros for channel-specific features
(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.
Attached patch patch (obsolete) — Splinter Review
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.
Assignee: nobody → gavin.sharp
Status: NEW → ASSIGNED
Attachment #741610 - Flags: feedback?(gps)
Attachment #741610 - Flags: feedback?(bhearsum)
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
Summary: Create new build-time macros for channel-specific features → Create new build-time defines for channel-specific features
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.
Attachment #741610 - Flags: feedback?(gps) → feedback+
Comment on attachment 741610 [details] [diff] [review]
patch

We'll have a few small adjustments to make, but nothing that makes this plan unworkable.
Attachment #741610 - Flags: feedback?(bhearsum) → feedback+
Depends on: 865814
Attached patch patch v2Splinter Review
Adjusted the location of defines.sh, but left the other stuff as-is per discussion on IRC.
Attachment #741610 - Attachment is obsolete: true
Attachment #741989 - Flags: review?(gps)
Attachment #741989 - Flags: review?(gps) → review+
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?
Flags: needinfo?(bhearsum)
(In reply to :Gavin Sharp (use gavin@gavinsharp.com 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.
Flags: needinfo?(bhearsum)
RelEng believes FF23 is a better target for this build-time feature.
https://hg.mozilla.org/mozilla-central/rev/23bb737193ed
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Blocks: 875342
Depends on: 901892
Depends on: 902066
Depends on: 902084
No longer depends on: 902066, 902084
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.