Closed Bug 565191 Opened 10 years ago Closed 10 years ago

Add build option "--with-debug-label"

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: dholbert, Assigned: Callek)

References

Details

Attachments

(3 files, 1 obsolete file)

Before we land Bug 561674 and stop auto-defining DEBUG_<person>, it'd be nice to provide an easy way for people to specify (in their .mozconfig files) that they want DEBUG_<person> defined.

This bug is on adding a mozconfig variable called something like
  --with-debug-label
or perhaps
  --with-debug-username

(I prefer "label", since the values shouldn't necessarily be usernames, though they mostly would be right now.)

Requirements:
 * In --disable-debug builds, this variable should have no effect
 * In --enable-debug builds, this flag should take a comma-separated* list of labels. For example:
        ac_add_options --with-debug-label=foo,bar
   would define DEBUG_foo and DEBUG_bar.

*Note that there's precedent for comma-separated mozconfig list values in the "--enable-extensions" configuration option.  (Chime in if there's a better way to specify multiple values, though.)
Blocks: 561674
Note that ted linked to a way to do this, on the newsgroup thread:
  http://groups.google.com/group/mozilla.dev.platform/msg/75081beda79abc35
>   ac_add_options --enable-debug="-g -DDEBUG_zack" --disable-optimize
but, for avoiding typos and improving readability, it'd be nicer to support:
>   ac_add_options --enable-debug --disable-optimize
>   ac_add_options --with-debug-label=zack
Ted, do you have a preference on the option, my personal pref is --with-debug-label as daniel suggests.
Also do you want the "comma separated" as an option, or only one label supported this way, or "other"?

