Closed
Bug 561674
Opened 15 years ago
Closed 15 years ago
Stop defining DEBUG_<username>
Categories
(Firefox Build System :: General, enhancement)
Firefox Build System
General
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla1.9.3a5
People
(Reporter: khuey, Assigned: khuey)
References
Details
Attachments
(3 files)
(deleted),
patch
|
ted
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
ted
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Callek
:
review+
|
Details | Diff | Splinter Review |
Assignee | ||
Comment 1•15 years ago
|
||
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•15 years ago
|
||
(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).
Assignee | ||
Comment 3•15 years ago
|
||
Attachment #441628 -
Flags: review?(ted.mielczarek)
Updated•15 years ago
|
Blocks: FAIL_ON_WARNINGS
Updated•15 years ago
|
Attachment #441628 -
Flags: review?(ted.mielczarek) → review+
Assignee | ||
Comment 5•15 years ago
|
||
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.
Assignee | ||
Comment 6•15 years ago
|
||
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a5
Assignee | ||
Comment 7•15 years ago
|
||
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.
Comment 9•15 years ago
|
||
(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•15 years ago
|
||
(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?
Assignee | ||
Comment 11•15 years ago
|
||
(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•15 years ago
|
||
(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?
Assignee | ||
Comment 13•15 years ago
|
||
(In reply to comment #12)
> So, IIUC, this patch should be fine to land again, right?
AIUI yes.
Comment 14•15 years ago
|
||
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
Comment 15•15 years ago
|
||
Filed bug 565191 on --with-debug-label (or whatever we want to call it).
Assignee | ||
Comment 16•15 years ago
|
||
Status: REOPENED → RESOLVED
Closed: 15 years ago → 15 years ago
Resolution: --- → FIXED
Comment 17•15 years ago
|
||
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.
Assignee | ||
Comment 18•15 years ago
|
||
Backed out again. Third time is the charm, right ;-)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 19•15 years ago
|
||
(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.
Assignee | ||
Comment 20•15 years ago
|
||
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: 15 years ago → 15 years ago
Resolution: --- → FIXED
Comment 21•15 years ago
|
||
(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.
Comment 22•15 years ago
|
||
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•15 years ago
|
||
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 → ---
Assignee | ||
Comment 24•15 years ago
|
||
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)
Comment 26•15 years ago
|
||
(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).
Updated•15 years ago
|
Attachment #449535 -
Flags: review?(bugspam.Callek) → review+
Comment 27•15 years ago
|
||
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+
Comment 28•15 years ago
|
||
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•15 years ago
|
||
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
Assignee | ||
Comment 30•15 years ago
|
||
--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•15 years ago
|
||
(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•15 years ago
|
||
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
Assignee | ||
Comment 33•15 years ago
|
||
(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.
Assignee | ||
Comment 34•15 years ago
|
||
Landed as http://hg.mozilla.org/mozilla-central/rev/23f1170df612
Leaving open for now for the c-c piece.
Assignee | ||
Comment 35•15 years ago
|
||
Landed on c-c
http://hg.mozilla.org/comm-central/rev/489045800586
I don't care about NSPR. Marking fixed.
Status: REOPENED → RESOLVED
Closed: 15 years ago → 15 years ago
Resolution: --- → FIXED
Updated•7 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•