Pass -Wno-header-guard when using Clang to avoid reporting the following error in the system NSS he…

Pass -Wno-header-guard when using Clang to avoid reporting the following error in the system NSS header:

=============================
In file included from ../../net/third_party/nss/ssl/ssl3ecc.c:30:
/usr/include/nss/secmod.h:4:9: error: '_SECMOD_H_' is used as a header guard here, followed by #define of a different macro [-Werror,-Wheader-guard]
#ifndef _SECMOD_H_
        ^~~~~~~~~~
/usr/include/nss/secmod.h:5:9: note: '_SEDMOD_H_' is defined here; did you mean '_SECMOD_H_'?
#define _SEDMOD_H_
        ^~~~~~~~~~
        _SECMOD_H_
1 error generated.
=============================

R=thakis@chromium.org
BUG=None

[Alexander Potapenko, 2013-06-19 13:39:26]

Nico, WDYT?

[Nico, 2013-06-19 13:57:57]

Not lgtm. This should be fixed on trunk, wtc fixed this in nss 2 days ago.
I fixed all others wheader-guard warnings. Are you seeing this at chromium
trunk?
On Jun 19, 2013 6:39 AM, <glider@chromium.org> wrote:

> Reviewers: Nico,
>
> Message:
> Nico, WDYT?
>
> Description:
> Pass -Wno-header-guard when using Clang to avoid reporting the following
> error
> in the system NSS header:
>
> =============================
> In file included from ../../net/third_party/nss/ssl/**ssl3ecc.c:30:
> /usr/include/nss/secmod.h:4:9: error: '_SECMOD_H_' is used as a header
> guard
> here, followed by #define of a different macro [-Werror,-Wheader-guard]
> #ifndef _SECMOD_H_
>         ^~~~~~~~~~
> /usr/include/nss/secmod.h:5:9: note: '_SEDMOD_H_' is defined here; did you
> mean
> '_SECMOD_H_'?
> #define _SEDMOD_H_
>         ^~~~~~~~~~
>         _SECMOD_H_
> 1 error generated.
> =============================
>
> R=thakis@chromium.org
> BUG=None
>
> Please review this at
https://codereview.chromium.**org/17003006/<https://codereview.chromium.org/1...
>
> SVN Base:
svn://svn.chromium.org/chrome/**trunk/src/<http://svn.chromium.org/chrome/trunk/src/>
>
> Affected files:
>   M     build/common.gypi
>
>
> Index: build/common.gypi
> ==============================**==============================**=======
> --- build/common.gypi   (revision 206992)
> +++ build/common.gypi   (working copy)
> @@ -3081,6 +3081,11 @@
>
>                # Warns when a const char[] is converted to bool.
>                '-Wstring-conversion',
> +
> +              # Clang reports an incorrect define guard in
> +              # /usr/include/nss/secmod.h
> +              '-Wno-header-guard',
> +
>              ],
>              'cflags!': [
>                # Clang doesn't seem to know know this flag.
>
>
>

[(unused - use chromium), 2013-06-19 13:59:29]

(And if we need to disable a warning for a single project, we should
disable it in that project's gyp file, not globally.)
On Jun 19, 2013 6:57 AM, "Nico Weber" <thakis@chromium.org> wrote:

> Not lgtm. This should be fixed on trunk, wtc fixed this in nss 2 days ago.
> I fixed all others wheader-guard warnings. Are you seeing this at chromium
> trunk?
> On Jun 19, 2013 6:39 AM, <glider@chromium.org> wrote:
>
>> Reviewers: Nico,
>>
>> Message:
>> Nico, WDYT?
>>
>> Description:
>> Pass -Wno-header-guard when using Clang to avoid reporting the following
>> error
>> in the system NSS header:
>>
>> =============================
>> In file included from ../../net/third_party/nss/ssl/**ssl3ecc.c:30:
>> /usr/include/nss/secmod.h:4:9: error: '_SECMOD_H_' is used as a header
>> guard
>> here, followed by #define of a different macro [-Werror,-Wheader-guard]
>> #ifndef _SECMOD_H_
>>         ^~~~~~~~~~
>> /usr/include/nss/secmod.h:5:9: note: '_SEDMOD_H_' is defined here; did
>> you mean
>> '_SECMOD_H_'?
>> #define _SEDMOD_H_
>>         ^~~~~~~~~~
>>         _SECMOD_H_
>> 1 error generated.
>> =============================
>>
>> R=thakis@chromium.org
>> BUG=None
>>
>> Please review this at
https://codereview.chromium.**org/17003006/<https://codereview.chromium.org/1...
>>
>> SVN Base:
svn://svn.chromium.org/chrome/**trunk/src/<http://svn.chromium.org/chrome/trunk/src/>
>>
>> Affected files:
>>   M     build/common.gypi
>>
>>
>> Index: build/common.gypi
>> ==============================**==============================**=======
>> --- build/common.gypi   (revision 206992)
>> +++ build/common.gypi   (working copy)
>> @@ -3081,6 +3081,11 @@
>>
>>                # Warns when a const char[] is converted to bool.
>>                '-Wstring-conversion',
>> +
>> +              # Clang reports an incorrect define guard in
>> +              # /usr/include/nss/secmod.h
>> +              '-Wno-header-guard',
>> +
>>              ],
>>              'cflags!': [
>>                # Clang doesn't seem to know know this flag.
>>
>>
>>

[Alexander Potapenko, 2013-06-19 14:02:53]

I'm using LKGR Chromium and trunk Clang (see the asan-canary bot)
How comes we're including a system header here if we already have our own copy
of NSS in third_party?

[Nico, 2013-06-19 18:31:49]

Oh, I didn't realize that this was in the system copy. Sounds similar to
http://crbug.com/138571#c8 – does it work if you put this flag in
net/third_party/nss/ssl.gyp, next to -Wno-incompatible-pointer-types?

[wtc, 2013-06-19 19:58:05]

On 2013/06/19 18:31:49, Nico wrote:
> Oh, I didn't realize that this was in the system copy. Sounds similar to
> http://crbug.com/138571#c8 – does it work if you put this flag in
> net/third_party/nss/ssl.gyp, next to -Wno-incompatible-pointer-types?

I see... Some files in crypto and net/cert also use NSS, so
net/third_party/nss/ssl.gyp is not the only place that needs
the -Wno-header-guard flag.

I think the best place to add the -Wno-header-guard flag is
build/linux/system.gyp, in the target 'ssl'. We need a new
section:

            ['use_openssl==0 and clang==1', {
              'direct_dependent_settings': {
                'cflags': [
                  # Clang reports an incorrect define guard in
                  # /usr/include/nss/secmod.h
                  '-Wno-header-guard',
                ],
              },
            }],

[wtc, 2013-06-19 20:01:22]

https://codereview.chromium.org/17003006/diff/1/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/17003006/diff/1/build/common.gypi#newcode3086
build/common.gypi:3086: # /usr/include/nss/secmod.h

I forgot to add: please add the upstream NSS bug URL
    https://bugzilla.mozilla.org/show_bug.cgi?id=884072
to this comment, so we know when this flag can be removed.

[(unused - use chromium), 2013-06-27 21:11:07]

On Wed, Jun 19, 2013 at 12:58 PM, <wtc@chromium.org> wrote:

> On 2013/06/19 18:31:49, Nico wrote:
>
>> Oh, I didn't realize that this was in the system copy. Sounds similar to
>> http://crbug.com/138571#c8 – does it work if you put this flag in
>> net/third_party/nss/ssl.gyp, next to -Wno-incompatible-pointer-**types?
>>
>
> I see... Some files in crypto and net/cert also use NSS, so
> net/third_party/nss/ssl.gyp is not the only place that needs
> the -Wno-header-guard flag.
>
> I think the best place to add the -Wno-header-guard flag is
> build/linux/system.gyp, in the target 'ssl'. We need a new
> section:
>
>             ['use_openssl==0 and clang==1', {
>               'direct_dependent_settings': {
>                 'cflags': [
>
>                   # Clang reports an incorrect define guard in
>                   # /usr/include/nss/secmod.h
>                   '-Wno-header-guard',
>                 ],
>               },
>             }],
>

wtc: Do you mean you'd add this in addition to
changing net/third_party/nss/ssl.gyp? net/third_party/nss/ssl.gyp seems to
not depend on build/linux/system.gyp, so it wouldn't pick it up if it's
added only there.


>
>
https://codereview.chromium.**org/17003006/<https://codereview.chromium.org/1...
>

[wtc, 2013-06-27 21:50:03]

On 2013/06/27 21:11:07, thakis wrote:
>
> wtc: Do you mean you'd add this in addition to
> changing net/third_party/nss/ssl.gyp? net/third_party/nss/ssl.gyp seems to
> not depend on build/linux/system.gyp, so it wouldn't pick it up if it's
> added only there.

I actually meant I'd add this to build/linux/system.gyp.

But I didn't realize that net/third_party/nss/ssl.gyp does
not depend on build/linux/system.gyp. I think it should.
Perhaps we should make net/third_party/nss/ssl.gyp depend
on build/linux/system.gyp first. The difficulty with doing
that seems to be we'll need to filter out -lssl3 in
'libraries'.

In any case, I am fine with adding this to both build/linux/system.gyp
and net/third_party/nss/ssl.gyp.

[(unused - use chromium), 2013-06-27 21:54:22]

On Thu, Jun 27, 2013 at 2:50 PM, <wtc@chromium.org> wrote:

> On 2013/06/27 21:11:07, thakis wrote:
>
>  wtc: Do you mean you'd add this in addition to
>> changing net/third_party/nss/ssl.gyp? net/third_party/nss/ssl.gyp seems to
>> not depend on build/linux/system.gyp, so it wouldn't pick it up if it's
>> added only there.
>>
>
> I actually meant I'd add this to build/linux/system.gyp.
>
> But I didn't realize that net/third_party/nss/ssl.gyp does
> not depend on build/linux/system.gyp. I think it should.
> Perhaps we should make net/third_party/nss/ssl.gyp depend
> on build/linux/system.gyp first. The difficulty with doing
> that seems to be we'll need to filter out -lssl3 in
> 'libraries'.
>
> In any case, I am fine with adding this to both build/linux/system.gyp
> and net/third_party/nss/ssl.gyp.
>

Ok, I'll go with that then. Thanks!


>
>
https://codereview.chromium.**org/17003006/<https://codereview.chromium.org/1...
>

[Alexander Potapenko, 2013-06-28 10:01:18]

On 2013/06/27 21:54:22, thakis wrote:
> On Thu, Jun 27, 2013 at 2:50 PM, <mailto:wtc@chromium.org> wrote:
> 
> > On 2013/06/27 21:11:07, thakis wrote:
> >
> >  wtc: Do you mean you'd add this in addition to
> >> changing net/third_party/nss/ssl.gyp? net/third_party/nss/ssl.gyp seems to
> >> not depend on build/linux/system.gyp, so it wouldn't pick it up if it's
> >> added only there.
> >>
> >
> > I actually meant I'd add this to build/linux/system.gyp.
> >
> > But I didn't realize that net/third_party/nss/ssl.gyp does
> > not depend on build/linux/system.gyp. I think it should.
> > Perhaps we should make net/third_party/nss/ssl.gyp depend
> > on build/linux/system.gyp first. The difficulty with doing
> > that seems to be we'll need to filter out -lssl3 in
> > 'libraries'.
Ugh, I've stumbled upon the same issue while trying to fix this CL yesterday.

[Nico, 2013-07-02 17:39:32]

On 2013/06/27 21:54:22, thakis wrote:
> On Thu, Jun 27, 2013 at 2:50 PM, <mailto:wtc@chromium.org> wrote:
> 
> > On 2013/06/27 21:11:07, thakis wrote:
> >
> >  wtc: Do you mean you'd add this in addition to
> >> changing net/third_party/nss/ssl.gyp? net/third_party/nss/ssl.gyp seems to
> >> not depend on build/linux/system.gyp, so it wouldn't pick it up if it's
> >> added only there.
> >>
> >
> > I actually meant I'd add this to build/linux/system.gyp.
> >
> > But I didn't realize that net/third_party/nss/ssl.gyp does
> > not depend on build/linux/system.gyp. I think it should.
> > Perhaps we should make net/third_party/nss/ssl.gyp depend
> > on build/linux/system.gyp first. The difficulty with doing
> > that seems to be we'll need to filter out -lssl3 in
> > 'libraries'.
> >
> > In any case, I am fine with adding this to both build/linux/system.gyp
> > and net/third_party/nss/ssl.gyp.
> >
> 
> Ok, I'll go with that then. Thanks!

I landed this ^ as part of http://crrev.com/209703

> 
> 
> >
> >
>
https://codereview.chromium.**org/17003006/%3Chttps://codereview.chromium.org...>
> >