Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1598493004:
Change default Windows compiler to VS 2015 (Closed)
|
Created:
4 years, 11 months ago by brucedawson Modified:
4 years, 10 months ago CC:
chromium-reviews, danakj, Sébastien Marchand Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionChange default Windows compiler to VS 2015
The change to get_landmines.py is there because modifying this file
affects analyze behavior so that all tests run. Changing the printed
message is purely a side effect.
This change also removes some redundant INCLUDE paths. These are
unnecessary when building with VS 2015 (because it defaults to the
Windows 10 SDK) and actively harmful (they make the INCLUDE path
problematically long).
BUG=440500, 584782
Committed: https://crrev.com/4c17ac0db59d7dbe78734950f83912ab564bd6fe
Cr-Commit-Position: refs/heads/master@{#373955}
Patch Set 1 #Patch Set 2 : Modify build_config.h to force rebuild, and re-fix vs_toolchain.py #Patch Set 3 : List compiler version being used. #Patch Set 4 : Remove test code and modify get_landmines.py to trigger all tests #Patch Set 5 : Remove whitespace change to build_config.h #
Total comments: 3
Patch Set 6 : New test hash for VS 2015 - no UCRT! #Patch Set 7 : Update to latest #Patch Set 8 : Switch to VM created package with 'if x64' fix #Patch Set 9 : Update to latest origin/master #Patch Set 10 : Remove duplicate include paths and explain in comments #
Total comments: 2
Messages
Total messages: 35 (14 generated)
Description was changed from ========== Change default Windows compiler to VS 2015 BUG=440500 ========== to ========== Change default Windows compiler to VS 2015 The change to get_landmines.py is there because modifying this file affects analyze behavior so that all tests run. Changing the printed message is purely a side effect. BUG=440500 ==========
brucedawson@chromium.org changed reviewers: + dpranke@chromium.org, scottmg@chromium.org
PTAL. I'm still waiting on the final bot configs to be updated, but the results are looking bizarrely good. dpranke@ - can you confirm that touching build\get_landmines.py will trigger all tests? scottmg@ - any last minute concerns, other than bot updates? Big change...
Do it! lgtm
The CQ bit was checked by brucedawson@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1598493004/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1598493004/80001
lgtm. changing get_landmines.py should cause all the tests to be run on the tryjobs. https://codereview.chromium.org/1598493004/diff/80001/build/get_landmines.py File build/get_landmines.py (right): https://codereview.chromium.org/1598493004/diff/80001/build/get_landmines.py#... build/get_landmines.py:55: print "Switch to VS2013" not sure why you changed this; just to simplify the output?
https://codereview.chromium.org/1598493004/diff/80001/build/get_landmines.py File build/get_landmines.py (right): https://codereview.chromium.org/1598493004/diff/80001/build/get_landmines.py#... build/get_landmines.py:55: print "Switch to VS2013" On 2016/01/22 22:17:06, Dirk Pranke (slow) wrote: > not sure why you changed this; just to simplify the output? Well, I had to change something in order to trigger the tests, and this seemed like a reasonable choice. I could do a comment change or some-such if that would be less confusing. I wish the old prints had been renamed as new ones were added since the current output is slightly confusing on first viewing.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
https://codereview.chromium.org/1598493004/diff/80001/build/get_landmines.py File build/get_landmines.py (right): https://codereview.chromium.org/1598493004/diff/80001/build/get_landmines.py#... build/get_landmines.py:55: print "Switch to VS2013" On 2016/01/23 00:47:48, brucedawson wrote: > On 2016/01/22 22:17:06, Dirk Pranke (slow) wrote: > > not sure why you changed this; just to simplify the output? > > Well, I had to change something in order to trigger the tests, and this seemed > like a reasonable choice. I could do a comment change or some-such if that would > be less confusing. > > I wish the old prints had been renamed as new ones were added since the current > output is slightly confusing on first viewing. Agreed, and makes sense.
The CQ bit was checked by brucedawson@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dpranke@chromium.org, scottmg@chromium.org Link to the patchset: https://codereview.chromium.org/1598493004/#ps160001 (title: "Update to latest origin/master")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1598493004/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1598493004/160001
I like this CL and would like to subscribe to your newsletter.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
Description was changed from ========== Change default Windows compiler to VS 2015 The change to get_landmines.py is there because modifying this file affects analyze behavior so that all tests run. Changing the printed message is purely a side effect. BUG=440500 ========== to ========== Change default Windows compiler to VS 2015 The change to get_landmines.py is there because modifying this file affects analyze behavior so that all tests run. Changing the printed message is purely a side effect. BUG=440500,584782 ==========
The goma failures are now well understood and this CL now includes a fix. PTAL. The failures were caused by a goma limitation (caused by wine?) of 1K for the length of environment variables. sebmarchand@'s toolchain download change added 410 bytes to the INCLUDE variable (369 for VS 2013) which caused goma to fail. The new INCLUDE variable was over the limit for VS 2013 as well but the trailing paths were (apparently) not critical so this was not noticed. The other reason the include variable is so long is because several directories are listed twice - once from SetEnv.cmd/vcvarsall.bat and once from common.gypi/setup_toolchain.py. This duplication is necessary with VS 2013 because it is the only way to get the Windows 10 SDK at the front of the include path. It is *not* necessary with VS 2015, and right now it is causing problems. So... In addition to switching to VS 2015 this also removes four INCLUDE entries. This gets us well under the 1K limit. Additionally the goma team will be raising the limit to 2K, probably early next week.
Also... I have tested local gn builds, and gyp builds with DEPOT_TOOLS_TOOLCHAIN=0 and they build with no problems. If developers force VS 2013 and try to use DEPOT_TOOLS_TOOLCHAIN=0 then builds will fail because the Windows 8.1 SDK will be used. This is unfortunate but difficult to avoid. This will not affect people using DEPOT_TOOLS_TOOLCHAIN=1 (the default) and it will not affect developers if we revert this change, it only affects setting GYP_MSVS_VERSION=2013 after this change lands, which will quickly be impossible anyway. If this is a problem then we could restore the duplicate paths after goma is fixed, or could make this change VS 2015 only.
https://codereview.chromium.org/1598493004/diff/180001/build/common.gypi File build/common.gypi (left): https://codereview.chromium.org/1598493004/diff/180001/build/common.gypi#oldc... build/common.gypi:5676: '<(windows_sdk_path)/Include/10.0.10586.0/shared', Won't these be needed for DEPOT_TOOLS_WIN_TOOLCHAIN=0?
On 2016/02/05 21:52:45, scottmg (afk until tues 9feb) wrote: > https://codereview.chromium.org/1598493004/diff/180001/build/common.gypi > File build/common.gypi (left): > > https://codereview.chromium.org/1598493004/diff/180001/build/common.gypi#oldc... > build/common.gypi:5676: '<(windows_sdk_path)/Include/10.0.10586.0/shared', > Won't these be needed for DEPOT_TOOLS_WIN_TOOLCHAIN=0? Oops, missed your update. OK, LGTM, bombs away.
sebmarchand@chromium.org changed reviewers: + sebmarchand@chromium.org
fire in the hole ! (lgtm)
Description was changed from ========== Change default Windows compiler to VS 2015 The change to get_landmines.py is there because modifying this file affects analyze behavior so that all tests run. Changing the printed message is purely a side effect. BUG=440500,584782 ========== to ========== Change default Windows compiler to VS 2015 The change to get_landmines.py is there because modifying this file affects analyze behavior so that all tests run. Changing the printed message is purely a side effect. This change also removes some redundant INCLUDE paths. These are unnecessary when building with VS 2015 (because it defaults to the Windows 10 SDK) and actively harmful (they make the INCLUDE path problematically long). BUG=440500,584782 ==========
https://codereview.chromium.org/1598493004/diff/180001/build/common.gypi File build/common.gypi (left): https://codereview.chromium.org/1598493004/diff/180001/build/common.gypi#oldc... build/common.gypi:5676: '<(windows_sdk_path)/Include/10.0.10586.0/shared', On 2016/02/05 21:52:45, scottmg (afk until tues 9feb) wrote: > Won't these be needed for DEPOT_TOOLS_WIN_TOOLCHAIN=0? Only with VS 2013, which this switches away from (commenting here for posterity).
The CQ bit was checked by brucedawson@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dpranke@chromium.org Link to the patchset: https://codereview.chromium.org/1598493004/#ps180001 (title: "Remove duplicate include paths and explain in comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1598493004/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1598493004/180001
Message was sent while issue was closed.
Description was changed from ========== Change default Windows compiler to VS 2015 The change to get_landmines.py is there because modifying this file affects analyze behavior so that all tests run. Changing the printed message is purely a side effect. This change also removes some redundant INCLUDE paths. These are unnecessary when building with VS 2015 (because it defaults to the Windows 10 SDK) and actively harmful (they make the INCLUDE path problematically long). BUG=440500,584782 ========== to ========== Change default Windows compiler to VS 2015 The change to get_landmines.py is there because modifying this file affects analyze behavior so that all tests run. Changing the printed message is purely a side effect. This change also removes some redundant INCLUDE paths. These are unnecessary when building with VS 2015 (because it defaults to the Windows 10 SDK) and actively harmful (they make the INCLUDE path problematically long). BUG=440500,584782 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Change default Windows compiler to VS 2015 The change to get_landmines.py is there because modifying this file affects analyze behavior so that all tests run. Changing the printed message is purely a side effect. This change also removes some redundant INCLUDE paths. These are unnecessary when building with VS 2015 (because it defaults to the Windows 10 SDK) and actively harmful (they make the INCLUDE path problematically long). BUG=440500,584782 ========== to ========== Change default Windows compiler to VS 2015 The change to get_landmines.py is there because modifying this file affects analyze behavior so that all tests run. Changing the printed message is purely a side effect. This change also removes some redundant INCLUDE paths. These are unnecessary when building with VS 2015 (because it defaults to the Windows 10 SDK) and actively harmful (they make the INCLUDE path problematically long). BUG=440500,584782 Committed: https://crrev.com/4c17ac0db59d7dbe78734950f83912ab564bd6fe Cr-Commit-Position: refs/heads/master@{#373955} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/4c17ac0db59d7dbe78734950f83912ab564bd6fe Cr-Commit-Position: refs/heads/master@{#373955}
Message was sent while issue was closed.
On 2016/02/05 23:55:18, commit-bot: I haz the power wrote: > Patchset 10 (id:??) landed as > https://crrev.com/4c17ac0db59d7dbe78734950f83912ab564bd6fe > Cr-Commit-Position: refs/heads/master@{#373955} 🎉 yay!
Message was sent while issue was closed.
A revert of this CL (patchset #10 id:180001) has been created in https://codereview.chromium.org/1678663002/ by thakis@chromium.org. The reason for reverting is: Speculative for https://code.google.com/p/chromium/issues/detail?id=498544#c20 . Will reland if this doesn't help.. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
