Re-disable warning 4018 in the GN Windows build.

Re-disable warning 4018 in the GN Windows build.

Note even base compiles with the change to disable it.

TBR=pkasting

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=285860

[brettw, 2014-07-27 22:47:56]

The CQ bit was checked by brettw@chromium.org

[commit-bot: I haz the power, 2014-07-27 22:48:58]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/brettw@chromium.org/422773002/1

[commit-bot: I haz the power, 2014-07-28 03:16:42]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)

[commit-bot: I haz the power, 2014-07-28 04:04:24]

Change committed as 285860

[Peter Kasting, 2014-07-28 11:20:48]

Where are the build failures from this?  I can't fix what I can't see.

Your CL description is confusing.  "Note even base compiles with the change to
disable it"?  Was that supposed to be "Not even base compiles with the change to
enable it"?

https://codereview.chromium.org/422773002/diff/1/build/config/compiler/BUILD.gn
File build/config/compiler/BUILD.gn (right):

https://codereview.chromium.org/422773002/diff/1/build/config/compiler/BUILD....
build/config/compiler/BUILD.gn:571: # TODO(GYP) The GYP build doesn't have this
globally enabled but disabled
Is "doesn't have" here supposed to read "has"?

Why are these targets (those that build in GN, at least) not also disabling this
warning in the GN build?  It concerns me if the GN build is globally disabling
something that isn't globally disabled in the GYP build.

[brettw, 2014-07-28 18:05:42]

https://codereview.chromium.org/422773002/diff/1/build/config/compiler/BUILD.gn
File build/config/compiler/BUILD.gn (right):

https://codereview.chromium.org/422773002/diff/1/build/config/compiler/BUILD....
build/config/compiler/BUILD.gn:571: # TODO(GYP) The GYP build doesn't have this
globally enabled but disabled
On 2014/07/28 11:20:47, Peter Kasting wrote:
> Is "doesn't have" here supposed to read "has"?
> 
> Why are these targets (those that build in GN, at least) not also disabling
this
> warning in the GN build?  It concerns me if the GN build is globally disabling
> something that isn't globally disabled in the GYP build.

Previously it was globally disabled in the GYP build and then a bunch of targets
individually disabled it. I don't know why this was the case and I'm quire
surprised that removing the global one actually worked.

In GYP we have a bunch of stuff disabled globally and the same things disabled
per-target. I'm not duplicating the per-target ones if it's disabled globally.

I agree that having a divergence is bad. I think the right solution is to either
disable it in GYP globally again or go in and add it to the gn files missing it.

[Peter Kasting, 2014-07-28 19:20:50]

https://codereview.chromium.org/422773002/diff/1/build/config/compiler/BUILD.gn
File build/config/compiler/BUILD.gn (right):

https://codereview.chromium.org/422773002/diff/1/build/config/compiler/BUILD....
build/config/compiler/BUILD.gn:571: # TODO(GYP) The GYP build doesn't have this
globally enabled but disabled
On 2014/07/28 18:05:42, brettw wrote:
> On 2014/07/28 11:20:47, Peter Kasting wrote:
> > Is "doesn't have" here supposed to read "has"?
> > 
> > Why are these targets (those that build in GN, at least) not also disabling
> this
> > warning in the GN build?  It concerns me if the GN build is globally
disabling
> > something that isn't globally disabled in the GYP build.
> 
> Previously it was globally disabled in the GYP build and then a bunch of
targets
> individually disabled it. I don't know why this was the case and I'm quire
> surprised that removing the global one actually worked.

Removing the global one worked because I manually fixed all the errors that it
triggered before I landed the change to remove it.  Ideally individual targets
would not override this, but even if they do, this means we still have the
warning enabled for the majority of our codebase, which is progress over where
we were before.

> In GYP we have a bunch of stuff disabled globally and the same things disabled
> per-target. I'm not duplicating the per-target ones if it's disabled globally.
> 
> I agree that having a divergence is bad. I think the right solution is to
either
> disable it in GYP globally again or go in and add it to the gn files missing
it.

