53226 - need to ensure that FORCE_PR_LOG is off for final builds

Status: RESOLVED FIXED

(SeaMonkey :: General, defect, P2)

Description

[Judson Valeski]

http://lxr.mozilla.org/seamonkey/search?string=FORCE_PR_LOG

This generates bloat, we need to make sure we don't build w/ this flag in final
builds.

Comment 1

[Doron Rosenberg (IBM)]

do you mean mozilla final or netscape final?

Comment 2

[Judson Valeski]

-> chofmann. this is a final build flag.

Comment 3

[chris hofmann]

over to granrose. jj leaf to cc

Comment 4

[Jon Granrose]

this looks nasty.

what exactly does FORCE_PR_LOG do?  And what exactly does disabling it give us?

It looks like we have to check in on the branch to remove all the -DFORCE_PR_LOG
entries since I'd expect we don't want this turned off in anything but the
Netscape and/or mozilla official releases (does mozilla care about this possible
bloat?).  If that's the case this may be moved to bugscape.

ccing cls, lpham, and kysmith.

Comment 5

[Judson Valeski]

it's not as nasty as it appears.

force_log was put in to enable PR_LOGGING in release builds so we could provide
logging to QA in optimized bits. It's been argued that it should be removed
altogether, I'm easily convinced of this :-). It's not global, each module that
wants logging in opt bits has to define it.

We want it removed in a "final" build so all those logging statements get boiled
out and we reduce code bloat.

Comment 6

[Jon Granrose]

cc lisa for her input from the QA perspective about turning off logging in the
release builds and when we want to do this.

Personally, I would think this wouldn't make much difference to QA, but a world
of difference to engineers who might have a harder time debugging a problem
without the log info in the release builds.

Comment 7

[lchiang]

Actually, this may affect QA as well.  Mail uses a lot of logging to figure out
problems with IMAP, POP, SMTP, NNTP protocols.  See
http://www.mozilla.org/quality/mailnews/mail-troubleshoot.html#imap.

Does turning this flag off prevent those protocols from being generated?

Comment 8

[Jon Granrose]

retargeting for 0.9.1 since we still don't know when we want to turn this off. 
Regardless, we'll need to turn this off for a milestone or beta to test it
before we turn it off for any RTM releases.

Comment 9

[Jon Granrose]

I'm not going to have time to do this, reassigning to cls.  setting to moz 0.9 
since we want to get this done asap.

cls - can you put together a patch to make FORCE_PR_LOG a global and get that 
reviewed and in?  Then once moz 0.9 is cut, turn it off on the branch so the 
release builds don't have it.

Comment 10

[lchiang]

cc: mscott.  Scott, can you see my question on 3-29?  We rely on the protocol
logs a lot on release builds.

Comment 11

[Scott MacGregor]

Sorry guys, you can't turn off FORCE_PR_LOG globally. Our RTM imap builds have
to have logging capability. We work through most of our interop issues via imap
logs companies submit to us from our RTM bits.

I keep having to remind Judson about this every time he gets it in his head to
disable logging for everyone in rtm builds =).....

Comment 12

[cls]

so can I just WONTFIX this?

Comment 13

[Judson Valeski]

this flag is abused. the whole foundation of it was so that RTM folks who 
*needed* this logging could have it. now people are using it all over the place. 
we should determine which modules have the *real* needs (right now, only 
mail/news AFAIK), and those modules should have to opt-in at build time for 
NS/Mozilla browser RTMs), default is to not log.

I need the ability to ensure that *no* logging is on in *my* RTM build though 
(thus build time flags). I'm emebedding gecko, and I don't want *any* logging to 
occur, _anywhere_.

The whole point was that this would be a build flag, *not* a #define (which is 
what people are doing) in source code. The interested modules would build 
themselves w/ this flag in the cmd line, *not* the .h/.c*.

Comment 14

[cls]

oooook.

Waterson, any objections to overriding the --disable-logging option to handle
this PR_Log case as well or do I have to do something lame like add a
--disable-pr-logging ?

Comment 15

[Chris Waterson]

Naw, that should be fine.

Comment 16

[cls]

Attachment: Update --disable-logging to turn off PR_Log

Comment 17

[Judson Valeski]

- equiv on windows is what? just setting build time flags as usual?
- this looks right to me, but we need to ensure that mail/news is built (for 
moz/NS) w/ this flag on. how do we ensure that?

Comment 18

[cls]

Windows?  *sigh*  You don't ensure that this is on for mailnews.  This is the
all or nothing option that you requested.  It's on by default (for unix anyway)
and you turn it off by specifying the --disable-logging option.  I'll cobble
togther a NO_LOGGING equiv for windows and I'll have to hand off the mac changes
to someone else.