I'm willing to do this up if no-one else has started or has time.
(In reply to comment #2)
> I'm willing to do this up if no-one else has started or has time.

I haven't started, and I was hoping someone else would volunteer to take it. :)  So, all yours if you'd like it.
(In reply to comment #2)
> Also do you want the "comma separated" as an option, or only one label
> supported this way, or "other"?

(IMHO "only one label supported" would not be a good path -- as long as we're doing this, we should support multiple labels at a time (e.g. DEBUG_dbaron and DEBUG_dholbert both simultaneously defined), since that makes this much more powerful & useful.  I don't particularly care whether it's "--with-debug-label=dholbert,dbaron" or some other syntax, as long as it's concise.)
Eh, I don't even like DEBUG_foo stuff, so do whatever is easiest. I'll r+ it because clearly other people find it useful.
Assignee: nobody → bugspam.Callek
Attached patch v1 (obsolete) — Splinter Review
First Attempt, this should do it, but have not yet tested.
Attachment #446273 - Flags: feedback?(dholbert)
Comment on attachment 446273 [details] [diff] [review]
v1

>+MOZ_ARG_WITH_STRING(debug-label,
>+[  --with-debug-label[=LABELS]

Looks good to me -- I'd remove the square brackets around "=LABELS", though.  The brackets imply (to me anyway) that =LABELS is optional & that the option still does something useful if you omit =LABELS. (Which it doesn't)
Attachment #446273 - Flags: feedback?(dholbert) → feedback+
Comment on attachment 446273 [details] [diff] [review]
v1

Seems to work well here...
Attachment #446273 - Flags: review?(ted.mielczarek)
(In reply to comment #7)
> (From update of attachment 446273 [details] [diff] [review])
> >+MOZ_ARG_WITH_STRING(debug-label,
> >+[  --with-debug-label[=LABELS]
> 
> Looks good to me -- I'd remove the square brackets around "=LABELS", though. 

Fixed locally.
Comment on attachment 446273 [details] [diff] [review]
v1

Actually I included too much context here... Accidentally included the changes that drop this being automatic.
Attachment #446273 - Flags: review?(ted.mielczarek)
Attached patch v2Splinter Review
Attachment #446273 - Attachment is obsolete: true
Attachment #446388 - Flags: review?(ted.mielczarek)
Comment on attachment 446388 [details] [diff] [review]
v2

># HG changeset patch
># User Justin Wood <Callek@gmail.com>
># Date 1274301431 14400
># Node ID d062394a52a78e2cf607d0d82e64c75a6b16aa6f
># Parent  64e1cfd8ba01945a5f2dd2fc6d98dd2c5455068e
>Bug 565191 - Add build option "--with-debug-label"
>
>diff --git a/configure.in b/configure.in
>--- a/configure.in
>+++ b/configure.in
>@@ -6935,6 +6935,14 @@ MOZ_ARG_ENABLE_STRING(debug,
>   MOZ_DEBUG=)
> 
> MOZ_DEBUG_ENABLE_DEFS="-DDEBUG -D_DEBUG"
>+MOZ_ARG_WITH_STRING(debug-label,
>+[  --with-debug-label=LABELS
>+                           Enabled the use of DEBUG_label ifdefs
>+                           (comma separated)],


Please make this help text more readable. Perhaps something like "Define DEBUG_<value> for each comma-separated value given."

r=me with that fixed.
Attachment #446388 - Flags: review?(ted.mielczarek) → review+
Status: NEW → ASSIGNED
Callek said on IRC that this is mildly checkin-needed, since he won't be able to land this himself for a few days.

So, I've imported the patch into my patch-queue, with comment 12 addressed (using ted's suggested text), and I intend to landing it later today.
http://hg.mozilla.org/users/dholbert_mozilla.com/simple-patches/file/9f4363a355aa/565191-fix.patch
http://hg.mozilla.org/mozilla-central/rev/42f90d3591da
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Reopening to hit js/src as well.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch v1, in js/srcSplinter Review
Meant to be applied on tracemonkey and pushed there.
Attachment #450036 - Flags: review?(ted.mielczarek)
Attached patch c-c versionSplinter Review
Attachment #450038 - Flags: review?(kairo)
Comment on attachment 450038 [details] [diff] [review]
c-c version

I'm also willing to accept Ted's review here instead of Roberts, if ted is faster.
Attachment #450038 - Flags: review?(ted.mielczarek)
Attachment #450038 - Flags: review?(ted.mielczarek)
Attachment #450038 - Flags: review?(kairo)
Attachment #450038 - Flags: review+
http://hg.mozilla.org/comm-central/rev/80ac4595205d

Still waiting on tracemonkey review/landing
Comment on attachment 450036 [details] [diff] [review]
v1, in js/src

You don't need separate review from me to land on a separate branch. Also, generally tracemonkey just gets its changes by merging them from mozilla-central. Why would we land this patch there separately? AFAIK there's not anything that's going to break on tm if we don't land this patch there, and they'll get it when they merge anyway.
Attachment #450036 - Flags: review?(ted.mielczarek)
Comment on attachment 450036 [details] [diff] [review]
v1, in js/src

What he means is that this applies to js/src/configure.in, which got missed before.
Attachment #450036 - Attachment description: v1, applied on tracemonkey → v1, in js/src
Attachment #450036 - Flags: review?(ted.mielczarek)
Comment on attachment 450036 [details] [diff] [review]
v1, in js/src

Oh. well that was confusing.
Attachment #450036 - Flags: review?(ted.mielczarek) → review+
(In reply to comment #22)
> (From update of attachment 450036 [details] [diff] [review])
> Oh. well that was confusing.

Sorry; and yea, I initially meant to land on tracemonkey/js/src. Per a brief chat in #developers that patching tracemonkey was better; but I can just land to m-c if you like.
Not sure if I will get around to landing this within the week, so checkin-needed for now.
Keywords: checkin-needed
Whiteboard: [c-n attachment 450036 (js/src)]
This has apparently already landed on c-c
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Keywords: checkin-needed
Whiteboard: [c-n attachment 450036 (js/src)]
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.