1285458 - mistaken concerns about EARLY_BETA_OR_EARLIER disabling process

Status: RESOLVED INVALID

(Firefox Build System :: General, defect)

Description

[Daniel Holbert [:dholbert]]

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:
> #ifdef EARLY_BETA_OR_EARLIER
>    Telemetry::Accumulate(Telemetry::PWMGR_PASSWORD_INPUT_IN_FORM, !!mForm);
> #endif
https://dxr.mozilla.org/mozilla-central/source/dom/html/HTMLInputElement.cpp#5115

PROPOSAL:
So right now, partway through the beta cycle, release management lands a patch to change build/defines.sh like so:
-EARLY_BETA_OR_EARLIER=1
+EARLY_BETA_OR_EARLIER=
https://hg.mozilla.org/releases/mozilla-beta/rev/9512f571459bbc816dda70d2a554cad8e848e67e

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.

Thoughts?

Comment 1

[Mike Hommey [:glandium]]

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
  EARLY_BETA_OR_EARLIER=
elif test "$EARLY_BETA_OR_EARLIER"; then
  AC_DEFINE(EARLY_BETA_OR_EARLIER)
fi
AC_SUBST(EARLY_BETA_OR_EARLIER)


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

[Daniel Holbert [:dholbert]]

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:
 https://hg.mozilla.org/releases/mozilla-beta/pushloghtml?changeset=dd7af1fa4ece
...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 3

[Mike Hommey [:glandium]]

It becomes clearer when looking at the revision graph:
https://hg.mozilla.org/releases/mozilla-beta/graph/9512f571459bbc816dda70d2a554cad8e848e67e

Comment 4

[Daniel Holbert [:dholbert]]

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)