gn: Fix build on Windows

gn: Fix build on Windows

R=brettw@chromium.org, johannkoenig@google.com

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

[ckocagil, 2014-09-25 20:48:00]

ckocagil@chromium.org changed reviewers:
+ brettw@chromium.org, tomfinegan@chromium.org

[Johann, 2014-09-25 21:00:25]

johannkoenig@google.com changed reviewers:
+ johannkoenig@google.com

[Johann, 2014-09-25 21:00:25]

https://codereview.chromium.org/603173004/diff/1/BUILD.gn
File BUILD.gn (right):

https://codereview.chromium.org/603173004/diff/1/BUILD.gn#newcode46
BUILD.gn:46: "/wd4057",
We (try) to track Windows errors pretty carefully and have removed all the
warnings we're aware of in both our local builds and Chromium. Has something
changed in GN that warnings which were not enabled in GYP are now turned on?

[ckocagil, 2014-09-25 21:06:21]

On 2014/09/25 21:00:25, Johann wrote:
> https://codereview.chromium.org/603173004/diff/1/BUILD.gn
> File BUILD.gn (right):
> 
> https://codereview.chromium.org/603173004/diff/1/BUILD.gn#newcode46
> BUILD.gn:46: "/wd4057",
> We (try) to track Windows errors pretty carefully and have removed all the
> warnings we're aware of in both our local builds and Chromium. Has something
> changed in GN that warnings which were not enabled in GYP are now turned on?

Chromium code uses /W4 (level 4 warnings), while third-party libraries are
(usually?) built with /W3 which is more permissive. So we should decide which
one to use for libvpx, but it's something vpx owners and/or Brett should decide.

[brettw, 2014-09-25 21:08:52]

Could the warnings problem be from the chromium_code setting? I think libvpx
isn't compiled with that set which will mean a lower warning level.

[brettw, 2014-09-25 21:09:31]

On 2014/09/25 21:08:52, brettw wrote:
> Could the warnings problem be from the chromium_code setting? I think libvpx
> isn't compiled with that set which will mean a lower warning level.

