GN: Fix CPU detection in Visual Studio generator

GN: Fix CPU detection in Visual Studio generator

The previous version assumed that if there is no build override
for "current_cpu" then it means "x86".

R=brettw@chromium.org, tmoniuszko@opera.com
BUG=

Committed: https://crrev.com/5dd34fc85aab35792d7034ac3caf3a8f9f38ba8a
Cr-Commit-Position: refs/heads/master@{#377577}

[brettw, 2016-02-24 21:51:48]

I'll give an owners LGTM, but please wait for Tomasz's OK before landing.

https://codereview.chromium.org/1733023002/diff/1/tools/gn/visual_studio_writ...
File tools/gn/visual_studio_writer.cc (right):

https://codereview.chromium.org/1733023002/diff/1/tools/gn/visual_studio_writ...
tools/gn/visual_studio_writer.cc:224: // between different toolchains.
This is not the case (CPU does change between toolchains, the PNaCl toolchain in
Chrome does this, and for a long time 32-bit Windows builds of Chrome were
actually cross-compiles using 32-bit target and 64-bit host (we changed this
since it's wasteful and unnecessary to compile those things more than once).

It's reasonable that the VS generator isn't perfect, especially if this ends up
not mattering too much in practice. Can you reword this comment to make more
clear that this is an incorrect simplifying assumption, and what the problems
are that will result?

https://codereview.chromium.org/1733023002/diff/1/tools/gn/visual_studio_writ...
tools/gn/visual_studio_writer.cc:227: const Value* is_debug_value =
scope->GetValue("is_debug");
"is_debug" is a variable in the Chrome build, not something built into GN. Can
we avoid making assumptions about such things here?

I realize you just moved existing code so this is partially a question for
tmoniuszko: What happens if we don't do this? Can we just have a "generic"
configuration that VS sees? I don't think there's any actual switching we can do
in the IDE.

[NGG, 2016-02-24 22:19:16]

https://codereview.chromium.org/1733023002/diff/1/tools/gn/visual_studio_writ...
File tools/gn/visual_studio_writer.cc (right):

https://codereview.chromium.org/1733023002/diff/1/tools/gn/visual_studio_writ...
tools/gn/visual_studio_writer.cc:224: // between different toolchains.
On 2016/02/24 21:51:48, brettw wrote:
> This is not the case (CPU does change between toolchains, the PNaCl toolchain
in
> Chrome does this, and for a long time 32-bit Windows builds of Chrome were
> actually cross-compiles using 32-bit target and 64-bit host (we changed this
> since it's wasteful and unnecessary to compile those things more than once).
> 
> It's reasonable that the VS generator isn't perfect, especially if this ends
up
> not mattering too much in practice. Can you reword this comment to make more
> clear that this is an incorrect simplifying assumption, and what the problems
> are that will result?

Cross-compiles work with this if only the "current_*" variables change with the
toolchain and not the "target_*" ones, I think this is true.
If "target_cpu" really changes then here we would need to find the default
toolchain's scope and I didn't find an easier way than to look at targets in a
loop until we find one with the default toolchain. I didn't want that because it
would probably slow down the generator a bit.

https://codereview.chromium.org/1733023002/diff/1/tools/gn/visual_studio_writ...
tools/gn/visual_studio_writer.cc:227: const Value* is_debug_value =
scope->GetValue("is_debug");
On 2016/02/24 21:51:48, brettw wrote:
> "is_debug" is a variable in the Chrome build, not something built into GN. Can
> we avoid making assumptions about such things here?
> 
> I realize you just moved existing code so this is partially a question for
> tmoniuszko: What happens if we don't do this? Can we just have a "generic"
> configuration that VS sees? I don't think there's any actual switching we can
do
> in the IDE.

+1 for this. It could be called "GN". I don't know of any VS plugins that use
the configuration name for anything.

[brettw, 2016-02-24 23:38:16]

https://codereview.chromium.org/1733023002/diff/1/tools/gn/visual_studio_writ...
File tools/gn/visual_studio_writer.cc (right):

https://codereview.chromium.org/1733023002/diff/1/tools/gn/visual_studio_writ...
tools/gn/visual_studio_writer.cc:224: // between different toolchains.
Oh, I missed "target" and I thought you were talking about "current". You're
right.

[Tomasz Moniuszko, 2016-02-25 11:06:14]

https://codereview.chromium.org/1733023002/diff/1/tools/gn/visual_studio_writ...
File tools/gn/visual_studio_writer.cc (right):

https://codereview.chromium.org/1733023002/diff/1/tools/gn/visual_studio_writ...
tools/gn/visual_studio_writer.cc:227: const Value* is_debug_value =
scope->GetValue("is_debug");
On 2016/02/24 22:19:15, NGG wrote:
> On 2016/02/24 21:51:48, brettw wrote:
> > "is_debug" is a variable in the Chrome build, not something built into GN.
Can
> > we avoid making assumptions about such things here?
> > 
> > I realize you just moved existing code so this is partially a question for
> > tmoniuszko: What happens if we don't do this? Can we just have a "generic"
> > configuration that VS sees? I don't think there's any actual switching we
can
> do
> > in the IDE.
> 
> +1 for this. It could be called "GN". I don't know of any VS plugins that use
> the configuration name for anything.

Visual Studio allows to rename configuration to any name so I guess it should
work. I think it's convenient to have Debug/Release string in configuration name
(as I switch between debug/non-debug solutions often) but it's not critical.
Removing is_debug_config_ sgtm.

[NGG, 2016-02-25 14:06:26]

The CQ bit was checked by ngg@ngg.hu

[commit-bot: I haz the power, 2016-02-25 14:07:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1733023002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1733023002/1

[commit-bot: I haz the power, 2016-02-25 14:52:06]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-02-25 14:54:22]

Description was changed from

==========
GN: Fix CPU detection in Visual Studio generator

The previous version assumed that if there is no build override
for "current_cpu" then it means "x86".

R=brettw@chromium.org, tmoniuszko@opera.com
BUG=
==========

to

==========
GN: Fix CPU detection in Visual Studio generator

The previous version assumed that if there is no build override
for "current_cpu" then it means "x86".

R=brettw@chromium.org, tmoniuszko@opera.com
BUG=

Committed: https://crrev.com/5dd34fc85aab35792d7034ac3caf3a8f9f38ba8a
Cr-Commit-Position: refs/heads/master@{#377577}
==========

[commit-bot: I haz the power, 2016-02-25 14:54:23]

Patchset 1 (id:??) landed as
https://crrev.com/5dd34fc85aab35792d7034ac3caf3a8f9f38ba8a
Cr-Commit-Position: refs/heads/master@{#377577}

[XAX, 2016-02-25 16:06:22]

x-a-x@hotmail.co.th changed reviewers:
+ x-a-x@hotmail.co.th