Ensure GYP_MSVS_VERSION is set for GN build.

Ensure GYP_MSVS_VERSION is set for GN build.

BUG=460462
Committed: https://crrev.com/815e177eeae85e09c9ad93bca88cd4d299a62e19
Cr-Commit-Position: refs/heads/master@{#369079}

[haltonhuo, 2016-01-12 08:31:23]

halton.huo@intel.com changed reviewers:
+ bratell@opera.com, brucedawson@chromium.org, scottmg@chromium.org

[haltonhuo, 2016-01-12 08:31:23]

This CL is follow up one for https://codereview.chromium.org/1556993002 to fix
the GYP_MSVS_VERSION is not set issue, PTAL.

[Daniel Bratell, 2016-01-12 08:42:02]

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

https://codereview.chromium.org/1580703002/diff/1/build/vs_toolchain.py#newco...
build/vs_toolchain.py:110: os.environ['GYP_MSVS_VERSION'] = version_as_year
From a code maintenance point of view it doesn't seem right that the "Detect"
function has side effects on the environment. 

What do you think about moving it to _SetupScript in setup_toolchain.py and to
SetEnvironmentAndGetRuntimeDllDirs in vs_toolchain.py, or to a new shared helper
function?

[haltonhuo, 2016-01-12 09:57:47]

On 2016/01/12 08:42:02, Daniel Bratell wrote:
> https://codereview.chromium.org/1580703002/diff/1/build/vs_toolchain.py
> File build/vs_toolchain.py (right):
> 
>
https://codereview.chromium.org/1580703002/diff/1/build/vs_toolchain.py#newco...
> build/vs_toolchain.py:110: os.environ['GYP_MSVS_VERSION'] = version_as_year
> From a code maintenance point of view it doesn't seem right that the "Detect"
> function has side effects on the environment. 
> 
> What do you think about moving it to _SetupScript in setup_toolchain.py and to
> SetEnvironmentAndGetRuntimeDllDirs in vs_toolchain.py, or to a new shared
helper
> function?

I prefer a new helper function, will upload a new patch set.

[haltonhuo, 2016-01-12 10:17:47]

On 2016/01/12 09:57:47, haltonhuo wrote:
> > What do you think about moving it to _SetupScript in setup_toolchain.py and
to
> > SetEnvironmentAndGetRuntimeDllDirs in vs_toolchain.py, or to a new shared
> helper
> > function?
> 
> I prefer a new helper function, will upload a new patch set.

Daniel, patch set2 is using new helper function GetVisualStudioVersion(), PTAL.

[Daniel Bratell, 2016-01-12 10:35:05]

On 2016/01/12 10:17:47, haltonhuo wrote:
> On 2016/01/12 09:57:47, haltonhuo wrote:
> > > What do you think about moving it to _SetupScript in setup_toolchain.py
and
> to
> > > SetEnvironmentAndGetRuntimeDllDirs in vs_toolchain.py, or to a new shared
> > helper
> > > function?
> > 
> > I prefer a new helper function, will upload a new patch set.
> 
> Daniel, patch set2 is using new helper function GetVisualStudioVersion(),
PTAL.

lgtm (non-owner)

At some time (in a year or two maybe) we'll want to strip everything GYP from
the tree so this is a useful step in that direction as well.

[scottmg, 2016-01-12 20:56:44]

It'd probably be better if GN didn't depend on *GYP_*MSVS_VERSION, but I guess
that window's already broken, so lgtm.

https://codereview.chromium.org/1580703002/diff/20001/build/vs_toolchain.py
File build/vs_toolchain.py (right):

https://codereview.chromium.org/1580703002/diff/20001/build/vs_toolchain.py#n...
build/vs_toolchain.py:104: 
Remove blank line.

https://codereview.chromium.org/1580703002/diff/20001/build/vs_toolchain.py#n...
build/vs_toolchain.py:115: # Default to Visual Studio 2013 for now.
Remove this comment, it's in the function body now.

[haltonhuo, 2016-01-13 01:08:36]

On 2016/01/12 20:56:44, scottmg wrote:
> It'd probably be better if GN didn't depend on *GYP_*MSVS_VERSION, but I guess
> that window's already broken, so lgtm.
> 
> https://codereview.chromium.org/1580703002/diff/20001/build/vs_toolchain.py
> File build/vs_toolchain.py (right):
> 
>
https://codereview.chromium.org/1580703002/diff/20001/build/vs_toolchain.py#n...
> build/vs_toolchain.py:104: 
> Remove blank line.
> 
>
https://codereview.chromium.org/1580703002/diff/20001/build/vs_toolchain.py#n...
> build/vs_toolchain.py:115: # Default to Visual Studio 2013 for now.
> Remove this comment, it's in the function body now.

patch set3 upload to reflect your comments.

[haltonhuo, 2016-01-13 01:08:46]

The CQ bit was checked by halton.huo@intel.com

[haltonhuo, 2016-01-13 01:08:46]

The patchset sent to the CQ was uploaded after l-g-t-m from bratell@opera.com,
scottmg@chromium.org
Link to the patchset: https://codereview.chromium.org/1580703002/#ps40001
(title: "nit update on scottmg's review")

[commit-bot: I haz the power, 2016-01-13 01:09:02]

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

[brucedawson, 2016-01-13 01:43:16]

I guess it's too late to say that I found a bug in this CL...

I tried modifying it to default to VS 2015 and that revealed that the behavior
is incorrect when depot_tools\win_toolchain\get_toolchain_if_necessary.py is
invoked. Because GYP_MSVS_VERSION is not actually set to VS2015 it downloads the
package to vs2013_files instead of to vs_files.

This isn't a practical problem until I change the default, but at that point we
may have to set GYP_MSVS_VERSION, as in the original patch set for this CL.
That's what I ended up doing in a separate CL I'm working on.

[haltonhuo, 2016-01-13 01:51:15]

On 2016/01/13 01:43:16, brucedawson wrote:
> I guess it's too late to say that I found a bug in this CL...
> 
> I tried modifying it to default to VS 2015 and that revealed that the behavior
> is incorrect when depot_tools\win_toolchain\get_toolchain_if_necessary.py is
> invoked. Because GYP_MSVS_VERSION is not actually set to VS2015 it downloads
the
> package to vs2013_files instead of to vs_files.
> 
> This isn't a practical problem until I change the default, but at that point
we
> may have to set GYP_MSVS_VERSION, as in the original patch set for this CL.
> That's what I ended up doing in a separate CL I'm working on.

Sorry about that, I do not have VS 2015 environment, looking forward to see your
CL.

[commit-bot: I haz the power, 2016-01-13 02:23:38]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-01-13 02:24:54]

