Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(426)

Issue 2884613003: Update merge_pgc_files.py in preparation for VS2017 (Closed)

Created:
3 years, 7 months ago by Sébastien Marchand
Modified:
3 years, 7 months ago
Reviewers:
brucedawson, scottmg
CC:
chromium-reviews
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

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

Patch Set 1 #

Patch Set 2 : fix #

Patch Set 3 : Copy pgomgr's dependencies. #

Patch Set 4 : Copy pgomgr's dependencies. #

Patch Set 5 : Copy pgomgr's dependencies. #

Total comments: 4

Patch Set 6 : Address Scott's comments. #

Total comments: 2

Patch Set 7 : raise #

Unified diffs Side-by-side diffs Delta from patch set Stats (+34 lines, -15 lines) Patch
M build/vs_toolchain.py View 1 2 3 4 5 6 2 chunks +23 lines, -14 lines 0 comments Download
M build/win/merge_pgc_files.py View 1 2 3 4 5 2 chunks +11 lines, -1 line 0 comments Download

Messages

Total messages: 24 (11 generated)
Sébastien Marchand
PTAL
3 years, 7 months ago (2017-05-18 20:58:51 UTC) #3
Sébastien Marchand
+scottmg as Bruce seems to be OOO today.
3 years, 7 months ago (2017-05-19 20:11:27 UTC) #5
scottmg
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#newcode221 build/vs_toolchain.py:221: def _FindVCToolsRoot(): Since you're calling this ouside of this ...
3 years, 7 months ago (2017-05-19 20:25:10 UTC) #6
Sébastien Marchand
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#newcode221 build/vs_toolchain.py:221: def _FindVCToolsRoot(): On 2017/05/19 20:25:10, scottmg wrote: ...
3 years, 7 months ago (2017-05-19 22:30:37 UTC) #7
scottmg
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#newcode240 build/vs_toolchain.py:240: return This return is a bit weird. raise ...
3 years, 7 months ago (2017-05-19 22:46:19 UTC) #8
Sébastien Marchand
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#newcode240 build/vs_toolchain.py:240: return On 2017/05/19 22:46:19, scottmg wrote: > This return ...
3 years, 7 months ago (2017-05-19 23:27:11 UTC) #9
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2884613003/120001
3 years, 7 months ago (2017-05-19 23:27:57 UTC) #12
commit-bot: I haz the power
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_compile_dbg/builds/273931) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, ...
3 years, 7 months ago (2017-05-19 23:37:05 UTC) #14
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2884613003/120001
3 years, 7 months ago (2017-05-19 23:50:52 UTC) #16
commit-bot: I haz the power
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_android_rel_ng/builds/298835)
3 years, 7 months ago (2017-05-20 04:21:33 UTC) #18
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2884613003/120001
3 years, 7 months ago (2017-05-20 14:14:26 UTC) #20
commit-bot: I haz the power
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/ab3a1821bb57a51d11be2b038b3a698236ccd5fc
3 years, 7 months ago (2017-05-20 15:00:23 UTC) #23
Sébastien Marchand
3 years, 7 months ago (2017-05-22 15:06:42 UTC) #24
Message was sent while issue was closed.
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.

Powered by Google App Engine
This is Rietveld 408576698