561674 - Stop defining DEBUG_<username>

Status: RESOLVED FIXED

(Firefox Build System :: General, enhancement)

Description

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

http://groups.google.com/group/mozilla.dev.platform/browse_thread/thread/b0bedb1c7db30f5b#

Comment 1

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Based on feedback on .platform, we should just remove the define in configure.in and leave the code in the tree for the owners to change.

Comment 2

[Daniel Holbert [:dholbert]]

(In reply to comment #1)
> Based on feedback on .platform, we should just remove the define in
> configure.in and leave the code in the tree for the owners to change.

I don't think dbaron meant that the owners necessarily had to be the ones to remove that code -- more that it's a separate issue from removing the auto-define, and may need some case-by-case handling.

That is to say: the code-removal should happen in separate bug(s) from this one, but it could be performed by people other than the user who added the #ifdef'd code, as long as that user approves (or is long-gone from the mozilla community).

Comment 3

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

Comment 5

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

It was pointed out to me that one of the few good uses of this is having debug code run on tinderbox for quashing random oranges.  We probably should have tinderbox continue to define DEBUG_cltbld through the workarounds discussed in .platform.

Comment 6

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

http://hg.mozilla.org/mozilla-central/rev/54c9fad6aa00

Comment 7

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I backed this out because of the reasons in comment 5 (there's some debug code in there now that dbaron didn't want to turn on for everyone).

Comment 8

[timeless]

at some point i'll post to the newsgroup explaining why i think this is a bad idea.

i have dozens of mozconfigs spread across a bunch of computers. and i subscribe to debug bits in a bunch of areas. in xpconnect we actually have a proper DEBUG_xpchacker category and users subscribe to it. spidermonkey also has a couple of these

I'm willing to try to refactor other things to use categories, but i don't think that we should make it harder for people to use their systems. it's also helpful to be able to recognize which things a certain person uses to debug problems. whereas most people do not version their mozconfigs.

Comment 9

[Justin Wood (:Callek)]

(In reply to comment #8)
> at some point i'll post to the newsgroup explaining why i think this is a bad
> idea. [snip]

I've been thinking on this, and have a proposal [IFF ted accepts the concept]

Rather than requireing developers to manually define DEBUG_* in their flags, would it be acceptible for a configure value of --enable-debug-var[s]=xyz,abc,123

for a -DDEBUG_xyz -DDEBUG_abc -DDEBUG_123 output

So people who care about their own DEBUG_* foo, can set it in the mozconfig, with a push for making it module/use specific rather than dev's checking in "personal-only" hacking code.

Comment 10

[Daniel Holbert [:dholbert]]

(In reply to comment #9)
> Rather than requireing developers to manually define DEBUG_* in their flags,
> would it be acceptible for a configure value of
> --enable-debug-var[s]=xyz,abc,123
> 
> for a -DDEBUG_xyz -DDEBUG_abc -DDEBUG_123 output

Just for the record, since it hasn't been mentioned on this bug yet -- one already-working way of doing this (from one of Ted's newsgroup posts) is:
   --enable-debug="-DDEBUG_xyz -DDEBUG_abc -DDEBUG_123"
While this isn't as elegant as Callek's suggested syntax in comment 9, I don't think it's prohibitively clunky for current use.

So from Comment 5 & Comment 7, it sounds like we just need to add
  --enable-debug="-DDEBUG_cltbld"
to tinderbox mozconfigs, in order for this to re-land.  Is that correct?  Is there already a bug filed on that?

Comment 11

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to comment #10)
> So from Comment 5 & Comment 7, it sounds like we just need to add
>   --enable-debug="-DDEBUG_cltbld"
> to tinderbox mozconfigs, in order for this to re-land.  Is that correct?  Is
> there already a bug filed on that?

Yes.  It's blocking this one.

Comment 12

[Daniel Holbert [:dholbert]]

(In reply to comment #11)
> > So from Comment 5 & Comment 7, it sounds like we just need to add
> >   --enable-debug="-DDEBUG_cltbld"
> Yes.  It's blocking this one.

Ok -- that was bug 563193, but that's now WONTFIX -- it looks like ted & catlee aren't super excited about supporting tinderbox-only debug code.

However, good news -- the only instance of DEBUG_cltbld is now gone, in bug 563980 comment 11. This MXR search shows there's no more of this debug:
http://mxr.mozilla.org/mozilla-central/search?string=DEBUG_cltbld

So, IIUC, this patch should be fine to land again, right?

Comment 13

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to comment #12)
> So, IIUC, this patch should be fine to land again, right?

AIUI yes.

Comment 14

[Daniel Holbert [:dholbert]]

dbaron suggested in #developers (and I agree) that, before this lands, it'd be best to add a mozconfig option like:
    --enable-debug-username=FOO
or perhaps...
    --with-debug-username=FOO
...as timeless_mbp suggests, because presumably this flag would do nothing in non-debug builds, and so "enable" isn't quite the right word.

I'll file a dependent bug on that.

Comment 15

[Daniel Holbert [:dholbert]]

Filed bug 565191 on --with-debug-label (or whatever we want to call it).

Comment 16

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

http://hg.mozilla.org/mozilla-central/rev/7efb41b5e57a

Comment 17

[Daniel Holbert [:dholbert]]

Just talked with khuey on IRC -- he'd missed comment 14 & 15 before landing, so he wasn't aware of the dependency on bug 565191 (--with-debug-label).

If it's possible to get that bug rubber-stamped and landed today, then I think this can stay in -- otherwise, we probably need to back this out before Monday morning MV-time.

Comment 18

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Backed out again.  Third time is the charm, right ;-)

Comment 19

[Justin Wood (:Callek)]

(In reply to comment #17)
> Just talked with khuey on IRC -- he'd missed comment 14 & 15 before landing, so
> he wasn't aware of the dependency on bug 565191 (--with-debug-label).

For the record, whenever this lands I highly recommend some newsgroup posting + a blog post somewhere about the change. As this will affect some active developers.

Comment 20

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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

http://groups.google.com/group/mozilla.dev.platform/browse_thread/thread/1426e66cdb9f0324#

I don't have a blog on planet so if dholbert, ted, or Callek want to do that that would be awesome.

Comment 21

[Justin Wood (:Callek)]

(In reply to comment #20)
> I don't have a blog on planet so if dholbert, ted, or Callek want to do that
> that would be awesome.

I don't have the time to write up a post Kyle, but if you write something up I'll post it by proxy for you.

Either write here, or in e-mail to me ([e-mail]) and I'll get it up to planet within 24hours max for you.

Comment 22

[[e-mail]]

This was only done in the main configure; js/src, nsprpub and comm-central configure presumably still define DEBUG_<username>.

All four configures also still search for whoami.

Comment 23

[Daniel Holbert [:dholbert]]

Ah, good catch.  Reopening since it's not completely fixed yet, then.

khuey, would you mind nixing the same chunk of code in those other configure files?

Comment 24

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Kill it with fire

This hits everything in m-c that Neil mentioned except the NSPR check for whoami, because NSPR is doing something with that at http://mxr.mozilla.org/mozilla-central/source/nsprpub/configure#1177 that I don't understand.  This is percolating through tryserver now.

If js/src needs --with-debug-label we should reopen and fix the other bug as well before we land this patch.

Comment 25

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: c-c version

c-c patch.

Comment 26

[Justin Wood (:Callek)]

(In reply to comment #24)
> Created an attachment (id=449534) [details]
> Kill it with fire
> 
> If js/src needs --with-debug-label we should reopen and fix the other bug as
> well before we land this patch.

I think we should add that to js/src as well. I'll write up a patch on that other bug in next few days. (Feel free to reopen before I do if you want).

Comment 27

[(not currently active) Ted Mielczarek]

Comment on attachment 449534 [details] [diff] [review]
Kill it with fire

The NSPR part would have to be separately landed in NSPR CVS. I'm not sure that it matters anyway, given that we're not going to use warnings as errors there.

Comment 28

[Brendan Eich [:brendan]]

I just lost part of a day due to the failure to remove DEBUG_$(USER{,NAME}) from js/src/configure.in. Grump.

/be

Comment 29

[Brendan Eich [:brendan]]

How does one add -DDEBUG_brendan now, via make cmd-line var-setting or envar, all good but I don't know the magic any longer. XCFLAGS is dead.

/be

Comment 30

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

--with-debug-label=brendan should work on the non-js/src parts of the tree.  I haven't landed the js/src part yet so it should still be defined there automatically.

Comment 31

[Brendan Eich [:brendan]]

(In reply to comment #30)
> --with-debug-label=brendan should work on the non-js/src parts of the tree.  I
> haven't landed the js/src part yet so it should still be defined there
> automatically.

Yes, and there are #ifdef DEBUG_brendaneich around instrumentation-only struct members, so some code saw offsets for later members that were displaced by those members, while other code (not living in js/src directly) saw no instrumentation members and wrong offsets. Bad times, and wasted (mostly machine, some human) cycles scratching heads and debugging.

Those ifdefs are perhaps ill-advised, maybe they should just be ifdef DEBUG, but the instrumentation costs, chiefly in dumping to an envar-named file.

Anyway, this worked for years, and is now broken. I'm likely to fix it in tracemonkey at least, right away -- force majeure.

/be

Comment 32

[Brendan Eich [:brendan]]

My point is, personal problems aside, this kind of change needs to be all or nothing. Maybe that was made already in this bug, but I wanted to emphasize it.

/be

Comment 33

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to comment #32)
> My point is, personal problems aside, this kind of change needs to be all or
> nothing. Maybe that was made already in this bug, but I wanted to emphasize it.
> 
> /be

Yes, I missed part of it the first time around.  My apologies for the trouble.

Comment 34

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Landed as http://hg.mozilla.org/mozilla-central/rev/23f1170df612

Leaving open for now for the c-c piece.

Comment 35

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Landed on c-c

http://hg.mozilla.org/comm-central/rev/489045800586

I don't care about NSPR.  Marking fixed.

Comment 36

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/2b17dcd5cba6