Rework win_toolchains a bit and copy the vs runtime DLLs as needed.

Rework win_toolchains a bit and copy the vs runtime DLLs as needed.

In order to run both the visual studio tools and the binaries built
by them (and ninja), we need to ensure that the VS runtime DLLs
are available in the path.

In the GYP build, we accomplish this by copying them into the
Debug and Debug_x64 dirs as appropriate inside the gyp_chromium
script.

In the pure-GN build, then, things would be broken, so we need to
modify the GN build to do the copy as well, or we need to inject
a step somewhere that happens after GN runs but before Ninja tries
to run (since none of the toolchain binaries will work).

This patch accomplishes this by calling out to vs_toolchain.py to
copy the DLLs as neede when the toolchain is defined. This is somewhat
less than ideal (makes 'gn gen' slower) but seems better than forcing
devs to have to run an additional command.

In addition, the GYP build writes targets into Debug and Debug_x64
concurrently. This doesn't really carry over into GN correctly, and
we probably only ever want to write targets into Debug and Debug/64
(or some such).

However, the way the toolchains are currently implemented, it's not
clear if this really works and the interplay between 32-bit and 64-bit
is weird (we apparently normally "force" 32-bit even if we set cpu_arch
to 64-bit, and require you to specify force_win64). To work around this
and make sure that we copy the right DLLs for the right arch into the
outer Debug/ directory, this patch temporarily disables the cross-arch
part of the build, forcing the host_toolchain to match the target_toolchain.

This likely means that 'cpu_arch="x86"' works (the default), but the 'host'
binaries like image_diff and mksnapshot will be compiled in 32-bit mode,
not 64-bit mode. 'cpu_arch="x64" force_win64=true' should also work, and
produce all-64-bit binaries. 'cpu_arch="x64"' does not work at all and
won't until we can clean up the above stuff.

