Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1609933004:
Skip include\ucrt on VS 2013 packages (Closed)
|
Created:
4 years, 11 months ago by brucedawson Modified:
4 years, 10 months ago Reviewers:
scottmg CC:
chromium-reviews, dpranke+depot_tools_chromium.org, iannucci+depot_tools_chromium.org Base URL:
https://chromium.googlesource.com/chromium/tools/depot_tools.git@master Target Ref:
refs/heads/master Project:
depot_tools Visibility:
Public. |
DescriptionSkip include\ucrt on VS 2013 packages
In change crrev.com/1504983002 the include\ucrt path from
the Windows 10 SDK was added to the include search path,
but this is not a legal thing to do on VS 2013. This change
makes the ucrt path VS 2015 specific.
Testing shows that this makes no difference to the VS 2015
package.
BUG=440500
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=298330
Patch Set 1 #Patch Set 2 : Fix VS 2013 packaging #Patch Set 3 : Remove pointless .replace() call #
Total comments: 5
Messages
Total messages: 16 (4 generated)
Description was changed from ========== Skip include\ucrt on VS 2013 packages ========== to ========== Skip include\ucrt on VS 2013 packages In change crrev.com/1504983002 the include\ucrt path from the Windows 10 SDK was added to the include search path, but this is not a legal thing to do on VS 2013. This change makes the ucrt path VS 2015 specific. Testing shows that this makes no difference to the VS 2015 package. BUG=440500 ==========
brucedawson@chromium.org changed reviewers: + scottmg@chromium.org
Arguably we shouldn't even be packaging the ucrt directories on VS 2013, but that's a larger change that can't be justified. This allows me to create a VS 2013/SDK 10586 package, and avoids leaving the script in a partially broken state. We never actually use this fix, but it's good to have the option. The problem would probably have been avoided if we put the CRT include directories first, which is what vcvarsall.bat does. Any idea why we have them last?
I assume it doesn't matter too much on 2013 since the directory won't exist anyway? On 2016/01/21 00:41:28, brucedawson wrote: > Arguably we shouldn't even be packaging the ucrt directories on VS 2013, but > that's a larger change that can't be justified. > > This allows me to create a VS 2013/SDK 10586 package, and avoids leaving the > script in a partially broken state. > > We never actually use this fix, but it's good to have the option. > > The problem would probably have been avoided if we put the CRT include > directories first, which is what vcvarsall.bat does. Any idea why we have them > last? Nope, no idea. I'm a proponent of not depending on include search path order, but I guess it matters here sadly?
https://codereview.chromium.org/1609933004/diff/40001/win_toolchain/package_f... File win_toolchain/package_from_installed.py (right): https://codereview.chromium.org/1609933004/diff/40001/win_toolchain/package_f... win_toolchain/package_from_installed.py:206: '%~dp0..\\..\\win_sdk\\Lib\\WINVERSION\\ucrt\\x86;' # VS 2015 This too? https://codereview.chromium.org/1609933004/diff/40001/win_toolchain/package_f... win_toolchain/package_from_installed.py:217: '%~dp0..\\..\\win_sdk\\Lib\\WINVERSION\\ucrt\\x64;' # VS 2015 And here
On 2016/01/21 00:50:44, scottmg wrote: > I assume it doesn't matter too much on 2013 since the directory won't exist > anyway? Hmmm? It *only* matters on 2013. We package the ucrt directory because it's part of the Windows 10 SDK and having it in the include path causes VS 2013 to include ucrt\10586\stdlib.h which pulls in ucrt headers and breaks things instantly.
It would be interesting to try changing the include order to see if it works, just to keep things compatible with packaged VS, but not today. https://codereview.chromium.org/1609933004/diff/40001/win_toolchain/package_f... File win_toolchain/package_from_installed.py (right): https://codereview.chromium.org/1609933004/diff/40001/win_toolchain/package_f... win_toolchain/package_from_installed.py:206: '%~dp0..\\..\\win_sdk\\Lib\\WINVERSION\\ucrt\\x86;' # VS 2015 On 2016/01/21 00:50:49, scottmg wrote: > This too? Well, technically. The downside is it requires uglifying the code, and it isn't actually *necessary* because the only libs in there are libucrt[d].lib and ucrt[d].lib, which are never referenced. So, I can remove those references as well, but I'm tempted not to. Thoughts?
https://codereview.chromium.org/1609933004/diff/40001/win_toolchain/package_f... File win_toolchain/package_from_installed.py (right): https://codereview.chromium.org/1609933004/diff/40001/win_toolchain/package_f... win_toolchain/package_from_installed.py:206: '%~dp0..\\..\\win_sdk\\Lib\\WINVERSION\\ucrt\\x86;' # VS 2015 On 2016/01/21 00:55:49, brucedawson wrote: > On 2016/01/21 00:50:49, scottmg wrote: > > This too? > > Well, technically. The downside is it requires uglifying the code, and it isn't > actually *necessary* because the only libs in there are libucrt[d].lib and > ucrt[d].lib, which are never referenced. > > So, I can remove those references as well, but I'm tempted not to. Thoughts? OK. We should delete WINVERSION after we switch anyway and make this 2015-only, so lgtm either way.
> OK. We should delete WINVERSION after we switch anyway and make this 2015-only, > so lgtm either way. WIN_VERSION or VS_VERSION? We'll probably need WIN_VERSION forever, but VS_VERSION can certainly go. It's quite educational hacking on all these different build variants...
On 2016/01/21 01:01:40, brucedawson wrote: > > OK. We should delete WINVERSION after we switch anyway and make this > 2015-only, > > so lgtm either way. > > WIN_VERSION or VS_VERSION? We'll probably need WIN_VERSION forever, but > VS_VERSION can certainly go. Er, sorry, yes. I meant VS_VERSION. > It's quite educational hacking on all these different build variants... As long as you're enjoying it...? ;)
On 2016/01/21 01:10:09, scottmg wrote: > On 2016/01/21 01:01:40, brucedawson wrote: > > > OK. We should delete WINVERSION after we switch anyway and make this > > 2015-only, > > > so lgtm either way. > > > > WIN_VERSION or VS_VERSION? We'll probably need WIN_VERSION forever, but > > VS_VERSION can certainly go. > > Er, sorry, yes. I meant VS_VERSION. > > > It's quite educational hacking on all these different build variants... > > As long as you're enjoying it...? ;) Enjoying might be too strong a word. But I will be pleased when the matrix of 10240/10586, 2013/2015, gyp/gn, and DEPOT_TOOLS_WIN_TOOLCHAIN shrinks a bit.
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/1609933004/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1609933004/40001
Message was sent while issue was closed.
Description was changed from ========== Skip include\ucrt on VS 2013 packages In change crrev.com/1504983002 the include\ucrt path from the Windows 10 SDK was added to the include search path, but this is not a legal thing to do on VS 2013. This change makes the ucrt path VS 2015 specific. Testing shows that this makes no difference to the VS 2015 package. BUG=440500 ========== to ========== Skip include\ucrt on VS 2013 packages In change crrev.com/1504983002 the include\ucrt path from the Windows 10 SDK was added to the include search path, but this is not a legal thing to do on VS 2013. This change makes the ucrt path VS 2015 specific. Testing shows that this makes no difference to the VS 2015 package. BUG=440500 Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=298330 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as http://src.chromium.org/viewvc/chrome?view=rev&revision=298330
Message was sent while issue was closed.
https://codereview.chromium.org/1609933004/diff/40001/win_toolchain/package_f... File win_toolchain/package_from_installed.py (left): https://codereview.chromium.org/1609933004/diff/40001/win_toolchain/package_f... win_toolchain/package_from_installed.py:195: 'if "%1"=="/x64" goto x64\n'.replace('WINVERSION', WIN_VERSION)) Ah crap. I accidentally deleted this whole line when I was only supposed to delete the .replace(). This is causing breakage on the new VS 2015 package, but I'm not sure why it didn't cause problems before. No packages created since then? |
