gn: Fix more build issues on Win

gn: Fix more build issues on Win

BUG=354261
R=brettw
TBR=piman,wtc
NOTRY=true

Committed: https://crrev.com/fc8d7f23b2f20a93c5d987d0c073e892d09f0ade
Cr-Commit-Position: refs/heads/master@{#297481}

[ckocagil, 2014-09-25 13:32:47]

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

[ckocagil, 2014-09-25 20:51:06]

https://codereview.chromium.org/603143002/diff/60001/build/toolchain/win/BUIL...
File build/toolchain/win/BUILD.gn (right):

https://codereview.chromium.org/603143002/diff/60001/build/toolchain/win/BUIL...
build/toolchain/win/BUILD.gn:74: command = "$python_path gyp-win-tool
asm-wrapper $env ml.exe {{defines}} {{include_dirs}} /safeseh /c /Fo {{output}}
{{source}}"
I put the /safeseh here but ideally this should be parametrized like cflags.
Does that require changes to gn itself?

[ckocagil, 2014-09-27 14:10:23]

ping (in case this went unnoticed)

[brettw, 2014-09-29 16:45:24]

https://codereview.chromium.org/603143002/diff/60001/build/toolchain/win/BUIL...
File build/toolchain/win/BUILD.gn (right):

https://codereview.chromium.org/603143002/diff/60001/build/toolchain/win/BUIL...
build/toolchain/win/BUILD.gn:74: command = "$python_path gyp-win-tool
asm-wrapper $env ml.exe {{defines}} {{include_dirs}} /safeseh /c /Fo {{output}}
{{source}}"
On 2014/09/25 20:51:06, ckocagil wrote:
> I put the /safeseh here but ideally this should be parametrized like cflags.
> Does that require changes to gn itself?

Looks like it. I filed
https://code.google.com/p/chromium/issues/detail?id=418613
for this. I think your change is correct for now, I'm not sure we'll ever need
more than this so adding the flags variable might be a waste of time. Can you
reference that bug in a comment here?

https://codereview.chromium.org/603143002/diff/60001/build/vs_toolchain.py
File build/vs_toolchain.py (right):

https://codereview.chromium.org/603143002/diff/60001/build/vs_toolchain.py#ne...
build/vs_toolchain.py:168: # If WINDOWSSDKDIR is not set, search the default SDK
path and set it.
I'd like scottmg to check this one.

https://codereview.chromium.org/603143002/diff/60001/gpu/config/BUILD.gn
File gpu/config/BUILD.gn (right):

https://codereview.chromium.org/603143002/diff/60001/gpu/config/BUILD.gn#newc...
gpu/config/BUILD.gn:51: include_dirs = [ "//third_party/mesa/src/include" ]
Can you add a comment here explaining why this is necessary?

[scottmg, 2014-09-29 18:14:28]

scottmg@chromium.org changed reviewers:
+ scottmg@chromium.org

[scottmg, 2014-09-29 18:14:28]

https://codereview.chromium.org/603143002/diff/60001/build/vs_toolchain.py
File build/vs_toolchain.py (right):

https://codereview.chromium.org/603143002/diff/60001/build/vs_toolchain.py#ne...
build/vs_toolchain.py:168: # If WINDOWSSDKDIR is not set, search the default SDK
path and set it.
On 2014/09/29 16:45:24, brettw wrote:
> I'd like scottmg to check this one.

I think it'd be better to do this in build/config/win/visual_studio_version.gni
instead if that works.

[ckocagil, 2014-09-29 21:47:09]

Comments addressed. I fixed a few more errors in Patch Set 5. PTAL.

https://codereview.chromium.org/603143002/diff/60001/build/toolchain/win/BUIL...
File build/toolchain/win/BUILD.gn (right):

https://codereview.chromium.org/603143002/diff/60001/build/toolchain/win/BUIL...
build/toolchain/win/BUILD.gn:74: command = "$python_path gyp-win-tool
asm-wrapper $env ml.exe {{defines}} {{include_dirs}} /safeseh /c /Fo {{output}}
{{source}}"
On 2014/09/29 16:45:24, brettw wrote:
> On 2014/09/25 20:51:06, ckocagil wrote:
> > I put the /safeseh here but ideally this should be parametrized like cflags.
> > Does that require changes to gn itself?
> 
> Looks like it. I filed
> https://code.google.com/p/chromium/issues/detail?id=418613
> for this. I think your change is correct for now, I'm not sure we'll ever need
> more than this so adding the flags variable might be a waste of time. Can you
> reference that bug in a comment here?

Done.

https://codereview.chromium.org/603143002/diff/60001/build/vs_toolchain.py
File build/vs_toolchain.py (right):

https://codereview.chromium.org/603143002/diff/60001/build/vs_toolchain.py#ne...
build/vs_toolchain.py:168: # If WINDOWSSDKDIR is not set, search the default SDK
path and set it.
On 2014/09/29 18:14:28, scottmg wrote:
> On 2014/09/29 16:45:24, brettw wrote:
> > I'd like scottmg to check this one.
> 
> I think it'd be better to do this in
build/config/win/visual_studio_version.gni
> instead if that works.

We wouldn't be able to check whether the path exists on the OS there though.
Also, I don't see how the gni is preferable to here. AFAICT we do all the path
lookup stuff here and simply use the results in gni.

https://codereview.chromium.org/603143002/diff/60001/gpu/config/BUILD.gn
File gpu/config/BUILD.gn (right):

https://codereview.chromium.org/603143002/diff/60001/gpu/config/BUILD.gn#newc...
gpu/config/BUILD.gn:51: include_dirs = [ "//third_party/mesa/src/include" ]
On 2014/09/29 16:45:24, brettw wrote:
> Can you add a comment here explaining why this is necessary?

Done.

[scottmg, 2014-09-29 22:00:19]

https://codereview.chromium.org/603143002/diff/60001/build/vs_toolchain.py
File build/vs_toolchain.py (right):

https://codereview.chromium.org/603143002/diff/60001/build/vs_toolchain.py#ne...
build/vs_toolchain.py:168: # If WINDOWSSDKDIR is not set, search the default SDK
path and set it.
On 2014/09/29 21:47:09, ckocagil wrote:
> On 2014/09/29 18:14:28, scottmg wrote:
> > On 2014/09/29 16:45:24, brettw wrote:
> > > I'd like scottmg to check this one.
> > 
> > I think it'd be better to do this in
> build/config/win/visual_studio_version.gni
> > instead if that works.
> 
> We wouldn't be able to check whether the path exists on the OS there though.
> Also, I don't see how the gni is preferable to here. AFAICT we do all the path
> lookup stuff here and simply use the results in gni.

Ah, OK. lgtm then. I'm pretty "excited" that we have 3 copies of this now that
are slowly forking
https://code.google.com/p/chromium/codesearch#search/&q=gettoolchaindir&sq=pa...

[brettw, 2014-09-30 17:05:03]

lgtm

[ckocagil, 2014-09-30 17:05:49]

The CQ bit was checked by ckocagil@chromium.org

[commit-bot: I haz the power, 2014-09-30 17:06:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/603143002/100001

[commit-bot: I haz the power, 2014-09-30 17:14:58]

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

[commit-bot: I haz the power, 2014-09-30 17:15:00]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[brettw, 2014-09-30 17:25:59]

This should be OK to TBR other owners for.

[ckocagil, 2014-09-30 18:07:47]

The CQ bit was checked by ckocagil@chromium.org

[commit-bot: I haz the power, 2014-09-30 18:09:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/603143002/120001

[commit-bot: I haz the power, 2014-09-30 18:35:49]

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

[commit-bot: I haz the power, 2014-09-30 18:35:51]

Try jobs failed on following builders:
  linux_chromium_gn_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[ckocagil, 2014-09-30 19:26:37]

gn trybot failures are flakes, sheriffs are investigating. linux_chromium_gn_rel
passed, so this should be safe to land.

[ckocagil, 2014-09-30 19:29:02]

The CQ bit was checked by ckocagil@chromium.org

[commit-bot: I haz the power, 2014-09-30 19:30:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/603143002/120001

[commit-bot: I haz the power, 2014-09-30 19:31:49]

Committed patchset #7 (id:120001) as 65b20d7fb9c70cd7781020dc8616fe712e4481c0

[commit-bot: I haz the power, 2014-09-30 19:32:34]

Patchset 7 (id:??) landed as
https://crrev.com/fc8d7f23b2f20a93c5d987d0c073e892d09f0ade
Cr-Commit-Position: refs/heads/master@{#297481}