Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2030723002:
Handle case where WINDOWSSDKDIR is not set (Closed)
|
Created:
4 years, 6 months ago by brucedawson Modified:
4 years, 6 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionHandle case where WINDOWSSDKDIR is not set
If DEPOT_TOOLS_WIN_TOOLCHAIN=0 and vcvarsall.bat has not been run then
WINDOWSSDKDIR will probably not be set and copying cdb\cdb.exe will fail
when copy_cdb_to_output.py tries to read WINDOWSSDKDIR from environ.
This change handles that case by using the default SDK location.
BUG=616146
Committed: https://crrev.com/250abd283b7db0274df689e6b0dad571fd180a4f
Cr-Commit-Position: refs/heads/master@{#397483}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Remove if/else per code-review feedback #
Total comments: 2
Patch Set 3 : Fixing indentation #Messages
Total messages: 27 (10 generated)
brucedawson@chromium.org changed reviewers: + kbr@chromium.org
My tests show that this now works correctly without affecting cases where it previously worked. PTAL.
kbr@chromium.org changed reviewers: + scottmg@chromium.org
Thanks Bruce for picking this up. LGTM though CC'ing scottmg in case he has concerns about maintainability of this directory path. (Should it be provided by some other script?)
P.S. I'm not an OWNER in this directory.
Hardcoded path isn't super, but it's not the first place we have it, and something more complicated seems like overkill in this script, so lgtm. https://codereview.chromium.org/2030723002/diff/1/build/win/copy_cdb_to_outpu... File build/win/copy_cdb_to_output.py (right): https://codereview.chromium.org/2030723002/diff/1/build/win/copy_cdb_to_outpu... build/win/copy_cdb_to_output.py:61: win_sdk_dir = os.path.normpath(os.environ['WINDOWSSDKDIR']) replace the if/else with os.path.normpath(os.environ.get('WINDOWSSDKDIR', 'C:\\Program Files...'))
Hardcoded path isn't super, but it's not the first place we have it, and something more complicated seems like overkill in this script, so lgtm. https://codereview.chromium.org/2030723002/diff/1/build/win/copy_cdb_to_outpu... File build/win/copy_cdb_to_output.py (right): https://codereview.chromium.org/2030723002/diff/1/build/win/copy_cdb_to_outpu... build/win/copy_cdb_to_output.py:61: win_sdk_dir = os.path.normpath(os.environ['WINDOWSSDKDIR']) replace the if/else with os.path.normpath(os.environ.get('WINDOWSSDKDIR', 'C:\\Program Files...'))
https://codereview.chromium.org/2030723002/diff/1/build/win/copy_cdb_to_outpu... File build/win/copy_cdb_to_output.py (right): https://codereview.chromium.org/2030723002/diff/1/build/win/copy_cdb_to_outpu... build/win/copy_cdb_to_output.py:61: win_sdk_dir = os.path.normpath(os.environ['WINDOWSSDKDIR']) On 2016/06/01 21:07:35, scottmg wrote: > replace the if/else with os.path.normpath(os.environ.get('WINDOWSSDKDIR', > 'C:\\Program Files...')) Done.
The CQ bit was checked by brucedawson@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kbr@chromium.org, scottmg@chromium.org Link to the patchset: https://codereview.chromium.org/2030723002/#ps20001 (title: "Remove if/else per code-review feedback")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2030723002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2030723002/20001
https://codereview.chromium.org/2030723002/diff/20001/build/win/copy_cdb_to_o... File build/win/copy_cdb_to_output.py (right): https://codereview.chromium.org/2030723002/diff/20001/build/win/copy_cdb_to_o... build/win/copy_cdb_to_output.py:60: 'C:\\Program Files (x86)\\Windows Kits\\10')) This indent is wrong and makes it look like it's the second argument to normpath. It should be under 'WINDOWSSDKDIR'.
The CQ bit was unchecked by brucedawson@chromium.org
*Now* it's perfect! And it still works, with and without DEPOT_TOOLS_WIN_TOOLCHAIN=0. https://codereview.chromium.org/2030723002/diff/20001/build/win/copy_cdb_to_o... File build/win/copy_cdb_to_output.py (right): https://codereview.chromium.org/2030723002/diff/20001/build/win/copy_cdb_to_o... build/win/copy_cdb_to_output.py:60: 'C:\\Program Files (x86)\\Windows Kits\\10')) On 2016/06/01 23:38:05, scottmg wrote: > This indent is wrong and makes it look like it's the second argument to > normpath. It should be under 'WINDOWSSDKDIR'. Uggh. Yeah. I was apparently too focused on looking up the correct indication for the previous line in the Python style guide. Okay, fixed.
lgtm!
The CQ bit was checked by brucedawson@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kbr@chromium.org Link to the patchset: https://codereview.chromium.org/2030723002/#ps40001 (title: "Fixing indentation")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2030723002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2030723002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2016/06/02 02:23:19, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) """<H1>Over Quota</H1> This application is temporarily over its serving quota. Please try again later.""" I guess I'll try later.
On 2016/06/02 17:02:42, brucedawson wrote: > On 2016/06/02 02:23:19, commit-bot: I haz the power wrote: > > Try jobs failed on following builders: > > linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, > > > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) > > """<H1>Over Quota</H1> > This application is temporarily over its serving quota. Please try again > later.""" > > I guess I'll try later. That seems to have been resolved now.
The CQ bit was checked by brucedawson@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2030723002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2030723002/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Handle case where WINDOWSSDKDIR is not set If DEPOT_TOOLS_WIN_TOOLCHAIN=0 and vcvarsall.bat has not been run then WINDOWSSDKDIR will probably not be set and copying cdb\cdb.exe will fail when copy_cdb_to_output.py tries to read WINDOWSSDKDIR from environ. This change handles that case by using the default SDK location. BUG=616146 ========== to ========== Handle case where WINDOWSSDKDIR is not set If DEPOT_TOOLS_WIN_TOOLCHAIN=0 and vcvarsall.bat has not been run then WINDOWSSDKDIR will probably not be set and copying cdb\cdb.exe will fail when copy_cdb_to_output.py tries to read WINDOWSSDKDIR from environ. This change handles that case by using the default SDK location. BUG=616146 Committed: https://crrev.com/250abd283b7db0274df689e6b0dad571fd180a4f Cr-Commit-Position: refs/heads/master@{#397483} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/250abd283b7db0274df689e6b0dad571fd180a4f Cr-Commit-Position: refs/heads/master@{#397483} |