I mean I don't see chromium_code set in GYP (where it's off by default), so we
should probably turn it off in GN (where it's on by default).

[ckocagil, 2014-09-25 21:17:28]

On 2014/09/25 21:09:31, brettw wrote:
> On 2014/09/25 21:08:52, brettw wrote:
> > Could the warnings problem be from the chromium_code setting? I think libvpx
> > isn't compiled with that set which will mean a lower warning level.
> 
> I mean I don't see chromium_code set in GYP (where it's off by default), so we
> should probably turn it off in GN (where it's on by default).

Done.

[Johann, 2014-09-25 21:31:20]

I would have expected the Windows build to need the -m<arch> options somewhere
(although maybe only for clang?). For MSVS, it appears the only /arch we use
upstream is AVX (quick search).

Do you have a link to the build failure?

https://codereview.chromium.org/603173004/diff/20001/BUILD.gn
File BUILD.gn (right):

https://codereview.chromium.org/603173004/diff/20001/BUILD.gn#newcode189
BUILD.gn:189: cflags = [ "-mssse3" ]
Specifically for windows and ssse3 the gyp files uses:

        ['OS=="win" and clang==1', {
          # cl.exe's /arch flag doesn't have a setting for SSSE3/4, and cl.exe
          # doesn't need it for intrinsics. clang-cl does need it, though.
          'msvs_settings': {
            'VCCLCompilerTool': { 'AdditionalOptions': [ '-mssse3' ] },
          },
        }],

[brettw, 2014-09-25 21:33:13]

Okay, this makes more sense. LGTM from my perspective.

[jzern, 2014-09-25 21:39:07]

On 2014/09/25 21:00:25, Johann wrote:
> https://codereview.chromium.org/603173004/diff/1/BUILD.gn
> File BUILD.gn (right):
> 
> https://codereview.chromium.org/603173004/diff/1/BUILD.gn#newcode46
> BUILD.gn:46: "/wd4057",
> We (try) to track Windows errors pretty carefully and have removed all the
> warnings we're aware of in both our local builds and Chromium. Has something
> changed in GN that warnings which were not enabled in GYP are now turned on?

The nightly visual studio builds for libvpx run with /W3

[jzern, 2014-09-25 21:40:07]

On 2014/09/25 21:06:21, ckocagil wrote:
> On 2014/09/25 21:00:25, Johann wrote:
> > https://codereview.chromium.org/603173004/diff/1/BUILD.gn
> > File BUILD.gn (right):
> > 
> > https://codereview.chromium.org/603173004/diff/1/BUILD.gn#newcode46
> > BUILD.gn:46: "/wd4057",
> > We (try) to track Windows errors pretty carefully and have removed all the
> > warnings we're aware of in both our local builds and Chromium. Has something
> > changed in GN that warnings which were not enabled in GYP are now turned on?
> 
> Chromium code uses /W4 (level 4 warnings), while third-party libraries are
> (usually?) built with /W3 which is more permissive. So we should decide which
> one to use for libvpx, but it's something vpx owners and/or Brett should
decide.

Is there a link to that setting? No warnings from /W4 disabled?

[ckocagil, 2014-09-25 21:40:56]

On 2014/09/25 21:31:20, Johann wrote:
> I would have expected the Windows build to need the -m<arch> options somewhere
> (although maybe only for clang?). For MSVS, it appears the only /arch we use
> upstream is AVX (quick search).
> 
> Do you have a link to the build failure?

The failure is """Command line warning D9002 : ignoring unknown option
'-msse2'""" since cl.exe doesn't use that syntax.

> https://codereview.chromium.org/603173004/diff/20001/BUILD.gn
> File BUILD.gn (right):
> 
> https://codereview.chromium.org/603173004/diff/20001/BUILD.gn#newcode189
> BUILD.gn:189: cflags = [ "-mssse3" ]
> Specifically for windows and ssse3 the gyp files uses:
> 
>         ['OS=="win" and clang==1', {
>           # cl.exe's /arch flag doesn't have a setting for SSSE3/4, and cl.exe
>           # doesn't need it for intrinsics. clang-cl does need it, though.
>           'msvs_settings': {
>             'VCCLCompilerTool': { 'AdditionalOptions': [ '-mssse3' ] },
>           },
>         }],

So cl.exe doesn't need any options here. I don't know what clang-cl is or
whether it is needed.

[ckocagil, 2014-09-25 21:42:47]

On 2014/09/25 21:40:07, jzern wrote:
> On 2014/09/25 21:06:21, ckocagil wrote:
> > On 2014/09/25 21:00:25, Johann wrote:
> > > https://codereview.chromium.org/603173004/diff/1/BUILD.gn
> > > File BUILD.gn (right):
> > > 
> > > https://codereview.chromium.org/603173004/diff/1/BUILD.gn#newcode46
> > > BUILD.gn:46: "/wd4057",
> > > We (try) to track Windows errors pretty carefully and have removed all the
> > > warnings we're aware of in both our local builds and Chromium. Has
something
> > > changed in GN that warnings which were not enabled in GYP are now turned
on?
> > 
> > Chromium code uses /W4 (level 4 warnings), while third-party libraries are
> > (usually?) built with /W3 which is more permissive. So we should decide
which
> > one to use for libvpx, but it's something vpx owners and/or Brett should
> decide.
> 
> Is there a link to that setting? No warnings from /W4 disabled?

See the configs named "chromium_code" and "no_chromium_code" here:
https://code.google.com/p/chromium/codesearch#chromium/src/build/config/compi...

no_chromium_code uses /W3 and successfully builds vpx.

[Johann, 2014-09-25 21:43:53]

On 2014/09/25 21:40:56, ckocagil wrote:
> >         ['OS=="win" and clang==1', {
> >           # cl.exe's /arch flag doesn't have a setting for SSSE3/4, and
cl.exe
> >           # doesn't need it for intrinsics. clang-cl does need it, though.
> >           'msvs_settings': {
> >             'VCCLCompilerTool': { 'AdditionalOptions': [ '-mssse3' ] },
> >           },
> >         }],
> 
> So cl.exe doesn't need any options here. I don't know what clang-cl is or
> whether it is needed.

LGTM - if a clang-cl.exe target is added, this might have to be revisited.

[ckocagil, 2014-09-25 21:46:45]

On 2014/09/25 21:43:53, Johann wrote:
> On 2014/09/25 21:40:56, ckocagil wrote:
> > >         ['OS=="win" and clang==1', {
> > >           # cl.exe's /arch flag doesn't have a setting for SSSE3/4, and
> cl.exe
> > >           # doesn't need it for intrinsics. clang-cl does need it, though.
> > >           'msvs_settings': {
> > >             'VCCLCompilerTool': { 'AdditionalOptions': [ '-mssse3' ] },
> > >           },
> > >         }],
> > 
> > So cl.exe doesn't need any options here. I don't know what clang-cl is or
> > whether it is needed.
> 
> LGTM - if a clang-cl.exe target is added, this might have to be revisited.

Thanks for the reviews. My priority here is to get the gn Win build on par with
Linux. clang-cl can be made to work later.

[ckocagil, 2014-09-25 21:48:33]

The CQ bit was checked by ckocagil@chromium.org

[commit-bot: I haz the power, 2014-09-25 21:48:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-25 21:48:57]

Commit queue rejected this change because it did not recognize the base URL.
Please commit your change manually.

[Johann, 2014-09-25 21:51:33]

libvpx is still in svn :/ you'll need to upload your change to
svn://svn.chromium.org/chrome/trunk/deps/third_party/libvpx/

[ckocagil, 2014-09-25 21:58:07]

gn: Fix build on Windows

[ckocagil, 2014-09-25 21:58:40]

Committed patchset #3 (id:40001) manually as r292146 (presubmit successful).

[Johann, 2014-09-25 22:11:00]

On 2014/09/25 21:58:40, ckocagil wrote:
> Committed patchset #3 (id:40001) manually as r292146 (presubmit successful).

How did you do that? I usually give up and re-upload from an SVN client.

[ckocagil, 2014-09-25 22:14:27]

On 2014/09/25 22:11:00, Johann wrote:
> On 2014/09/25 21:58:40, ckocagil wrote:
> > Committed patchset #3 (id:40001) manually as r292146 (presubmit successful).
> 
> How did you do that? I usually give up and re-upload from an SVN client.

svn co <svn path to repo> --depth=files (I used a shallow checkout to make it
faster)
patch < my_patch
svn change gn
svn upload gn -i <cl_issue_id>
gcl commit gn

[ckocagil, 2014-09-25 22:16:00]

On 2014/09/25 22:14:27, ckocagil wrote:
> On 2014/09/25 22:11:00, Johann wrote:
> > On 2014/09/25 21:58:40, ckocagil wrote:
> > > Committed patchset #3 (id:40001) manually as r292146 (presubmit
successful).
> > 
> > How did you do that? I usually give up and re-upload from an SVN client.
> 
> svn co <svn path to repo> --depth=files (I used a shallow checkout to make it
> faster)
> patch < my_patch
> svn change gn
> svn upload gn -i <cl_issue_id>
> gcl commit gn

r/svn change/gcl change/
r/svn upload/gcl upload/