Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(7)

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 week, 6 days ago by Sébastien Marchand
Modified:
1 week, 1 day 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
Commit queue not available (can’t edit this change).

Messages

Total messages: 24 (11 generated)
Sébastien Marchand
PTAL
1 week, 3 days ago (2017-05-18 20:58:51 UTC) #3
Sébastien Marchand
+scottmg as Bruce seems to be OOO today.
1 week, 2 days 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 ...
1 week, 2 days 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: ...
1 week, 2 days 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 ...
1 week, 2 days 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 ...
1 week, 2 days 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
1 week, 2 days 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, ...
1 week, 2 days 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
1 week, 2 days 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)
1 week, 1 day 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
1 week, 1 day 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
1 week, 1 day ago (2017-05-20 15:00:23 UTC) #23
Sébastien Marchand
6 days, 12 hours 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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 650457f06