Update merge_pgc_files.py in preparation for VS2017

Update merge_pgc_files.py in preparation for VS2017

pgomgr.exe has been moved to a different location in VS2017 so the logic
that find it should be updated.

It can't just be copied to the build directory because we always run the
x64 bit version of this binary (and we don't want to mix x86 and x64
binaries in the same build dir)

BUG=719319
Review-Url: https://codereview.chromium.org/2884613003
Cr-Commit-Position: refs/heads/master@{#473438}
Committed: https://chromium.googlesource.com/chromium/src/+/ab3a1821bb57a51d11be2b038b3a698236ccd5fc

[Sébastien Marchand, 2017-05-18 20:56:58]

Description was changed from

==========
Copy pgomgr.exe to the build directory.

This binary is required during a PGO build, this change is complementary
to https://codereview.chromium.org/2878693002/.

BUG=719319
==========

to

==========
Update merge_pgc_files.py in preparation for VS2017

pgomgr.exe has been moved to a different location in VS2017 so the logic
that find it should be updated.

It can't just be copied to the build directory because we always run the
x64 bit version of this binary (and we don't want to mix x86 and x64
binaries in the same build dir)

BUG=719319
==========

[Sébastien Marchand, 2017-05-18 20:56:58]

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

[Sébastien Marchand, 2017-05-18 20:58:51]

PTAL

[Sébastien Marchand, 2017-05-19 20:11:26]

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

[Sébastien Marchand, 2017-05-19 20:11:27]

+scottmg as Bruce seems to be OOO today.

[scottmg, 2017-05-19 20:25:10]

https://codereview.chromium.org/2884613003/diff/80001/build/vs_toolchain.py
File build/vs_toolchain.py (right):

https://codereview.chromium.org/2884613003/diff/80001/build/vs_toolchain.py#n...
build/vs_toolchain.py:221: def _FindVCToolsRoot():
Since you're calling this ouside of this script, it shouldn't be _ prefixed.

https://codereview.chromium.org/2884613003/diff/80001/build/vs_toolchain.py#n...
build/vs_toolchain.py:232: vc_tools_msvc_root =
os.path.join(os.environ.get('GYP_MSVS_OVERRIDE_PATH'),
(I know you just moved this, but since it's in a separate function now)

os.environ.get() is going to return None if it's not found. Maybe
assert('GYP_MSVS_OVERRIDE_PATH' in os.environ) on the previous line, and then
just os.environ['GYP_MSVS_OVERRIDE_PATH'] here.

[Sébastien Marchand, 2017-05-19 22:30:37]

Thanks, PTAnL.

https://codereview.chromium.org/2884613003/diff/80001/build/vs_toolchain.py
File build/vs_toolchain.py (right):

https://codereview.chromium.org/2884613003/diff/80001/build/vs_toolchain.py#n...
build/vs_toolchain.py:221: def _FindVCToolsRoot():
On 2017/05/19 20:25:10, scottmg wrote:
> Since you're calling this ouside of this script, it shouldn't be _ prefixed.

Done.

https://codereview.chromium.org/2884613003/diff/80001/build/vs_toolchain.py#n...
build/vs_toolchain.py:232: vc_tools_msvc_root =
os.path.join(os.environ.get('GYP_MSVS_OVERRIDE_PATH'),
On 2017/05/19 20:25:10, scottmg wrote:
> (I know you just moved this, but since it's in a separate function now)
> 
> os.environ.get() is going to return None if it's not found. Maybe
> assert('GYP_MSVS_OVERRIDE_PATH' in os.environ) on the previous line, and then
> just os.environ['GYP_MSVS_OVERRIDE_PATH'] here.

Sure, asserting seems like a good-enough solution.

[scottmg, 2017-05-19 22:46:19]

lgtm

https://codereview.chromium.org/2884613003/diff/100001/build/vs_toolchain.py
File build/vs_toolchain.py (right):

https://codereview.chromium.org/2884613003/diff/100001/build/vs_toolchain.py#...
build/vs_toolchain.py:240: return
This return is a bit weird. raise maybe (and remove the assert below)? Or if you
want to allow None, make it `return None`.

[Sébastien Marchand, 2017-05-19 23:27:11]

https://codereview.chromium.org/2884613003/diff/100001/build/vs_toolchain.py
File build/vs_toolchain.py (right):

https://codereview.chromium.org/2884613003/diff/100001/build/vs_toolchain.py#...
build/vs_toolchain.py:240: return
On 2017/05/19 22:46:19, scottmg wrote:
> This return is a bit weird. raise maybe (and remove the assert below)? Or if
you
> want to allow None, make it `return None`.

raising make sense.

[Sébastien Marchand, 2017-05-19 23:27:17]

The CQ bit was checked by sebmarchand@chromium.org

[Sébastien Marchand, 2017-05-19 23:27:17]

The patchset sent to the CQ was uploaded after l-g-t-m from scottmg@chromium.org
Link to the patchset: https://codereview.chromium.org/2884613003/#ps120001
(title: "raise")

[commit-bot: I haz the power, 2017-05-19 23:27:57]

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-19 23:37:04]

The CQ bit was unchecked by commit-bot@chromium.org

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

Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[Sébastien Marchand, 2017-05-19 23:50:17]

The CQ bit was checked by sebmarchand@chromium.org

[commit-bot: I haz the power, 2017-05-19 23:50:52]

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-20 04:21:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-20 04:21:33]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Sébastien Marchand, 2017-05-20 14:14:16]

The CQ bit was checked by sebmarchand@chromium.org

[commit-bot: I haz the power, 2017-05-20 14:14:26]

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-20 15:00:10]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1495289656908960,
"parent_rev": "26e4f40adaad88cf84fb4abb46311fb7a55e1970", "commit_rev":
"ab3a1821bb57a51d11be2b038b3a698236ccd5fc"}

[commit-bot: I haz the power, 2017-05-20 15:00:22]

Description was changed from

==========
Update merge_pgc_files.py in preparation for VS2017

pgomgr.exe has been moved to a different location in VS2017 so the logic
that find it should be updated.

It can't just be copied to the build directory because we always run the
x64 bit version of this binary (and we don't want to mix x86 and x64
binaries in the same build dir)

BUG=719319
==========

to

==========
Update merge_pgc_files.py in preparation for VS2017

pgomgr.exe has been moved to a different location in VS2017 so the logic
that find it should be updated.

It can't just be copied to the build directory because we always run the
x64 bit version of this binary (and we don't want to mix x86 and x64
binaries in the same build dir)

BUG=719319

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

[commit-bot: I haz the power, 2017-05-20 15:00:23]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/ab3a1821bb57a51d11be2b038b3a...

[Sébastien Marchand, 2017-05-22 15:06:42]

A revert of this CL (patchset #7 id:120001) has been created in
https://codereview.chromium.org/2901543002/ by sebmarchand@chromium.org.

The reason for reverting is: I think that it's causing crbug.com/724921 , I'll
investigate but this should probably be reverted in the meantime.

BUG=724921.