Disabling it globally in GYP would be strictly worse than today, since that
would mean we won't get the warning coverage for all the targets that don't
individually disable it.

Can you change this to match the GYP build, i.e. re-enable this globally and
then disable it per-target instead?  Or if there's a list of the relevant
targets, send that to me to update, or something?

Basically, let's work together to make sure this is on by default instead of
off.

[brettw, 2014-07-28 21:27:21]

On 2014/07/28 19:20:50, Peter Kasting wrote:
>
https://codereview.chromium.org/422773002/diff/1/build/config/compiler/BUILD.gn
> File build/config/compiler/BUILD.gn (right):
> 
>
https://codereview.chromium.org/422773002/diff/1/build/config/compiler/BUILD....
> build/config/compiler/BUILD.gn:571: # TODO(GYP) The GYP build doesn't have
this
> globally enabled but disabled
> On 2014/07/28 18:05:42, brettw wrote:
> > On 2014/07/28 11:20:47, Peter Kasting wrote:
> > > Is "doesn't have" here supposed to read "has"?
> > > 
> > > Why are these targets (those that build in GN, at least) not also
disabling
> > this
> > > warning in the GN build?  It concerns me if the GN build is globally
> disabling
> > > something that isn't globally disabled in the GYP build.
> > 
> > Previously it was globally disabled in the GYP build and then a bunch of
> targets
> > individually disabled it. I don't know why this was the case and I'm quire
> > surprised that removing the global one actually worked.
> 
> Removing the global one worked because I manually fixed all the errors that it
> triggered before I landed the change to remove it.  Ideally individual targets
> would not override this, but even if they do, this means we still have the
> warning enabled for the majority of our codebase, which is progress over where
> we were before.

Ah that, makes sense.


> > In GYP we have a bunch of stuff disabled globally and the same things
disabled
> > per-target. I'm not duplicating the per-target ones if it's disabled
globally.
> > 
> > I agree that having a divergence is bad. I think the right solution is to
> either
> > disable it in GYP globally again or go in and add it to the gn files missing
> it.
> 
> Disabling it globally in GYP would be strictly worse than today, since that
> would mean we won't get the warning coverage for all the targets that don't
> individually disable it.

> 
> Can you change this to match the GYP build, i.e. re-enable this globally and
> then disable it per-target instead?  Or if there's a list of the relevant
> targets, send that to me to update, or something?

Sure:
git grep -l "4018,"|grep gyp
base/base.gypi
crypto/crypto.gyp
gpu/gles2_conform_support/gles2_conform_test.gyp
net/third_party/nss/ssl.gyp
third_party/cld/cld.gyp
third_party/leveldatabase/leveldatabase.gyp
third_party/libexif/libexif.gyp
third_party/libxml/libxml.gyp
third_party/mesa/mesa.gyp
third_party/protobuf/protobuf.gyp
third_party/re2/re2.gyp
third_party/snappy/snappy.gyp
third_party/sqlite/sqlite.gyp
third_party/usrsctp/usrsctp.gyp

I think these should mostly all compile on Windows GN except I don't think mesa
will, and I'm not sure usrsctp or libexif exists. I noticed this morning that
protobuf has a WIN32_LEAN_AND_MEAN error so you can ignore that if you see it.

Even for the ones that don't compile, it should be pretty obvious what to add.
As long as GN runs and doesn't give a syntax error, we'll fix the Windows build
for that target when we get there.

If you haven't tried GN yet:

  gn gen out/mybuild
  ninja -C out/mybuild libxml

and that target would be in third_party/libxml/BUILD.gn

You would add:
  if (is_win) {
    cflags = [ "/wd4018" ]  # Name of warning here.
  }
to the relevant targets. Many of them likely have 4267 or something else
disabled, so you can just tack onto that list.