R=scottmg@chromium.org, brettw@chromium.org
BUG=430661
Committed: https://crrev.com/0b95195e49489b7a4d87048d2ce4b747173a5b8a
Cr-Commit-Position: refs/heads/master@{#304310}

[Dirk Pranke, 2014-11-13 03:00:53]

This patch should un-bork the Win GN bots and make things work. 

I had to disable the ability to compile some things in 32-bit mode and some in
64-bit mode, but (a) I'm not sure that the existing code worked at all, and (b)
I think we probably need to re-think this approach regardless, as we probably
shouldn't propagate the way we did things in GYP over into GN.

I think we should try to eliminate the "force_win64" flag and say that if we are
building 32-bit everything should be 32-bit and if we are building 64-bit
everything should be 64-bit, except for the things that *have* to be different
(like NaCl; ignore NaCl for now).

I could be convinced that the above is wrong and that we should preserve the
host/target distinction for things like image_diff, but if we do want to do
that, we need it to be broken temporarily until we can figure out a better way
to handle the two dirs as I think we *really* shouldn't replicate the
Release/Release_x64 confusion ...

Let me know what you think?

[scottmg, 2014-11-13 03:49:59]

https://codereview.chromium.org/722723004/diff/1/build/config/BUILDCONFIG.gn
File build/config/BUILDCONFIG.gn (right):

https://codereview.chromium.org/722723004/diff/1/build/config/BUILDCONFIG.gn#...
build/config/BUILDCONFIG.gn:179: # TODO(dpranke): Delete this? x64 should mean
x64 and force_win64 should
I agree, we should get rid of force_win64, or at least rationalize it (I don't
understand what it's supposed to mean)

https://codereview.chromium.org/722723004/diff/1/build/config/BUILDCONFIG.gn#...
build/config/BUILDCONFIG.gn:504: if (cpu_arch == "x64") {
what's build_cpu_arch supposed to mean? the target that host binaries are built
for? or the bitness that the compiler is? (there's all 4 permutations of
{x86,x64}->{x86,x64} toolchains, the x86 bitness compilers are needed for
Express vs. Pro.

https://codereview.chromium.org/722723004/diff/1/build/toolchain/win/BUILD.gn
File build/toolchain/win/BUILD.gn (right):

https://codereview.chromium.org/722723004/diff/1/build/toolchain/win/BUILD.gn...
build/toolchain/win/BUILD.gn:179: # get it sorted out how we want to support
them both in a single build.
note that only one of these can be enabled at a time because we can't have them
copying runtimes to the same place.

https://codereview.chromium.org/722723004/diff/1/build/vs_toolchain.py
File build/vs_toolchain.py (right):

https://codereview.chromium.org/722723004/diff/1/build/vs_toolchain.py#newcod...
build/vs_toolchain.py:103: copy_runtime_impl(os.path.join(out_release,
pgo_runtime_dll), source_x86)
this will be broken

https://codereview.chromium.org/722723004/diff/1/build/vs_toolchain.py#newcod...
build/vs_toolchain.py:117: release config only the latter.
add a note to see the gyp version above, and note that this is used for GN and
the above for GYP

https://codereview.chromium.org/722723004/diff/1/build/vs_toolchain.py#newcod...
build/vs_toolchain.py:118: """
add a note that this will mean nacl tests will fail

https://codereview.chromium.org/722723004/diff/1/build/vs_toolchain.py#newcod...
build/vs_toolchain.py:198: def _CopyRuntime(target_dir, source_dir,
dll_pattern):
nit; move this up to before where it's used

[Dirk Pranke, 2014-11-13 05:49:36]

https://codereview.chromium.org/722723004/diff/1/build/config/BUILDCONFIG.gn
File build/config/BUILDCONFIG.gn (right):

https://codereview.chromium.org/722723004/diff/1/build/config/BUILDCONFIG.gn#...
build/config/BUILDCONFIG.gn:179: # TODO(dpranke): Delete this? x64 should mean
x64 and force_win64 should
On 2014/11/13 03:49:59, scottmg wrote:
> I agree, we should get rid of force_win64, or at least rationalize it (I don't
> understand what it's supposed to mean)

Yeah, I'm waiting for Brett's feedback, because I'm not sure why things are the
way they are, either.

https://codereview.chromium.org/722723004/diff/1/build/config/BUILDCONFIG.gn#...
build/config/BUILDCONFIG.gn:504: if (cpu_arch == "x64") {
On 2014/11/13 03:49:59, scottmg wrote:
> what's build_cpu_arch supposed to mean? the target that host binaries are
built
> for? or the bitness that the compiler is? (there's all 4 permutations of
> {x86,x64}->{x86,x64} toolchains, the x86 bitness compilers are needed for
> Express vs. Pro.

cpu_arch is the architecture you're trying to build for (target). build_cpu_arch
is the architecture you're actually running on. The clearest example is in the
Android builds, where build_cpu_arch == x64 (when running on Linux using a
64-bit GN on a 64-bit kernel) and cpu_arch == arm.

GN doesn't give you a built-in way to cross-compile for a different "host"
architecture, so I think that's what Brett is attempting to do with
host_toolchain, here (so, in the other files, we depend on things like
"image_diff($host_toolchain)".

I don't understand if this complexity is really needed; another place where I
need feedback from Brett.

https://codereview.chromium.org/722723004/diff/1/build/toolchain/win/BUILD.gn
File build/toolchain/win/BUILD.gn (right):

https://codereview.chromium.org/722723004/diff/1/build/toolchain/win/BUILD.gn...
build/toolchain/win/BUILD.gn:179: # get it sorted out how we want to support
them both in a single build.
On 2014/11/13 03:49:59, scottmg wrote:
> note that only one of these can be enabled at a time because we can't have
them
> copying runtimes to the same place.

Will do.

https://codereview.chromium.org/722723004/diff/1/build/vs_toolchain.py
File build/vs_toolchain.py (right):

https://codereview.chromium.org/722723004/diff/1/build/vs_toolchain.py#newcod...
build/vs_toolchain.py:103: copy_runtime_impl(os.path.join(out_release,
pgo_runtime_dll), source_x86)
On 2014/11/13 03:49:59, scottmg wrote:
> this will be broken

Good catch. Will fix. I thought I searched for other references to this, but I
guess I did that wrong.

https://codereview.chromium.org/722723004/diff/1/build/vs_toolchain.py#newcod...
build/vs_toolchain.py:117: release config only the latter.
On 2014/11/13 03:49:59, scottmg wrote:
> add a note to see the gyp version above, and note that this is used for GN and
> the above for GYP

Will do.

https://codereview.chromium.org/722723004/diff/1/build/vs_toolchain.py#newcod...
build/vs_toolchain.py:118: """
On 2014/11/13 03:49:59, scottmg wrote:
> add a note that this will mean nacl tests will fail

I'm not sure what you're referring to here. Which nacl tests? 

Do you mean something more specific than the idea that getting NaCl to work at
under GN on Windows will be blocked on getting this stuff straightened out?

https://codereview.chromium.org/722723004/diff/1/build/vs_toolchain.py#newcod...
build/vs_toolchain.py:198: def _CopyRuntime(target_dir, source_dir,
dll_pattern):
On 2014/11/13 03:49:59, scottmg wrote:
> nit; move this up to before where it's used

Will do.

[Nick Bray (chromium), 2014-11-13 20:10:52]

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

[Nick Bray (chromium), 2014-11-13 20:10:53]

https://codereview.chromium.org/722723004/diff/1/build/vs_toolchain.py
File build/vs_toolchain.py (right):

https://codereview.chromium.org/722723004/diff/1/build/vs_toolchain.py#newcod...
build/vs_toolchain.py:118: """
On 2014/11/13 05:49:36, Dirk Pranke wrote:
> On 2014/11/13 03:49:59, scottmg wrote:
> > add a note that this will mean nacl tests will fail
> 
> I'm not sure what you're referring to here. Which nacl tests? 
> 
> Do you mean something more specific than the idea that getting NaCl to work at
> under GN on Windows will be blocked on getting this stuff straightened out?

Reading between the lines: NaCl will need x64 dlls for x86 Chrome?  Is that the
issue?

[brettw, 2014-11-14 20:45:45]

https://codereview.chromium.org/722723004/diff/1/build/config/BUILDCONFIG.gn
File build/config/BUILDCONFIG.gn (right):

https://codereview.chromium.org/722723004/diff/1/build/config/BUILDCONFIG.gn#...
build/config/BUILDCONFIG.gn:179: # TODO(dpranke): Delete this? x64 should mean
x64 and force_win64 should
On 2014/11/13 03:49:59, scottmg wrote:
> I agree, we should get rid of force_win64, or at least rationalize it (I don't
> understand what it's supposed to mean)

Let's just delete all force_win64 stuff (including this block) and use the
current CPU arch to define the build type. When I wrote this, we didn't have
full x64 build support so there was some weirdness in that we needed to support
some, but not all of the build.

https://codereview.chromium.org/722723004/diff/1/build/config/BUILDCONFIG.gn#...
build/config/BUILDCONFIG.gn:503: # TODO(dpranke): Fix this.
I'd say we should delete my comment and your todo and just write here that on
Windows, we always use the same arch for host and target. This should make the
builds faster and I think will always work. It also doesn't prevent us from
cross-compiling for special cases.

https://codereview.chromium.org/722723004/diff/1/build/toolchain/win/BUILD.gn
File build/toolchain/win/BUILD.gn (right):

https://codereview.chromium.org/722723004/diff/1/build/toolchain/win/BUILD.gn...
build/toolchain/win/BUILD.gn:28: #  TODO(dpranke): Get rid of force_win64?
You can delete this because you should get rid of it.

[Will Harris, 2014-11-14 21:25:02]

On 2014/11/13 20:10:53, Nick Bray (chromium) wrote:

> Reading between the lines: NaCl will need x64 dlls for x86 Chrome?  Is that
the
> issue?

32-bit Chrome still needs to ship/build the 64-bit IRT (nacl_irt_x86_64.nexe)

[Nick Bray (chromium), 2014-11-14 21:31:32]

On 2014/11/14 21:25:02, Will Harris wrote:
> On 2014/11/13 20:10:53, Nick Bray (chromium) wrote:
> 
> > Reading between the lines: NaCl will need x64 dlls for x86 Chrome?  Is that
> the
> > issue?
> 
> 32-bit Chrome still needs to ship/build the 64-bit IRT (nacl_irt_x86_64.nexe)

Right, but the 64-bit IRT is completely insulated from VS and DLLs so it should
not be affected by this CL.  I believe it's the nacl64.exe binary that's the
issue because it's a x64 binary built in the x86 build.  I was trying to confirm
that was the issue.

[Dirk Pranke, 2014-11-14 23:03:22]

Okay, everything is updated. Please take another look?

With these changes, you'll get a win x64 build by default, assuming you're on
64-bit windows, and that build doesn't currently compile. So, if you're testing
this, make sure you specify cpu_arch="x86". I need to modify the bots
accordingly as well.

https://codereview.chromium.org/722723004/diff/1/build/config/BUILDCONFIG.gn
File build/config/BUILDCONFIG.gn (right):

https://codereview.chromium.org/722723004/diff/1/build/config/BUILDCONFIG.gn#...
build/config/BUILDCONFIG.gn:179: # TODO(dpranke): Delete this? x64 should mean
x64 and force_win64 should
On 2014/11/14 20:45:45, brettw wrote:
> On 2014/11/13 03:49:59, scottmg wrote:
> > I agree, we should get rid of force_win64, or at least rationalize it (I
don't
> > understand what it's supposed to mean)
> 
> Let's just delete all force_win64 stuff (including this block) and use the
> current CPU arch to define the build type. When I wrote this, we didn't have
> full x64 build support so there was some weirdness in that we needed to
support
> some, but not all of the build.

Done.

https://codereview.chromium.org/722723004/diff/1/build/config/BUILDCONFIG.gn#...
build/config/BUILDCONFIG.gn:503: # TODO(dpranke): Fix this.
On 2014/11/14 20:45:45, brettw wrote:
> I'd say we should delete my comment and your todo and just write here that on
> Windows, we always use the same arch for host and target. This should make the
> builds faster and I think will always work. It also doesn't prevent us from
> cross-compiling for special cases.

Done.

https://codereview.chromium.org/722723004/diff/1/build/toolchain/win/BUILD.gn
File build/toolchain/win/BUILD.gn (right):

https://codereview.chromium.org/722723004/diff/1/build/toolchain/win/BUILD.gn...
build/toolchain/win/BUILD.gn:28: #  TODO(dpranke): Get rid of force_win64?
On 2014/11/14 20:45:45, brettw wrote:
> You can delete this because you should get rid of it.

Done.

https://codereview.chromium.org/722723004/diff/1/build/toolchain/win/BUILD.gn...
build/toolchain/win/BUILD.gn:179: # get it sorted out how we want to support
them both in a single build.
On 2014/11/13 05:49:36, Dirk Pranke wrote:
> On 2014/11/13 03:49:59, scottmg wrote:
> > note that only one of these can be enabled at a time because we can't have
> them
> > copying runtimes to the same place.
> 
> Will do.

Done.

https://codereview.chromium.org/722723004/diff/1/build/vs_toolchain.py
File build/vs_toolchain.py (right):

https://codereview.chromium.org/722723004/diff/1/build/vs_toolchain.py#newcod...
build/vs_toolchain.py:198: def _CopyRuntime(target_dir, source_dir,
dll_pattern):
On 2014/11/13 05:49:36, Dirk Pranke wrote:
> On 2014/11/13 03:49:59, scottmg wrote:
> > nit; move this up to before where it's used
> 
> Will do.

Done.

[scottmg, 2014-11-14 23:14:17]

lgtm

https://codereview.chromium.org/722723004/diff/40001/build/vs_toolchain.py
File build/vs_toolchain.py (right):

https://codereview.chromium.org/722723004/diff/40001/build/vs_toolchain.py#ne...
build/vs_toolchain.py:150: _CopyRuntime(target_dir, runtime_dir,
'msvc%s120d.dll')
pgo won't work with gn then, but i guess not much else does either, so that's ok
for now.

[Dirk Pranke, 2014-11-14 23:24:27]

https://codereview.chromium.org/722723004/diff/40001/build/vs_toolchain.py
File build/vs_toolchain.py (right):

https://codereview.chromium.org/722723004/diff/40001/build/vs_toolchain.py#ne...
build/vs_toolchain.py:150: _CopyRuntime(target_dir, runtime_dir,
'msvc%s120d.dll')
On 2014/11/14 23:14:17, scottmg wrote:
> pgo won't work with gn then, but i guess not much else does either, so that's
ok
> for now.

I don't understand this comment?

[brettw, 2014-11-14 23:24:33]

lgtm

[Dirk Pranke, 2014-11-14 23:26:28]

The CQ bit was checked by dpranke@chromium.org

[Dirk Pranke, 2014-11-14 23:26:32]

The CQ bit was unchecked by dpranke@chromium.org

[Dirk Pranke, 2014-11-14 23:26:38]

The CQ bit was checked by dpranke@chromium.org

[scottmg, 2014-11-14 23:26:49]

https://codereview.chromium.org/722723004/diff/40001/build/vs_toolchain.py
File build/vs_toolchain.py (right):

https://codereview.chromium.org/722723004/diff/40001/build/vs_toolchain.py#ne...
build/vs_toolchain.py:150: _CopyRuntime(target_dir, runtime_dir,
'msvc%s120d.dll')
On 2014/11/14 23:24:27, Dirk Pranke wrote:
> On 2014/11/14 23:14:17, scottmg wrote:
> > pgo won't work with gn then, but i guess not much else does either, so
that's
> ok
> > for now.
> 
> I don't understand this comment? 

Sorry. The gyp build calls |CopyVsRuntimeDlls| which copies the CRT runtime
dlls, but also pgort120.dll which is a DLL required when running in PGO
(profiling) mode. The GN path |CopyDlls| does not currently do that copy, so at
some future time when we switch Official to GN, it'll fail because those DLLs
aren't there.

[commit-bot: I haz the power, 2014-11-14 23:28:29]

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

[Dirk Pranke, 2014-11-14 23:30:30]

https://codereview.chromium.org/722723004/diff/40001/build/vs_toolchain.py
File build/vs_toolchain.py (right):

https://codereview.chromium.org/722723004/diff/40001/build/vs_toolchain.py#ne...
build/vs_toolchain.py:150: _CopyRuntime(target_dir, runtime_dir,
'msvc%s120d.dll')
On 2014/11/14 23:26:49, scottmg wrote:
> On 2014/11/14 23:24:27, Dirk Pranke wrote:
> > On 2014/11/14 23:14:17, scottmg wrote:
> > > pgo won't work with gn then, but i guess not much else does either, so
> that's
> > ok
> > > for now.
> > 
> > I don't understand this comment? 
> 
> Sorry. The gyp build calls |CopyVsRuntimeDlls| which copies the CRT runtime
> dlls, but also pgort120.dll which is a DLL required when running in PGO
> (profiling) mode. The GN path |CopyDlls| does not currently do that copy, so
at
> some future time when we switch Official to GN, it'll fail because those DLLs
> aren't there.

Ah. That was an unintentional omission. I'll add that in a follow-up patch.

[commit-bot: I haz the power, 2014-11-15 00:09:24]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2014-11-15 00:10:55]

Patchset 3 (id:??) landed as
https://crrev.com/0b95195e49489b7a4d87048d2ce4b747173a5b8a
Cr-Commit-Position: refs/heads/master@{#304310}