Fix to vs_toolchain.FindVCToolsRoot

Fix to vs_toolchain.FindVCToolsRoot

This makes sure that the VS environment is always properly set when
calling vs_toolchain.FindVCToolsRoot from the bots.

Review-Url: https://codereview.chromium.org/2893293004
Cr-Commit-Position: refs/heads/master@{#473784}
Committed: https://chromium.googlesource.com/chromium/src/+/f44f50c9aaa57c68b4de5b23b2a8433d40ca98a2

[Sébastien Marchand, 2017-05-22 21:43:07]

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

[Sébastien Marchand, 2017-05-22 21:43:08]

PTAL

[brucedawson, 2017-05-22 21:47:08]

Description was changed from

==========
Fix to vs_toolchain.FindVCToolsRoot

This makes sure that the VS environement is always properly set when
calling vs_toolchain.FindVCToolsRoot from the bots.
==========

to

==========
Fix to vs_toolchain.FindVCToolsRoot

This makes sure that the VS environment is always properly set when
calling vs_toolchain.FindVCToolsRoot from the bots.
==========

[brucedawson, 2017-05-22 21:47:14]

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

https://codereview.chromium.org/2893293004/diff/1/build/vs_toolchain.py#newco...
build/vs_toolchain.py:232: SetEnvironmentAndGetRuntimeDllDirs()
Placing this call here means that the environment is set when you call
_CopyPGORuntime if-and-only-if you are building with VS 2017. It seems like it
would be more obvious if it was called directly and unconditionally from
_CopyPGORuntime. Thoughts?

[Sébastien Marchand, 2017-05-22 22:02:59]

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

https://codereview.chromium.org/2893293004/diff/1/build/vs_toolchain.py#newco...
build/vs_toolchain.py:232: SetEnvironmentAndGetRuntimeDllDirs()
On 2017/05/22 21:47:14, brucedawson wrote:
> Placing this call here means that the environment is set when you call
> _CopyPGORuntime if-and-only-if you are building with VS 2017. It seems like it
> would be more obvious if it was called directly and unconditionally from
> _CopyPGORuntime. Thoughts?

The environment is already always set when you call _CopyPGORuntime because this
only get called by CopyDlls, which calls SetEnvironmentAndGetRuntimeDllDirs.

The only issue that I could see with this function is that it leak some
environment variable, I probably could save the environment at the beginning of
the function and restore it before returning, but I'm not sure if it's really
worth it? 

This function (FindVCToolsRoot) gets called by the merge_pgc_files script, so
another option would be to call SetEnvironment... from this script instead of
doing it here?

[brucedawson, 2017-05-22 23:48:45]

lgtm either as is or if you want to move the call to the other script.

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

https://codereview.chromium.org/2893293004/diff/1/build/vs_toolchain.py#newco...
build/vs_toolchain.py:232: SetEnvironmentAndGetRuntimeDllDirs()
Ah - I understand.

I can see a case for calling SetEnvironment from the other script, unless it
just feels like that is postponing confusion.

I'm not sure I'd worry about environmental pollution.

[Sébastien Marchand, 2017-05-23 00:39:45]

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

https://codereview.chromium.org/2893293004/diff/1/build/vs_toolchain.py#newco...
build/vs_toolchain.py:232: SetEnvironmentAndGetRuntimeDllDirs()
On 2017/05/22 23:48:45, brucedawson wrote:
> Ah - I understand.
> 
> I can see a case for calling SetEnvironment from the other script, unless it
> just feels like that is postponing confusion.
> 
> I'm not sure I'd worry about environmental pollution.

I'll keep this call here then, relying on GYP_MSVS_OVERRIDE path being set seems
like an implementation detail that I don't want the callers to have to know.

[Sébastien Marchand, 2017-05-23 00:39:51]

The CQ bit was checked by sebmarchand@chromium.org

[commit-bot: I haz the power, 2017-05-23 00:40:24]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-23 02:08:37]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1495499991157760, "parent_rev":
"9fb6996d562bb75de00bc77d809530ff65404adb", "commit_rev":
"f44f50c9aaa57c68b4de5b23b2a8433d40ca98a2"}

[commit-bot: I haz the power, 2017-05-23 02:08:49]

Description was changed from

==========
Fix to vs_toolchain.FindVCToolsRoot

This makes sure that the VS environment is always properly set when
calling vs_toolchain.FindVCToolsRoot from the bots.
==========

to

==========
Fix to vs_toolchain.FindVCToolsRoot

This makes sure that the VS environment is always properly set when
calling vs_toolchain.FindVCToolsRoot from the bots.

Review-Url: https://codereview.chromium.org/2893293004
Cr-Commit-Position: refs/heads/master@{#473784}
Committed:
https://chromium.googlesource.com/chromium/src/+/f44f50c9aaa57c68b4de5b23b2a8...
==========

[commit-bot: I haz the power, 2017-05-23 02:08:50]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/f44f50c9aaa57c68b4de5b23b2a8...