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

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Mounir Lamouri (:mounir)]

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]]

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]]

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]]

Attachment: 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]]

Attachment: Define android NDK headers as system headers

Comment 5

[Mike Hommey [:glandium]]

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]]

https://hg.mozilla.org/mozilla-central/rev/14f43a9a1ecf

Comment 7

[Mike Hommey [:glandium]]

Attachment: 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]]

Attachment: 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]]

Attachment: Bug - Include memory.h from string_conversion.cc

Sent to upstream review:
http://breakpad.appspot.com/356001

Comment 10

[Mike Hommey [:glandium]]

Attachment: Fix comparison between signed and unsigned integer expressions in nsStringAPI.h

Comment 11

[Mike Hommey [:glandium]]

Attachment: Enable warnings as errors on mobile

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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]]

Attachment: 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

[(not currently active) Ted Mielczarek]

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 15

[Mike Hommey [:glandium]]

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

Comment 16

[Mike Hommey [:glandium]]

Backed out everything because of Linux red:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f7ec55cf0e95

Comment 17

[Mike Hommey [:glandium]]

Attachment: Avoid unexpectedly including toolkit/crashreporter/google-breakpad/src/common/memory.h.

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

Comment 18

[Mike Hommey [:glandium]]

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

Comment 19

[Marco Bonardo [:mak]]

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