The default bug view has changed. See this FAQ.

Use --enable-warnings-as-errors for Android builds

RESOLVED FIXED in mozilla14

Status

()

Core
Build Config
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mounir, Assigned: glandium)

Tracking

Trunk
mozilla14
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments, 3 obsolete attachments)

(Reporter)

Description

5 years ago
Glandium said we could try to do this by ignoring the warnings that pop (or not fail on it at least).
(Assignee)

Comment 1

5 years ago
I said we could try to ignore warnings in system headers (which is what the bulk of warnings comes from).
(Assignee)

Comment 2

5 years ago
So, in fact, system headers are normally ignored by default. But we don't include android headers as system headers.
(Assignee)

Comment 3

5 years ago
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)
(Assignee)

Comment 4

5 years ago
Created attachment 599069 [details] [diff] [review]
Define android NDK headers as system headers
Attachment #599069 - Flags: review?(khuey)
(Assignee)

Updated

5 years ago
Attachment #587004 - Attachment is obsolete: true
Attachment #587004 - Attachment is patch: true
Attachment #599069 - Flags: review?(khuey) → review+
(Assignee)

Updated

5 years ago
Assignee: nobody → mh+mozilla
(Assignee)

Comment 5

5 years ago
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.
Whiteboard: [leave open after inbound merge]
https://hg.mozilla.org/mozilla-central/rev/14f43a9a1ecf
Whiteboard: [leave open after inbound merge]
(Assignee)

Comment 7

5 years ago
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).
Attachment #602832 - Flags: review?(khuey)
(Assignee)

Comment 8

5 years ago
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.
Attachment #602833 - Flags: review?(jones.chris.g)
(Assignee)

Comment 9

5 years ago
Created attachment 602834 [details] [diff] [review]
Bug  - Include memory.h from string_conversion.cc

Sent to upstream review:
http://breakpad.appspot.com/356001
Attachment #602833 - Flags: review?(jones.chris.g) → review+
(Assignee)

Comment 10

5 years ago
Created attachment 603177 [details] [diff] [review]
Fix comparison between signed and unsigned integer expressions in nsStringAPI.h
Attachment #603177 - Flags: review?(dbaron)
(Assignee)

Comment 11

5 years ago
Created attachment 603179 [details] [diff] [review]
Enable warnings as errors on mobile
Attachment #603179 - Flags: review?(khuey)
Attachment #603179 - Flags: review?(khuey) → review+
Attachment #602832 - Flags: review?(khuey) → review+
Comment on attachment 603177 [details] [diff] [review]
Fix comparison between signed and unsigned integer expressions in nsStringAPI.h

r=dbaron
Attachment #603177 - Flags: review?(dbaron) → review+
(Assignee)

Comment 13

5 years ago
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.
Attachment #603705 - Flags: review?(ted.mielczarek)
(Assignee)

Updated

5 years ago
Attachment #602834 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Blocks: 734050
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.
Attachment #603705 - Flags: review?(ted.mielczarek) → review+
(Assignee)

Comment 15

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/923a278f7ad5
https://hg.mozilla.org/integration/mozilla-inbound/rev/8f0164c90145
https://hg.mozilla.org/integration/mozilla-inbound/rev/f5171db26f48
https://hg.mozilla.org/integration/mozilla-inbound/rev/7b590e1e392c
https://hg.mozilla.org/integration/mozilla-inbound/rev/bd9a2f904e1f
(Assignee)

Comment 16

5 years ago
Backed out everything because of Linux red:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f7ec55cf0e95
(Assignee)

Comment 17

5 years ago
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
Attachment #605361 - Flags: review?(ted.mielczarek)
(Assignee)

Updated

5 years ago
Attachment #603705 - Attachment is obsolete: true
Attachment #605361 - Flags: review?(ted.mielczarek) → review+
(Assignee)

Comment 18

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/4fd91573f494
https://hg.mozilla.org/integration/mozilla-inbound/rev/27afd563db7b
https://hg.mozilla.org/integration/mozilla-inbound/rev/012028b5efd9
https://hg.mozilla.org/integration/mozilla-inbound/rev/cbc1ba56dbde
https://hg.mozilla.org/integration/mozilla-inbound/rev/10123c7f98e9
https://hg.mozilla.org/mozilla-central/rev/4fd91573f494
https://hg.mozilla.org/mozilla-central/rev/27afd563db7b
https://hg.mozilla.org/mozilla-central/rev/012028b5efd9
https://hg.mozilla.org/mozilla-central/rev/cbc1ba56dbde
https://hg.mozilla.org/mozilla-central/rev/10123c7f98e9
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14
Depends on: 736220
You need to log in before you can comment on or make changes to this bug.