Update the toolchain if there's a mismatch between the env and win_toolchain.json

Update the toolchain if there's a mismatch between the env and win_toolchain.json

BUG=569152
Committed: https://crrev.com/e44b02e42a8b593a83bc569240923d0e6faa64d0
Cr-Commit-Position: refs/heads/master@{#369867}

[Sébastien Marchand, 2016-01-15 00:49:15]

sebmarchand@chromium.org changed reviewers:
+ brucedawson@chromium.org, scottmg@chromium.org

[Sébastien Marchand, 2016-01-15 00:49:15]

PTAL. Not sure if it'll fix the problem or if it's a silly idea (I'm don't
really know what's the purpose of the win_toolchain.json file here).

[brucedawson, 2016-01-15 00:55:39]

On 2016/01/15 00:49:15, Sébastien Marchand wrote:
> PTAL. Not sure if it'll fix the problem or if it's a silly idea (I'm don't
> really know what's the purpose of the win_toolchain.json file here).

I'm a bit unclear also. Scott?

[scottmg, 2016-01-15 01:05:19]

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

https://codereview.chromium.org/1582173003/diff/1/build/vs_toolchain.py#newco...
build/vs_toolchain.py:36: if not os.path.exists(json_data_file):
I don't think overriding the version that's coming from the .json file is right.
It's just a cache of the data that the toolchain zip stored, so you could end up
say that you're building with 2015 when you're actually building with 2013.

Woudl it work to instead check if the json matches the environment up here, and
if it doesn't then call Update()? I vaguely remember that maybe that's what this
if not exists is here for.

[Sébastien Marchand, 2016-01-15 20:40:50]

Description was changed from

==========
vs_toolchain.py: Always use the value of GYP_MSVS_VERSION from the environment.

BUG=569152
==========

to

==========
Update the toolchain if there's a mismatch between the env and
win_toolchain.json

BUG=569152
==========

[Sébastien Marchand, 2016-01-15 20:55:43]

Patchset #2 (id:20001) has been deleted

[Sébastien Marchand, 2016-01-15 20:58:39]

This should be better :), I've tried it locally and it seems to work, here's my
test scenario:
- delete .landmines and unset the GYP_MSVS_VERSION var.
- gclient runhooks, check that the .landmines file refers to VS2013.
- set GYP_MSVS_VERSION=2015
- gclient runhooks, this time check for VS2015 in the .landmines file.
- Unset GYP_MSVS_VERSION
- gclient runhooks, check for VS2013 in .landmines

[scottmg, 2016-01-15 21:08:13]

Yeah, that seems better. lgtm, maybe Bruce will suggestions of something to
test/try.

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

https://codereview.chromium.org/1582173003/diff/40001/build/vs_toolchain.py#n...
build/vs_toolchain.py:280: if version != env_version:
return version != env_version

[brucedawson, 2016-01-15 21:10:52]

Another test (which I think your change will pass): run "gclient runhooks" twice
after changing GYP_MSVS_VERSION. When landmines.py is misbehaving the landmine
triggers on the second run, which is inefficient and confusing.

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

https://codereview.chromium.org/1582173003/diff/40001/build/vs_toolchain.py#n...
build/vs_toolchain.py:25: CURRENT_TOOLCHAIN_VERSION = '2013'
Should probably be "CURRENT_DEFAULT_TOOLCHAIN_VERSION", to better reflect its
actual usage.

[Sébastien Marchand, 2016-01-15 21:21:40]

Yeah, I've run runhooks multiple time and it works as expected.

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

https://codereview.chromium.org/1582173003/diff/40001/build/vs_toolchain.py#n...
build/vs_toolchain.py:25: CURRENT_TOOLCHAIN_VERSION = '2013'
On 2016/01/15 21:10:52, brucedawson wrote:
> Should probably be "CURRENT_DEFAULT_TOOLCHAIN_VERSION", to better reflect its
> actual usage.

Done.

https://codereview.chromium.org/1582173003/diff/40001/build/vs_toolchain.py#n...
build/vs_toolchain.py:280: if version != env_version:
On 2016/01/15 21:08:13, scottmg wrote:
> return version != env_version

Done.

[brucedawson, 2016-01-15 21:34:08]

Somewhat related: depot_tools\win_toolchain\get_toolchain_if_necessary.py also
examines GYP_MSVS_VERSION. So, changing the default requires one of:

1) build\vs_toolchain.py setting GYP_MSVS_VERSION
2) build\vs_toolchain.py passing GYP_MSVS_VERSION to
get_toolchain_if_necessary.py on the command-line
3) Simultaneous updates to two repos

Option #3 seems like a bad idea. Option #2 is cleanest, but I'm not sure if it
is sufficient - if other places also need the value passed along. Any thoughts?
It seems at least tangentially related to this change.

On the other hand, probably simplest to land this improvement rather than trying
to hold so many balls in the air.

lgtm, but consider changing CURRENT_TOOLCHAIN_VERSION to
CURRENT_DEFAULT_TOOLCHAIN_VERSION to better reflect its meaning.

[scottmg, 2016-01-15 21:38:17]

I think ideally we'd delete all the version override stuff and just have "the
toolchain" as pointed to by the hashes. The version override was always a hack.

e.g. Does GN really care? Presumably all it really needs is the paths to the
toolchain and include dirs anyway.

[Sébastien Marchand, 2016-01-15 21:55:15]

Rebased and confirmed that it's still working, so I'll land this.

+1 to removing the version number and using only the hash.

[Sébastien Marchand, 2016-01-15 21:55:27]

The CQ bit was checked by sebmarchand@chromium.org

[Sébastien Marchand, 2016-01-15 21:55:28]

The patchset sent to the CQ was uploaded after l-g-t-m from
scottmg@chromium.org, brucedawson@chromium.org
Link to the patchset: https://codereview.chromium.org/1582173003/#ps100001
(title: "Rebase.")

[commit-bot: I haz the power, 2016-01-15 21:56:50]

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

[commit-bot: I haz the power, 2016-01-15 22:30:03]

Description was changed from

==========
Update the toolchain if there's a mismatch between the env and
win_toolchain.json

BUG=569152
==========

to

==========
Update the toolchain if there's a mismatch between the env and
win_toolchain.json

BUG=569152
==========

[commit-bot: I haz the power, 2016-01-15 22:30:04]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2016-01-15 22:30:59]

Description was changed from

==========
Update the toolchain if there's a mismatch between the env and
win_toolchain.json

BUG=569152
==========

to

==========
Update the toolchain if there's a mismatch between the env and
win_toolchain.json

BUG=569152

Committed: https://crrev.com/e44b02e42a8b593a83bc569240923d0e6faa64d0
Cr-Commit-Position: refs/heads/master@{#369867}
==========

[commit-bot: I haz the power, 2016-01-15 22:31:00]

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