435923 - Remove BUILD_OFFICIAL in favor of MOZILLA_OFFICIAL

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Nick Thomas [:nthomas] (UTC+12)]

We have both BUILD_OFFICIAL and MOZILLA_OFFICIAL in the build system. They're interchangeable in a bunch of places and Tinderbox sets both.

http://mxr.mozilla.org/seamonkey/search?string=BUILD_OFFICIAL
http://mxr.mozilla.org/seamonkey/search?string=MOZILLA_OFFICIAL

Comment 1

[Phil Ringnalda (:philor)]

cc'ing [e-mail] with absolute conviction that doing so will get me someone who can tell me whether to rename the one actual use of BUILD_OFFICIAL, or just remove it.

The only way this can really work is if we rename or remove http://mxr.mozilla.org/security/source/security/coreconf/ruleset.mk#213 so we can completely get rid of BUILD_OFFICIAL (and then wait years for it to leach out of the collective folklore of how to build, since even years after you didn't have to set either one to get a non-zero build id, people still advise setting both for that purpose).

BUILD_OFFICIAL does two things, "set +e" in the LOOP_OVER_DIRS for NSS, so that a build error in one won't stop the others from being built, which sounds sort of useful for NSS tinderboxes (which don't set it), to get all the possible errors in a single run instead of just one, and not very useful for consumers of a tag, which also don't set it anymore (near as I can tell, it's gone from all of current Fx/Tb/SM, though older things like Fx3.0 still set it), and also echoes a timestamp after each directory, which might be sort of useful on NSS tinderboxes (which don't set it), but is rather uninteresting on things like last night's Fx 3.0.x Linux nightly, which took just over 3 minutes from the first to the last timestamp, racking up eleven separate "Wed Jul  1 04:35:19 PDT 2009" stamps along the way. If you're getting eleven steps in a single tick, either your resolution is too low, or you're instrumenting something that's not worth counting, or both.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Phil, what's the objective here?
Is it
a) to make NSS builds behave with MOZILLA_OFFICIAL as they now do with 
BUILD_OFFICIAL?
b) to free up BUILD_OFFICIAL for some other use?
c) to eliminate the present BUILD_OFFICIAL behavior entirely?
d) other?

Would a solution that made BUILD_OFFICIAL and MOZILLA_OFFICIAL each have
the same effect be a satisfactory solution?

Is this only an issue for NSS?

Comment 3

[Phil Ringnalda (:philor)]

Oh, sorry, the objective's only clear to people who've spent too much time in Mozilla build config: c) if you don't want the behavior anymore, or d) rename the var to trigger the NSS behavior to something which couldn't possibly be confused with MOZILLA_OFFICIAL, and ideally which is related to what it does and who sets it.

The problem is that nobody knows what BUILD_OFFICIAL means, so sometimes a check outside of NSS that should be for MOZILLA_OFFICIAL is for MOZILLA_OFFICIAL, and other times it's for ifneq(,$(MOZILLA_OFFICIAL)$(BUILD_OFFICIAL)) or another similar construction. The objective is to not have two vars that sound very similar, but don't really mean the same thing, and that have similar but not identical effects, and that have effects that are only vaguely related to their name, by removing every use of BUILD_OFFICIAL outside of NSS, and either renaming the use in NSS to something clearer, or removing it.

