Closed Bug 291088 Opened 15 years ago Closed 12 years ago

bloat test files should not be included in optimized builds or release builds

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla1.9beta3

People

(Reporter: Bugzilla-alanjstrBugs, Assigned: RyanVM)

References

Details

Attachments

(1 file, 3 obsolete files)

http://lxr.mozilla.org/aviary101branch/source/build/Makefile.in#39

Firefox builds contain extra stuff in the /res/ directory that shouldn't be
there.  In this case, bloattest
Summary: http://lxr.mozilla.org/aviary101branch/source/build/Makefile.in#39 → bloat test files should not be included in optimized builds or release builds
What if I submitted a patch which didn't package these files if --disable-tests is set? bsmedberg, would that be acceptable?
Attached patch Like this (obsolete) — Splinter Review
Does this need sr too?
Attachment #278200 - Flags: review?
Attachment #278200 - Flags: review? → review?(benjamin)
Comment on attachment 278200 [details] [diff] [review]
Like this

You want "ifdef ENABLE_TESTS", but yes this looks correct. I'd like dbaron to confirm that this is ok
Attachment #278200 - Flags: superreview?(dbaron)
Attachment #278200 - Flags: review?(benjamin)
Attachment #278200 - Flags: review+
Same patch, new ifdef. Carrying over bsmedberg's earlier r+ and dbaron's sr request.
Attachment #278200 - Attachment is obsolete: true
Attachment #278885 - Flags: superreview?
Attachment #278885 - Flags: review+
Attachment #278200 - Flags: superreview?(dbaron)
Attachment #278885 - Flags: superreview? → superreview?(dbaron)
It would be nice if we could ifdef based on NS_BUILD_REFCNT_LOGGING, but that's not defined for makefiles and it's a little work to change.  Have you at least checked that all the tinderboxes running leak tests are not building with --disable-tests ?
hmm, good point. I checked fxdbug-linux-tbox, bm-xserve11, and nye and none of them have tests enabled. This patch isn't going work.
Attachment #278885 - Attachment is obsolete: true
Attachment #278885 - Flags: superreview?(dbaron)
dbaron, how does this work for you? As far as I can tell, this is the same check which is run when setting NS_BUILD_REFCNT_LOGGING in nsTraceRefcnt.h and it allows me to avoid resorting to configure hackery.
Assignee: mpgritti → ryanvm
Status: NEW → ASSIGNED
Attachment #295518 - Flags: review?
Attachment #295518 - Flags: review? → review?(dbaron)
The check should be for _ENABLE_LOGREFCNT I guess.
Attachment #295518 - Attachment is obsolete: true
Attachment #295519 - Flags: review?(dbaron)
Attachment #295518 - Flags: review?(dbaron)
Comment on attachment 295519 [details] [diff] [review]
Check for MOZ_DEBUG or _ENABLE_LOGREFCNT

I don't see any variables that are defined in makefiles based on --enable-logrefcnt.  If you wanted one, you'd need to add a line to autoconf.mk.in to match the currently unused AC_SUBST in configure.in for MOZ_LOG_REFCNT.

But it might be more appropriate to just make these check ENABLE_TESTS.  They're not really specific to refcount logging -- they're general tests that can be used with any type of logging.  (But we'd need to check that the necessary tinderboxes had tests enabled.)
Attachment #295519 - Flags: review?(dbaron) → review-
As pointed out in comment #6, none of the debug tinderboxen have tests enabled AFAICT.
"tests enabled" == "doesn't use --disable-tests" (since tests are on by default)
Attachment #295519 - Attachment is obsolete: true
Comment on attachment 278885 [details] [diff] [review]
Proper ifdef action

Unobsoleting per comment #9 and comment #11. I've checked, and none of the Tinderbox debug machines have --disable-tests set, so I guess we're in the clear.

This already had r+, so asking for approval1.9.
Attachment #278885 - Attachment is obsolete: false
Attachment #278885 - Flags: approval1.9?
Comment on attachment 278885 [details] [diff] [review]
Proper ifdef action

a=beltzner for 1.9
Attachment #278885 - Flags: approval1.9? → approval1.9+
Keywords: checkin-needed
Assignee: ryanvm → nobody
Status: ASSIGNED → NEW
Component: Embedding: GTK Widget → Build Config
OS: Windows 2000 → All
QA Contact: pavlov → build-config
Hardware: PC → All
Assignee: nobody → ryanvm
Status: NEW → ASSIGNED
Checking in build/Makefile.in;
/cvsroot/mozilla/build/Makefile.in,v  <--  Makefile.in
new revision: 1.20; previous revision: 1.19
done
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9 M11
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.