Comment 19

[cls]

Attachment: Add NO_LOGGING logic to win32 builds

Comment 20

[Judson Valeski]

>> This is the all or nothing option that you requested.
fair enough. thanks!

Comment 21

[Jon Granrose]

this may help jud but won't help the build team since we package the embedding
bits based on the mozilla bits.  so we'd either have to turn off all logging for
the browser and embedding, or build twice: browser w/logging, embedding w/o logging.

since embedding has priority, if we turn this on that means no logging in
release builds for mailnews until we straighten this out.  cc'ing phil and
selmer for executive oversight.

Comment 22

[Judson Valeski]

currently embedding customers don't use our packaged bits for their dists. so
let's leave this off for now (that will keep QA/mail/news happy). The biggest
thing is to allow this build flag to "work" so embedding customers can use it.
Thanks!

Comment 23

[chris hofmann]

do early in 0.9.1

Comment 24

[Phil Peterson]

Agree with Jud. We add the build flag which makes PR_LOG statements compile to
nothing and embedding customers run builds with that flag enabled. If
Netscape-branded builds don't run with the flag (in order to meet the mail
team's needs) that's fine with us.

Comment 25

[cls]

Attachment: Add pr_log option to mac build system

Comment 26

[Simon Fraser [no longer active]]

r=sfraser on mac build changes.

Comment 27

[(not reading, please use [e-mail] instead)]

so, for rtm mozilla and netscape bits we are going to force PR_Logging, right? 
but not for the embedding bits.  (is that something release will be doing? 
building twice, or having two trees, one for mozilla and one for embedding?)

as lchiang and mscott pointed out, we in mailnews land needs to ship rtm bits
with logging.

please confirm.  if yes, then r=sspitzer for the mailnews changes.

Comment 28

[Daniel (Leaf) Nunes]

client release won't be flipping this on for anything we produce, at first. I'm 
sure a friendly embedding testing customer will file an RFE for us to produce 
separate builds of embedding packages with it on, but the idea of *this* bug is 
to allow other people (outside of netscape client development) to produce 
embeddable stuff without logging on.

Comment 29

[Phil Peterson]

> I'm sure a friendly embedding testing customer will file an RFE for us to 
> produce separate builds of embedding packages with it on

I don't expect that. Our embedding customers don't take CPD-generated binaries
-- they do their own builds from source.

Comment 30

[Daniel (Leaf) Nunes]

but if qa is doing performance/footprint testing of the embedding tarfiles we 
spit out every day, the numbers are not going to match what is actually seem by 
customers using source. Isn't that the point of the embedding tarfiles?

Comment 31

[Phil Peterson]

Leaf, the embedding customers already don't see precisely the same behavior as
the embedding tarfiles would show -- they're not running gtkEmbed or mfcEmbed --
they're running their own apps.

In some cases, the build settings they use even need to be different than ours
(i.e. they use settings we wouldn't want, like this PR_LOG thing). Could that
make bug reproducibility hard? Sure. Welcome to our hell :-)

Comment 32

[chris hofmann]

our embedding test apps just help us to see what kinds of behavior
and estimated performance and size characteristics a wide variety 
of customers might see.   Individual customer's mileage may vary,
additional Taxes and License Fees may be appilicable to Residents of New Jersey
and Massachusetts and Oregon. 

Comment 33

[cls]

Attachment: Updated patch.

Comment 34

[Daniel (Leaf) Nunes]

r=leaf, though this is probably going to slow down the regular app, as
PR_LOGGING is going to be turned on for everything now, right?

Comment 35

[Judson Valeski]

y. I bet this is a non-trivial perf hit.

Comment 36

[cls]

Yep, that's the downside.  An alternative is to have configure set MOZ_LOGGING
or something and then have each module have a:
#ifdef MOZ_LOGGING
#define FORCE_PR_LOG
#endif

if they want to force logging on.  

Aieee...back to the random patch maker (damn you leaf :-)).

Comment 37

[Phil Peterson]

Is this really what we want? I think we want a way to force it off in the few
places which try to force it on. This patch appears to be a way to force it on
for all code everywhere. 

Comment 38

[cls]

Attachment: Updated, updated patch.

Comment 39

[Daniel (Leaf) Nunes]

r=leaf for 32673

Comment 40

[cls]

Attachment: security section of updated, updated patch

Comment 41

[cls]

Patch has been checked in.  Notice about wrapping FORCE_PR_LOG defines in a
MOZ_LOGGING ifdef has been sent out.  Marking fixed.