If you're happy with what BUILD_OFFICIAL does, I'd like to rename it to either something like NSS_BUILD_CONTINUE_ON_ERROR or at least NSS_OFFICIAL (though that's still a bit confusing since I'd expect that to be something the NSS tinderboxes set, which they don't, and if you only set it for official releases (if you do set it for releases), NSS_RELEASE might be better).

My fumbling understanding of what it does, though, makes me think you don't really like it, since continuing to build after an error to find as many errors as possible, and logging the time each build step takes, are not things that someone building a release typically wants, and it's not being set on your tinderboxes where I'd think it would be set if it was useful to you, so in that case I'd be happy to simplify things by just removing it.

Comment 4

[Phil Ringnalda (:philor)]

Maybe this is more clear:

NSS is the only thing which currently does something if-and-only-if BUILD_OFFICIAL is set, so whatever the case may have been in the past, now NSS owns BUILD_OFFICIAL.

Outside of NSS there are four things that are done only if MOZILLA_OFFICIAL is set, and another four that are done if either MOZILLA_OFFICIAL or BUILD_OFFICIAL is set. That distinction isn't needed or wanted, it's just an accident of who was copy-pasting from what.

The only documentation that seems to exist around their meaning is outdated (and probably incorrect even at the time) advice to set both of them so that the build id in your user-agent string isn't "00000000".

Any one of "remove the BUILD_OFFICIAL behavior from NSS" or "make the BUILD_OFFICIAL behavior in NSS triggered by MOZILLA_OFFICIAL" or "make the BUILD_OFFICIAL behavior in NSS triggered by NSS_FOOPY_GOATS" will work fine to meet the desires of this bug, which is to not have Mozilla developers asking "now, when I want an ifdef for official builds only, am I supposed to check for MOZILLA_OFFICIAL, BUILD_OFFICIAL, or both?" and have absolutely nobody who is able to answer.

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

One more question: Would you personally prefer that MOZILLA_OFFICIAL trigger
NSS's maximum build errors behavior? Or that it not?  

Here is my estimate of probable explanations for BUILD_OFFICIAL in NSS, in 
descending order of probability:

1) No builder of NSS depends on BUILD_OFFICIAL for this maximal error behavior,
so we can change it from BUILD_OFFICIAL to whatever we want (NSS_FOOPY_GOATS :)

2) Sun's or Red Hat's release engineers set this in their (non-Mozilla) builds 
of NSS, but no-one else uses it at all.  In this case, we ask those release engineers to change to use NSS_FOOPY_GOATS along with us.  

3. Someone else, not closely affiliated with NSS or Mozilla, builds NSS with 
BUILD_OFFICIAL, and would be put off if we didn't warn them in advance.  
The only way I know to find these (prior to making the change) is to announce 
the change in mozilla.dev.tech.crypto, and see if anyone responds.  

I will attempt to poll the release engineers and the dev.tech.crypto crowd
within the next 7 days (lots of folks are on vacation now through the 5th).

Comment 6

[Phil Ringnalda (:philor)]

Personally, I would prefer that it not be triggered by MOZILLA_OFFICIAL: in theory, MOZILLA_OFFICIAL should mean the Mozilla tinderboxes (though lots of developers and people building for themselves set it too, either for an actual reason or mostly because of the long-ago requirement to set it to get a non-zero build id), and since the non-NSS tinderboxes should be building a stable tag, and shouldn't be a source of either debug information (except for the first set of builds after a new tag is picked up) or build optimization information, it doesn't make sense to have them produce noisier logs, or take longer to fail if there is somehow going to be a build failure in NSS. It's also not kind to the hypothetical person who really does want the BUILD_OFFICIAL effect, to require them to accept all the side effects of MOZILLA_OFFICIAL to get their timestamps and set +e. 

NSS_FOOPY_GOATS is a happier solution all around, except for my remaining question: what do NSS developers build, when they build NSS together with a Firefox? If it's mozilla-central, no problem, I can have the m-c/configure.in pass along NSS_FOOPY_GOATS without any trouble, but if it's either 1.9.1 or 1.9, it's going to take a little more coordination and approval to get landed.

Comment 7

[Phil Ringnalda (:philor)]

Attachment: Part one - m-c bits

