Last Comment Bug 716544 - Use --enable-warnings-as-errors for Android builds
: Use --enable-warnings-as-errors for Android builds
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Build Config (show other bugs)
: Trunk
: All Android
: -- normal (vote)
: mozilla14
Assigned To: Mike Hommey [:glandium]
:
: Gregory Szorc [:gps]
Mentors:
Depends on: 736220
Blocks: 703121 734050
  Show dependency treegraph
 
Reported: 2012-01-09 07:38 PST by Mounir Lamouri (:mounir)
Modified: 2012-03-15 14:48 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
PoC (5.32 KB, patch)
2012-01-09 08:30 PST, Mike Hommey [:glandium]
no flags Details | Diff | Splinter Review
Define android NDK headers as system headers (10.64 KB, patch)
2012-02-21 01:44 PST, Mike Hommey [:glandium]
khuey: review+
Details | Diff | Splinter Review
Only define android bionic headers as system headers, and only use stlport includes when compiling C++ (9.04 KB, patch)
2012-03-05 02:06 PST, Mike Hommey [:glandium]
khuey: review+
Details | Diff | Splinter Review
Include assert.h from logging.h (1.68 KB, patch)
2012-03-05 02:12 PST, Mike Hommey [:glandium]
cjones.bugs: review+
Details | Diff | Splinter Review
Bug - Include memory.h from string_conversion.cc (1.26 KB, patch)
2012-03-05 02:17 PST, Mike Hommey [:glandium]
no flags Details | Diff | Splinter Review
Fix comparison between signed and unsigned integer expressions in nsStringAPI.h (1.63 KB, patch)
2012-03-06 00:03 PST, Mike Hommey [:glandium]
dbaron: review+
Details | Diff | Splinter Review
Enable warnings as errors on mobile (3.07 KB, patch)
2012-03-06 00:04 PST, Mike Hommey [:glandium]
khuey: review+
Details | Diff | Splinter Review
Avoid unexpectedly including toolkit/crashreporter/google-breakpad/src/common/memory.h (981 bytes, patch)
2012-03-07 06:59 PST, Mike Hommey [:glandium]
ted: review+
Details | Diff | Splinter Review
Avoid unexpectedly including toolkit/crashreporter/google-breakpad/src/common/memory.h. (1006 bytes, patch)
2012-03-13 05:50 PDT, Mike Hommey [:glandium]
ted: review+
Details | Diff | Splinter Review

Description Mounir Lamouri (:mounir) 2012-01-09 07:38:32 PST
Glandium said we could try to do this by ignoring the warnings that pop (or not fail on it at least).
Comment 1 Mike Hommey [:glandium] 2012-01-09 08:06:38 PST
I said we could try to ignore warnings in system headers (which is what the bulk of warnings comes from).
Comment 2 Mike Hommey [:glandium] 2012-01-09 08:25:47 PST
So, in fact, system headers are normally ignored by default. But we don't include android headers as system headers.
Comment 3 Mike Hommey [:glandium] 2012-01-09 08:30:49 PST
Created attachment 587004 [details] [diff] [review]
PoC

Something like this should work. I leave it to you to do the same in js/src and maybe nsprpub. There may be some tweeks in security/manager too.

(Note this patch removes the include from CFLAGS and CXXFLAGS because in practice, having it in CPPFLAGS will put it on all command lines where CFLAGS and CXXFLAGS are used)
Comment 4 Mike Hommey [:glandium] 2012-02-21 01:44:32 PST
Created attachment 599069 [details] [diff] [review]
Define android NDK headers as system headers
Comment 5 Mike Hommey [:glandium] 2012-02-26 23:29:12 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/14f43a9a1ecf

This should stay open until we actually enable warnings as errors. This landing reduces the number of warnings significantly.
Comment 6 Marco Bonardo [::mak] 2012-02-27 04:56:34 PST
https://hg.mozilla.org/mozilla-central/rev/14f43a9a1ecf
Comment 7 Mike Hommey [:glandium] 2012-03-05 02:06:18 PST
Created attachment 602832 [details] [diff] [review]
Only define android bionic headers as system headers, and only use stlport includes when compiling C++

This applies on top of bug 730968. I verified this doesn't break b2g, besides two things I'll attach separately. This also fixes building mobile with the arm-eabi toolchain (instead of arm-linux-androideabi).
Comment 8 Mike Hommey [:glandium] 2012-03-05 02:12:41 PST
Created attachment 602833 [details] [diff] [review]
Include assert.h from logging.h

stlport's assert.h #error's out when assert is already defined. I don't know why this doesn't happen unless building b2g with the patches from this bug applied, but since logging.h is fiddling with assert.h functionality, it's safer to include assert.h before doing so, to avoid anything included after logging.h pulling it and triggering the #error. Some other header also defines ERROR, which clashes with the definition from logging.h.
Comment 9 Mike Hommey [:glandium] 2012-03-05 02:17:34 PST
Created attachment 602834 [details] [diff] [review]
Bug  - Include memory.h from string_conversion.cc

Sent to upstream review:
http://breakpad.appspot.com/356001
Comment 10 Mike Hommey [:glandium] 2012-03-06 00:03:46 PST
Created attachment 603177 [details] [diff] [review]
Fix comparison between signed and unsigned integer expressions in nsStringAPI.h
Comment 11 Mike Hommey [:glandium] 2012-03-06 00:04:45 PST
Created attachment 603179 [details] [diff] [review]
Enable warnings as errors on mobile
Comment 12 David Baron :dbaron: ⌚️UTC-10 2012-03-06 09:48:39 PST
Comment on attachment 603177 [details] [diff] [review]
Fix comparison between signed and unsigned integer expressions in nsStringAPI.h

r=dbaron
Comment 13 Mike Hommey [:glandium] 2012-03-07 06:59:07 PST
Created attachment 603705 [details] [diff] [review]
Avoid unexpectedly including toolkit/crashreporter/google-breakpad/src/common/memory.h

So the fundamental problem is that we're not building breakpad as upstream does. Upstream has a single makefile that does everyting, while we have a Makefile in many subdirectories src/common. That makes us effectively -Isrc/common and possibly include memory.h from there when system headers include it, while this is not intended. As a matter of fact, upstream only does -Isrc, and memory.h is included with "common/memory.h" when it needs to be included.
Comment 14 Ted Mielczarek [:ted.mielczarek] 2012-03-12 10:51:26 PDT
Comment on attachment 603705 [details] [diff] [review]
Avoid unexpectedly including toolkit/crashreporter/google-breakpad/src/common/memory.h

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

Thanks. I should probably rework these hand-crafted Breakpad Makefiles, they're kind of crappy.
Comment 16 Mike Hommey [:glandium] 2012-03-13 02:35:13 PDT
Backed out everything because of Linux red:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f7ec55cf0e95
Comment 17 Mike Hommey [:glandium] 2012-03-13 05:50:39 PDT
Created attachment 605361 [details] [diff] [review]
Avoid unexpectedly including toolkit/crashreporter/google-breakpad/src/common/memory.h.

$(DIST)/include is needed for stl wrappers including mozilla/mozalloc.h

Note You need to log in before you can comment on or make changes to this bug.