Closed Bug 938505 Opened 6 years ago Closed 6 years ago

--enable-trace-malloc and --enable-dmd should fail at configure time

Categories

(Firefox Build System :: General, defect)

x86
Windows 7
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla28

People

(Reporter: cbook, Assigned: sylvestre)

Details

Attachments

(1 file, 3 obsolete files)

Tried to build a with DMD enabled and failed using a mozconfig with

ac_add_options --enable-trace-malloc

According to glandium this is expected:

23:24 < glandium> Tomcat|sheriffduty: i don't think trace malloc and replace malloc work together
23:24 < glandium> we should fail at configure time


So yeah we should make 
ac_add_options --enable-trace-malloc
and
ac_add_options --enable-dmd

at the same time make failing at configure
Attached patch fix-bug-938505.diff (obsolete) — Splinter Review
The patch replaces the transparent desactivation of DMD by trace-malloc by a failure.
Attachment #832275 - Flags: review?(mh+mozilla)
Comment on attachment 832275 [details] [diff] [review]
fix-bug-938505.diff

Review of attachment 832275 [details] [diff] [review]:
-----------------------------------------------------------------

::: configure.in
@@ +6768,5 @@
>      MOZ_DMD= )
>  
> +
> +dnl The two options are conflicting. Fails the configure to alert the user.
> +if test "$NS_TRACE_MALLOC" -a "$MOZ_DMD"; then

Make that MOZ_REPLACE_MALLOC instead of MOZ_DMD. dmd requires replace malloc, and it's replace malloc that conflicts with trace malloc.

(And in fact, configure.in already disables dmd when trace malloc is on. It would be better to AC_MSG_ERROR instead.)
Attachment #832275 - Flags: review?(mh+mozilla) → review-
Attached patch fix-bug-938505.diff (obsolete) — Splinter Review
Taking in account Mike's comments.
Attachment #832275 - Attachment is obsolete: true
Attachment #832843 - Flags: review?(mh+mozilla)
Comment on attachment 832843 [details] [diff] [review]
fix-bug-938505.diff

Review of attachment 832843 [details] [diff] [review]:
-----------------------------------------------------------------

::: configure.in
@@ +6834,5 @@
>  
> +dnl The two options are conflicting. Fails the configure to alert the user.
> +if test "$NS_TRACE_MALLOC" -a "$MOZ_REPLACE_MALLOC"; then
> +    AC_MSG_ERROR([--enable-trace-malloc and --enable-replace-malloc are conflicting options])
> +fi

While you're here, can you make --enable-trace-malloc --enable-dmd error out instead of siliently disabling dmd?
Attachment #832843 - Flags: review?(mh+mozilla) → review+
Attached patch fix-bug-938505.diff (obsolete) — Splinter Review
Here it is! (I think it is was I have done in the first commit).

I did that in a single commit, don't hesitate if you want me to split it in two.
Attachment #832843 - Attachment is obsolete: true
Attachment #833319 - Flags: review?(mh+mozilla)
Attachment #833319 - Flags: checkin?
Comment on attachment 833319 [details] [diff] [review]
fix-bug-938505.diff

Review of attachment 833319 [details] [diff] [review]:
-----------------------------------------------------------------

::: configure.in
@@ +6768,5 @@
>      MOZ_DMD= )
>  
> +dnl The two options are conflicting. Fails the configure to alert the user.
> +if test "$NS_TRACE_MALLOC" -a "$MOZ_DMD"; then
> +    AC_ERROR("--enable-trace-malloc and --enable-dmd are conflicting options")

AC_MSG_ERROR.
Attachment #833319 - Flags: review?(mh+mozilla) → review+
Sorry, fixed.
Attachment #833319 - Attachment is obsolete: true
Attachment #833319 - Flags: checkin?
Attachment #833322 - Flags: review?(mh+mozilla)
Comment on attachment 833322 [details] [diff] [review]
fix-bug-938505.diff

Review of attachment 833322 [details] [diff] [review]:
-----------------------------------------------------------------

No need to rerequest review when you were r+ed with trivial changed asked.
Attachment #833322 - Flags: review?(mh+mozilla) → review+
Attachment #833322 - Flags: checkin?
Comment on attachment 833322 [details] [diff] [review]
fix-bug-938505.diff

Please just use the checkin-needed bug keyword in the future :)
Attachment #833322 - Flags: checkin? → checkin+
https://hg.mozilla.org/mozilla-central/rev/01c2d7fb5e71
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.