This part can land separately from the configure/autoconf/NSS bits, since none of this really needs BUILD_OFFICIAL for anything (especially not the mkdepend chunks, which haven't needed to unset them both to avoid build_number issues since the whole ugly build_number thing went away early in the 1.9 cycle).

The Sisyphus set-build-env.sh bit bc's going to take care of, once he's made sure that just setting MOZILLA_OFFICIAL really will do the job for him, even back to 1.8.

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

In reply to comment 6:
> what do NSS developers build, when they build NSS together with a Firefox? 

The answer is: we don't build Firefox.  We download Firefox nightlies, and
then replace the NSS libs in those nightlies with our own NSS builds.  
This also means we don't have to use Hg (! :)  

I'll pursue NSS_FOOPY_GOATS, or some approximation thereof. :)

Comment 9

[Phil Ringnalda (:philor)]

Hmm, so you aren't actually ever configuring with /mozilla/configure, and the answer is "absolutely nobody is using BUILD_OFFICIAL since it's not getting passed down to that Makefile, and NSS_FOOPY_GOATS needs to be in nspr/configure.in instead"?

Comment 10

[Phil Ringnalda (:philor)]

Attachment: all the m-c bits [Checkin: Comment 13]

And that makes (my) life way easier: no tinderboxes building mozilla-central code set BUILD_OFFICIAL, and if no NSS developers are configuring with mozilla-central's configure, there's no point in an AC_SUBST that will never reach down to the single remaining use.

Comment 11

[(not currently active) Ted Mielczarek]

Comment on attachment 387120 [details] [diff] [review]
all the m-c bits
[Checkin: Comment 13]

Beautiful!

Comment 12

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 387120 [details] [diff] [review]
all the m-c bits
[Checkin: Comment 13]

> if no NSS developers are configuring with mozilla-central's configure, there's 
> no point in an AC_SUBST that will never reach down to the single remaining use.

I'm sure that's true.  I'm deferring to Ted's review for this patch.

Comment 13

[Phil Ringnalda (:philor)]

http://hg.mozilla.org/mozilla-central/rev/b161c43c41f0

Comment 14

[Nelson Bolyard (seldom reads bugmail)]

Um, today the NSS team reached agreement to change BUILD_OFFICIAL to something 
else in NSS's coreconf makefiles.  Should that change be part of this bug?

Comment 15

[Nelson Bolyard (seldom reads bugmail)]

Attachment: NSS coreconf patch [Checkin: Comment 17]

Christophe, please review.

Comment 16

[Christophe Ravel]

Comment on attachment 387387 [details] [diff] [review]
NSS coreconf patch
[Checkin: Comment 17]

r+ christophe

Comment 17

[Nelson Bolyard (seldom reads bugmail)]

Changed BUILD_OFFICIAL to NSS_BUILD_CONTINUE_ON_ERROR
Checking in coreconf/ruleset.mk; new revision: 1.22; previous revision: 1.21

Comment 18

[Nelson Bolyard (seldom reads bugmail)]

I want to revisit this.  The question I want to ask is: 

Who (if anyone) wants this behavior that is now enabled by 
NSS_BUILD_CONTINUE_ON_ERROR ?  

If you want it, please add a comment here saying so.

In bug 525221, Ben Smedberg requests that we stop using this in-line shell
script to loop over subdirectories, and instead use gmake's built-in for
and wildcard operators.  He claims this makes builds faster.  We can do 
that in NSS, but only if we're willing to give up the NSS_BUILD_CONTINUE_ON_ERROR behavior completely.  

So, my question is, does anybody actually USE that behavior?

Comment 19

[(not currently active) Ted Mielczarek]

AFAIK, nobody in Mozilla cares about that behavior.

Comment 20

[Justin Wood (:Callek)]

One case missed (not sure its importance, but it does set MOZILLA_OFFICIAL as well);

/testing/sisyphus/bin/set-build-env.sh :: http://mxr.mozilla.org/mozilla-central/search?string=BUILD_OFFICIAL

Comment 21

[Phil Ringnalda (:philor)]

Wasn't missed: comment 7, if it annoys you take it up directly with bc.

Comment 22

[Justin Wood (:Callek)]

(In reply to comment #21)
> Wasn't missed: comment 7, if it annoys you take it up directly with bc.

(In reply to comment #7)
> The Sisyphus set-build-env.sh bit bc's going to take care of, once he's made
> sure that just setting MOZILLA_OFFICIAL really will do the job for him, even
> back to 1.8.

Ahh ok, missed that bit from here when I went to investigate a related bug.  No big deal.

Comment 23

[Serge Gautherie (:sgautherie)]

(In reply to comment #21)
> take it up directly with bc.

I filed bug 542352.