mistaken concerns about EARLY_BETA_OR_EARLIER disabling process



2 years ago
9 months ago


(Reporter: dholbert, Unassigned)



Firefox Tracking Flags

(firefox50 affected)




2 years ago
The EARLY_BETA_OR_EARLIER build switch is a bit of a footgun right now (as I've just discovered in bug 1277092 comment 8 through bug 1277092 comment 12).

In particular:
 - Other similar build flags (RELEASE_BUILD, NIGHTLY_BUILD) are either #defined to 1, or not #defined at all. Hence, they're typically checked using "#ifdef".

 - BUT, the EARLY_BETA_OR_EARLIER flag is *always defined* for beta builds!!! It's either defined to 1, or to the empty string.  So unlike the other flags, it must be checked using #if -- not #ifdef.

So "#ifdef EARLY_BETA_OR_EARLIER" does not do what it looks like it does -- it will always succeed, for ANY beta build (even "late" beta builds) right now. that bit me in bug 1277092.

And it's not just me who got confused about this -- it looks like we have at least one other similarly-broken #ifdef check in HTMLInputElement.cpp:
>    Telemetry::Accumulate(Telemetry::PWMGR_PASSWORD_INPUT_IN_FORM, !!mForm);
> #endif

So right now, partway through the beta cycle, release management lands a patch to change build/defines.sh like so:

I propose that we just *delete* (or comment out) this line, so that EARLY_BETA_OR_EARLIER ends up not-defined-at-all, instead of simply adjusting what it's defined to.



2 years ago
See Also: → bug 1277092
I doubt this is true. Specifically, the relevant code in old-configure.in is:

# Allow influencing configure with a defines.sh script.
. "${srcdir}/build/defines.sh"

# If we're not building a release build, define EARLY_BETA_OR_EARLIER if it is
# set in defines.sh
if test "$BUILDING_RELEASE"; then
  # Override value in defines.sh, if any
elif test "$EARLY_BETA_OR_EARLIER"; then

What this means in practice is that if EARLY_BETA_OR_EARLIER is empty, it is not AC_DEFINE'd.

Which means #ifdef should work.

Comment 2

2 years ago
It's possible I jumped to conclusions too hastily and that in fact everything is fine here.

What I know is:
 (1) Current beta ("48.0b5") definitely has the about:config pref layout.css.prefixes.webkit=true (which implies that an "#ifdef EARLY_BETA_OR_EARLIER" check in all.js has succeeded; hence, EARLY_BETA_OR_EARLIER is defined to something in this 48.0b5 build)

I assumed that this beta was built after the "EARLY_BETA_OR_EARLIER=" flip, because sledru landed a patch a week ago, with commit message "Post Beta 4: disable EARLY_BETA_OR_EARLIER"  https://hg.mozilla.org/releases/mozilla-beta/rev/9512f571459bbc816dda70d2a554cad8e848e67e

I assumed that patch took effect in beta 5 (since that would be "post beta 4"). But I'm less sure now, because about:buildconfig in this build shows that it was built based off of this push:
...which was *before* sledru's "post beta 4" commit.

So, despite the commit message, it seems that sledru's "post beta 4" commit is actually going to make it into beta 6 (not beta 5), which means that's the build where I should be hoping to see my about:config pref (layout.css.prefixes.webkit) auto-disabled.

Comment 4

2 years ago
Per bug 1277092 comment 18, my "#ifdef EARLY_BETA_OR_EARLIER" check did eventually end up doing what I'd hoped it would do (i.e. failing in late-beta builds), once we actually shipped a beta release after the flag had been tweaked.

--> Resolving this bug as INVALID (and clarifying bug summary to avoid asserting something that's not true)
Last Resolved: 2 years ago
Resolution: --- → INVALID
Summary: EARLY_BETA_OR_EARLIER disabling process is a footgun, since (unlike other channel build-time defines) it stays defined → mistaken concerns about EARLY_BETA_OR_EARLIER disabling process


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