Description was changed from

==========
Ensure GYP_MSVS_VERSION is set for GN build.

BUG=460462
==========

to

==========
Ensure GYP_MSVS_VERSION is set for GN build.

BUG=460462

Committed: https://crrev.com/815e177eeae85e09c9ad93bca88cd4d299a62e19
Cr-Commit-Position: refs/heads/master@{#369079}
==========

[commit-bot: I haz the power, 2016-01-13 02:24:54]

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

[scottmg, 2016-01-13 02:55:19]

On 2016/01/13 01:43:16, brucedawson wrote:
> I guess it's too late to say that I found a bug in this CL...
> 
> I tried modifying it to default to VS 2015 and that revealed that the behavior
> is incorrect when depot_tools\win_toolchain\get_toolchain_if_necessary.py is
> invoked. Because GYP_MSVS_VERSION is not actually set to VS2015 it downloads
the
> package to vs2013_files instead of to vs_files.
> 
> This isn't a practical problem until I change the default, but at that point
we
> may have to set GYP_MSVS_VERSION, as in the original patch set for this CL.
> That's what I ended up doing in a separate CL I'm working on.

Oops, sorry. I should have left this for you. Feel free to revert if it's
causing any trouble with the test switch.

[brucedawson, 2016-01-13 18:34:44]

> Oops, sorry. I should have left this for you. Feel free to revert if it's
> causing any trouble with the test switch.

No worries. It was just unfortunate/amusing timing that I realized the problem
just a few minutes after the commit button was checked.

It won't affect anyone else and I'll make sure that the CL that makes the switch
handles this correctly.