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

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Daniel Holbert [:dholbert]]

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.)

Comment 1

[Daniel Holbert [:dholbert]]

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

Comment 2

[Justin Wood (:Callek)]

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.

Comment 3

[Daniel Holbert [:dholbert]]

(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.

Comment 4

[Daniel Holbert [:dholbert]]

(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.)

Comment 5

[(not currently active) Ted Mielczarek]

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.

Comment 6

[Justin Wood (:Callek)]

Attachment: v1

First Attempt, this should do it, but have not yet tested.

Comment 7

[Daniel Holbert [: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)

Comment 8

[Justin Wood (:Callek)]

Comment on attachment 446273 [details] [diff] [review]
v1

Seems to work well here...

Comment 9

[Justin Wood (:Callek)]

(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 10

[Justin Wood (:Callek)]

Comment on attachment 446273 [details] [diff] [review]
v1

Actually I included too much context here... Accidentally included the changes that drop this being automatic.

Comment 11

[Justin Wood (:Callek)]

Attachment: v2

Comment 12

[(not currently active) Ted Mielczarek]

Comment on attachment 446388 [details] [diff] [review]
v2

># HG changeset patch
># User Justin Wood <[e-mail]>
># 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.

Comment 13

[Daniel Holbert [:dholbert]]

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

Comment 14

[Daniel Holbert [:dholbert]]

http://hg.mozilla.org/mozilla-central/rev/42f90d3591da

Comment 15

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

Reopening to hit js/src as well.

Comment 16

[Justin Wood (:Callek)]

Attachment: v1, in js/src

Meant to be applied on tracemonkey and pushed there.

Comment 17

[Justin Wood (:Callek)]

Attachment: c-c version

Comment 18

[Justin Wood (:Callek)]

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.

Comment 19

[Justin Wood (:Callek)]

http://hg.mozilla.org/comm-central/rev/80ac4595205d

Still waiting on tracemonkey review/landing

Comment 20

[(not currently active) Ted Mielczarek]

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.

Comment 21

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

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.

Comment 22

[(not currently active) Ted Mielczarek]

Comment on attachment 450036 [details] [diff] [review]
v1, in js/src

Oh. well that was confusing.

Comment 23

[Justin Wood (:Callek)]

(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.

Comment 24

[Justin Wood (:Callek)]

Not sure if I will get around to landing this within the week, so checkin-needed for now.

Comment 25

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

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

Leaving open until c-c push

Comment 26

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

This has apparently already landed on c-c