Closed Bug 561674 Opened 10 years ago Closed 9 years ago

Stop defining DEBUG_<username>

Categories

(Firefox Build System :: General, enhancement)

enhancement
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla1.9.3a5

People

(Reporter: khuey, Assigned: khuey)

References

Details

Attachments

(3 files)

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.
(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).
Attached patch PatchSplinter Review
Attachment #441628 - Flags: review?(ted.mielczarek)
Attachment #441628 - Flags: review?(ted.mielczarek) → review+
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.
http://hg.mozilla.org/mozilla-central/rev/54c9fad6aa00
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a5
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).
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
(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.
(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?
(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.
(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?
(In reply to comment #12)
> So, IIUC, this patch should be fine to land again, right?

AIUI yes.
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.
Version: unspecified → Trunk
Depends on: 565191
Filed bug 565191 on --with-debug-label (or whatever we want to call it).
http://hg.mozilla.org/mozilla-central/rev/7efb41b5e57a
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
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.
Backed out again.  Third time is the charm, right ;-)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(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.
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.
Status: REOPENED → RESOLVED
Closed: 10 years ago9 years ago
Resolution: --- → FIXED
(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 (Callek@gmail.com) and I'll get it up to planet within 24hours max for you.
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.
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?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Attachment #449534 - Flags: review?(ted.mielczarek)
Attached patch c-c versionSplinter Review
c-c patch.
Attachment #449535 - Flags: review?(bugspam.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).
Attachment #449535 - Flags: review?(bugspam.Callek) → review+
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.
Attachment #449534 - Flags: review?(ted.mielczarek) → review+
I just lost part of a day due to the failure to remove DEBUG_$(USER{,NAME}) from js/src/configure.in. Grump.

/be
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
--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.
(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
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
(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.
Landed on c-c

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

I don't care about NSPR.  Marking fixed.
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.