1309505 - Add -Werror to gyp builds

Status: RESOLVED FIXED

(NSS :: Build, defect)

Description

[Franziskus Kiefer [:franziskus]]

All warnings must be errors in gyp builds as well.

Comment 1

[Franziskus Kiefer [:franziskus]]

Ted, can you point us into the right direction here? Or are you taking this on?

Comment 2

[(not currently active) Ted Mielczarek]

I am not likely to get to this right away. If you'd like to try fixing it, you probably want to do something like:
1) Add an "enable_werror" variable in the variables section of config.gypi: https://hg.mozilla.org/projects/nss/file/tip/coreconf/config.gypi#l84 It looks like this defaults to on on everything but Android right now? If that's the case you might need to put a condition in the previous section to set it per-platform: https://hg.mozilla.org/projects/nss/file/tip/coreconf/config.gypi#l20 and then copy it out to the higher scope like 'enable_werror%': '<(enable_werror)',
2) For each platform you will need to add a condition somewhere on '<(enable_werror)==1' to set the appropriate flags. Linux could go here:
https://hg.mozilla.org/projects/nss/file/tip/coreconf/config.gypi#l240
OS X could go here:
https://hg.mozilla.org/projects/nss/file/tip/coreconf/config.gypi#l217 (and it looks like I left a note as to what is needed, something like https://dxr.mozilla.org/mozilla-beta/source/media/webrtc/trunk/build/common.gypi#1881 )
Windows could go here:
https://hg.mozilla.org/projects/nss/file/tip/coreconf/config.gypi#l204

and it would use something like this:
https://github.com/springmeyer/gyp/blob/master/test/win/compiler-flags/warning-level.gyp#L14

Comment 3

[Franziskus Kiefer [:franziskus]]

Attachment: gyp-werror.patch

So this works for c files on Linux (and I guess mac).
I don't think we need an extra variable as we want Werror always enabled when possible (so only doing an exception for Android for now).

What I noticed is that ninja doesn't seem to pick up changes in the external_tests directory and doesn't pass on cflags_cc to the cpp code in there. I don't see anything in the gyp files that would explain this.

Comment 4

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8802014 [details] [diff] [review]
gyp-werror.patch

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

::: coreconf/config.gypi
@@ +263,5 @@
>                'ANDROID',
>              ],
>            }],
>            [ 'OS=="mac"', {
> +            'cflags': [

Looking at your comment again I guess this isn't working. I'll have to test on a mac.

Comment 5

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8802014 [details] [diff] [review]
gyp-werror.patch

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

The cpp problems came from missing files in the ssl_gtest.gyp. Let me have another look at this.

Comment 6

[Martin Thomson [:mt:]]

https://hg.mozilla.org/projects/nss/rev/a1d2234bed689864b7b0007e5d422f2aef89f453

Comment 7

[Martin Thomson [:mt:]]

Recursive make was screwing up:
https://hg.mozilla.org/projects/nss/rev/cbeabbf0fb44ae6341996a91d51d20ccf53b5ff0