linux/clang: Disable the last two warnings, turn on -Werror

linux/clang: Disable the last two warnings, turn on -Werror

BUG=138571
TBR=tony

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=152242

[Nico, 2012-08-18 00:13:09]

agl: ssl.gyp
thestig: breakpad.gyp
tony: common.gypi

[Lei Zhang, 2012-08-18 00:34:35]

lgtm

[agl, 2012-08-18 01:08:07]

(On vacation, please direct to rsleevi or wtc.)
On Aug 17, 2012 5:13 PM, <thakis@chromium.org> wrote:

> Reviewers: agl, Lei Zhang, tony,
>
> Message:
> agl: ssl.gyp
> thestig: breakpad.gyp
> tony: common.gypi
>
> Description:
> linux/clang: Disable the last two warnings, turn on -Werror
>
> BUG=138571
>
>
> Please review this at
http://codereview.chromium.**org/10855238/<http://codereview.chromium.org/108...
>
> SVN Base:
http://git.chromium.org/git/**chromium.git@trunk<http://git.chromium.org/git/...
>
> Affected files:
>   M breakpad/breakpad.gyp
>   M build/common.gypi
>   M net/third_party/nss/ssl.gyp
>
>
> Index: breakpad/breakpad.gyp
> diff --git a/breakpad/breakpad.gyp b/breakpad/breakpad.gyp
> index cbed22c90944dbac59b68b27b55e9c**7d3f7366e5..**
> a7d076712fa4e02489b69f46d70372**5356fe3f95 100644
> --- a/breakpad/breakpad.gyp
> +++ b/breakpad/breakpad.gyp
> @@ -613,6 +613,14 @@
>              '..',
>              '.',
>            ],
> +          'conditions': [
> +            [ 'clang == 1', {
> +              'cflags': [
> +                # See http://crbug.com/138571#c18
> +                '-Wno-unused-value',
> +              ],
> +            }],
> +          ],
>          },
>          {
>            'target_name': 'linux_dumper_unittest_helper'**,
> Index: build/common.gypi
> diff --git a/build/common.gypi b/build/common.gypi
> index c85107eb74eb9913dbe8b131a10b32**7d155efc70..**
> 7fd9f21b3a6f2d9c06ebb52a66cdf6**edb3a6db0b 100644
> --- a/build/common.gypi
> +++ b/build/common.gypi
> @@ -1618,7 +1618,6 @@
>              'cflags!': [
>                '-Wall',
>                '-Wextra',
> -              '-Werror',
>              ],
>              'cflags_cc': [
>                # Don't warn about hash_map in third-party code.
> @@ -1635,6 +1634,12 @@
>                '-Wsign-compare',
>              ]
>            }],
> +          # TODO: Fix all warnings on chromeos too.
> +          [ 'os_posix==1 and OS!="mac" and OS!="ios" and (clang!=1 or
> chromeos==1)', {
> +            'cflags!': [
> +              '-Werror',
> +            ],
> +          }],
>            [ 'os_posix==1 and os_bsd!=1 and OS!="mac" and OS!="android"', {
>              'cflags': [
>                # Don't warn about ignoring the return value from e.g.
> close().
> Index: net/third_party/nss/ssl.gyp
> diff --git a/net/third_party/nss/ssl.gyp b/net/third_party/nss/ssl.gyp
> index 07f1b64fb941af0cbd1acb42bcf335**fdb72f6c1d..**
> 7f319a19000df3c183e33c050b2850**bb9213854a 100644
> --- a/net/third_party/nss/ssl.gyp
> +++ b/net/third_party/nss/ssl.gyp
> @@ -89,6 +89,13 @@
>        ],
>        'msvs_disabled_warnings': [4018, 4244],
>        'conditions': [
> +        [ 'clang == 1', {
> +          'cflags': [
> +            # See http://crbug.com/138571#c8. In short, sslsecur.c picks
> up the
> +            # system's cert.h because cert.h isn't in chromium's repo.
> +            '-Wno-incompatible-pointer-**types',
> +          ],
> +        }],
>          [ 'OS == "mac" or OS == "ios"', {
>            'defines': [
>              'XP_UNIX',
>
>
>

[Nico, 2012-08-18 01:27:17]

s/agl/rsleevi/: rsleevi, please take a look at ssl.gyp.

[Ryan Sleevi, 2012-08-18 01:33:59]

lgtm

http://codereview.chromium.org/10855238/diff/5001/net/third_party/nss/ssl.gyp
File net/third_party/nss/ssl.gyp (right):

http://codereview.chromium.org/10855238/diff/5001/net/third_party/nss/ssl.gyp...
net/third_party/nss/ssl.gyp:95: # system's cert.h because cert.h isn't in
chromium's repo.
Your comment made it seem like you weren't sure if this was intentional.

It is (at least, on Linux).

If you wanted, you could only set this flag for Linux (or more aptly, match the
inverse of the conditional on line 141), so that any future NSS rolls on
Mac/Win/iOS ensured no further NSS-induced regressions. Since rolling NSS is
something generally done by wtc or myself, it's something we can quickly fix
upstream.

[Nico, 2012-08-18 01:35:20]

On Fri, Aug 17, 2012 at 6:33 PM,  <rsleevi@chromium.org> wrote:
> lgtm
>
>
>
>
> http://codereview.chromium.org/10855238/diff/5001/net/third_party/nss/ssl.gyp
> File net/third_party/nss/ssl.gyp (right):
>
>
http://codereview.chromium.org/10855238/diff/5001/net/third_party/nss/ssl.gyp...
> net/third_party/nss/ssl.gyp:95: # system's cert.h because cert.h isn't
> in chromium's repo.
> Your comment made it seem like you weren't sure if this was intentional.
>
> It is (at least, on Linux).

Cool, thanks :-) Do you know why?

>
> If you wanted, you could only set this flag for Linux (or more aptly,
> match the inverse of the conditional on line 141), so that any future
> NSS rolls on Mac/Win/iOS ensured no further NSS-induced regressions.
> Since rolling NSS is something generally done by wtc or myself, it's
> something we can quickly fix upstream.

cflags is only used on linux, so this is linux-only already.

[commit-bot: I haz the power, 2012-08-18 01:35:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/thakis@chromium.org/10855238/5001

[Ryan Sleevi, 2012-08-18 01:46:03]

On 2012/08/18 01:35:20, Nico wrote:
> On Fri, Aug 17, 2012 at 6:33 PM,  <mailto:rsleevi@chromium.org> wrote:
> > lgtm
> >
> >
> >
> >
> >
http://codereview.chromium.org/10855238/diff/5001/net/third_party/nss/ssl.gyp
> > File net/third_party/nss/ssl.gyp (right):
> >
> >
>
http://codereview.chromium.org/10855238/diff/5001/net/third_party/nss/ssl.gyp...
> > net/third_party/nss/ssl.gyp:95: # system's cert.h because cert.h isn't
> > in chromium's repo.
> > Your comment made it seem like you weren't sure if this was intentional.
> >
> > It is (at least, on Linux).
> 
> Cool, thanks :-) Do you know why?

On Linux, we use system NSS and NSPR, but we build and statically link a copy of
libssl (which is also shipped as part of libnss, which we don't use).

See
http://www.mozilla.org/projects/security/pki/nss/nss-guidelines.html#Structure -
everything below libssl we pull from the system-provided libnss , on Linux.
Because NSS and NSPR have super-strong ABI guarantees (as in, hasn't broken ABI
evar), it's "perfectly" safe to use a newer version of libssl even on super-old
libnss's.

On Windows/Mac/iOS, which doesn't have a concept of "system NSS", we statically
link in the whole kit and kaboodle, periodically importing NSS releases into
deps/third_party/nss and pulling in via DEPS.

> 
> >
> > If you wanted, you could only set this flag for Linux (or more aptly,
> > match the inverse of the conditional on line 141), so that any future
> > NSS rolls on Mac/Win/iOS ensured no further NSS-induced regressions.
> > Since rolling NSS is something generally done by wtc or myself, it's
> > something we can quickly fix upstream.
> 
> cflags is only used on linux, so this is linux-only already.

[wtc, 2012-08-18 02:30:07]

lgtm

http://codereview.chromium.org/10855238/diff/5001/net/third_party/nss/ssl.gyp
File net/third_party/nss/ssl.gyp (right):

http://codereview.chromium.org/10855238/diff/5001/net/third_party/nss/ssl.gyp...
net/third_party/nss/ssl.gyp:95: # system's cert.h because cert.h isn't in
chromium's repo.


It may be better to patch sslsecur.c to cast away the
const, than turning off this warning for all the files
in src/net/third_party/nss/ssl.

[commit-bot: I haz the power, 2012-08-18 04:24:33]

Change committed as 152242