Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1660723002:
Updates to package VS 2015 to not require UCRT (Closed)
|
Created:
4 years, 10 months ago by brucedawson Modified:
4 years, 10 months ago 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. |
DescriptionUpdates to package VS 2015 to not require UCRT
This change packages the api-ms-* DLLs and the VS 2015 CRT DLLs in all
of the VS package directories that we add to the path, so that they can
run without having the UCRT installed.
The Common7\IDE path was removed because it isn't actually packaged, in
VS 2013 or VS 2015, so adding it to the path is purely confusing.
In addition to changing the packaging script the installation script has
to change in order to continue if the UCRT cannot be installed. It
still makes sense to try to install it, and print a message saying where
the installer is, for the convenience of Google developers who may want
more flexibility in running VS 2015 binaries.
A 'calculating hash' message was added to make the mysterious hashing
hangs (which can be several minutes long) less mysterious.
BUG=440500
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=298541
Patch Set 1 #Patch Set 2 : Moving print statement #Patch Set 3 : Tweak no-elevation message #
Total comments: 9
Patch Set 4 : CR fixes #
Total comments: 2
Patch Set 5 : Add blank line per code review #Messages
Total messages: 20 (6 generated)
brucedawson@chromium.org changed reviewers: + scottmg@chromium.org
This appears to let the compiler run without installing the UCRT. I packaged the DLLs on Windows 8.1 and tested on Windows 7, so apparently (despite having different version numbers?) the DLLs are portable. I should probably just remove the code that tries to install the UCRT, if it's really not needed. Thoughts? I can't do a full end-to-end test without first landing this patch, and then doing a try job with a new hash. That is, testing this fully requires patches to two repos.
https://codereview.chromium.org/1660723002/diff/40001/win_toolchain/get_toolc... File win_toolchain/get_toolchain_if_necessary.py (right): https://codereview.chromium.org/1660723002/diff/40001/win_toolchain/get_toolc... win_toolchain/get_toolchain_if_necessary.py:115: print 'Calculating hash of toolchain in %s. Please wait...' % root This will probably need a stdout.flush() or you won't see it when run from gclient. https://codereview.chromium.org/1660723002/diff/40001/win_toolchain/package_f... File win_toolchain/package_from_installed.py (left): https://codereview.chromium.org/1660723002/diff/40001/win_toolchain/package_f... win_toolchain/package_from_installed.py:188: 'set PATH=%~dp0..\\..\\Common7\\IDE;%PATH%\n' Are you sure this isn't needed any more? I thought it was for .. pdb? or dia? or something? https://codereview.chromium.org/1660723002/diff/40001/win_toolchain/package_f... File win_toolchain/package_from_installed.py (right): https://codereview.chromium.org/1660723002/diff/40001/win_toolchain/package_f... win_toolchain/package_from_installed.py:165: for dest_dir in [ r"win_sdk\bin\x86", "sys32" ]: " -> ' https://codereview.chromium.org/1660723002/diff/40001/win_toolchain/package_f... win_toolchain/package_from_installed.py:172: for dest_dir in [ r"VC\bin\amd64_x86", r"VC\bin\amd64", and here
On 2016/02/02 22:46:46, brucedawson wrote: > This appears to let the compiler run without installing the UCRT. I packaged the > DLLs on Windows 8.1 and tested on Windows 7, so apparently (despite having > different version numbers?) the DLLs are portable. > > I should probably just remove the code that tries to install the UCRT, if it's > really not needed. Thoughts? > > I can't do a full end-to-end test without first landing this patch, and then > doing a try job with a new hash. That is, testing this fully requires patches to > two repos. Why can't you try it on a VM?
On 2016/02/02 22:55:27, scottmg wrote: > On 2016/02/02 22:46:46, brucedawson wrote: > > This appears to let the compiler run without installing the UCRT. I packaged > the > > DLLs on Windows 8.1 and tested on Windows 7, so apparently (despite having > > different version numbers?) the DLLs are portable. > > > > I should probably just remove the code that tries to install the UCRT, if it's > > really not needed. Thoughts? > > > > I can't do a full end-to-end test without first landing this patch, and then > > doing a try job with a new hash. That is, testing this fully requires patches > to > > two repos. > > Why can't you try it on a VM? I can try it on a VM, and have done partial tests - cl.exe and link.exe definitely launch. I just mean I can't do the full end-to-end testing (all operating systems, tests, etc.) on a VM. Building and running base_unittests on a VM is possible, but slow.
I might fix the paths for 32-bit OS at the same time. Thoughts? https://codereview.chromium.org/1660723002/diff/40001/win_toolchain/get_toolc... File win_toolchain/get_toolchain_if_necessary.py (right): https://codereview.chromium.org/1660723002/diff/40001/win_toolchain/get_toolc... win_toolchain/get_toolchain_if_necessary.py:115: print 'Calculating hash of toolchain in %s. Please wait...' % root On 2016/02/02 22:55:13, scottmg wrote: > This will probably need a stdout.flush() or you won't see it when run from > gclient. Done. https://codereview.chromium.org/1660723002/diff/40001/win_toolchain/package_f... File win_toolchain/package_from_installed.py (left): https://codereview.chromium.org/1660723002/diff/40001/win_toolchain/package_f... win_toolchain/package_from_installed.py:188: 'set PATH=%~dp0..\\..\\Common7\\IDE;%PATH%\n' On 2016/02/02 22:55:13, scottmg wrote: > Are you sure this isn't needed any more? I thought it was for .. pdb? or dia? or > something? Yep, pretty sure. Adding it to the path is a NOP because we haven't packaged that directory for a while. The directory that is needed for PDB (mspdb140.dll) is vc\bin\amd64. https://codereview.chromium.org/1660723002/diff/40001/win_toolchain/package_f... File win_toolchain/package_from_installed.py (right): https://codereview.chromium.org/1660723002/diff/40001/win_toolchain/package_f... win_toolchain/package_from_installed.py:165: for dest_dir in [ r"win_sdk\bin\x86", "sys32" ]: On 2016/02/02 22:55:13, scottmg wrote: > " -> ' D'oh! Done. https://codereview.chromium.org/1660723002/diff/40001/win_toolchain/package_f... win_toolchain/package_from_installed.py:172: for dest_dir in [ r"VC\bin\amd64_x86", r"VC\bin\amd64", On 2016/02/02 22:55:13, scottmg wrote: > and here Done.
On 2016/02/03 00:19:50, brucedawson wrote: > I might fix the paths for 32-bit OS at the same time. Thoughts? If you want. I'd rather not have the 32 host tools in here, but I guess since we do it might as well sorta-kinda-work. > > https://codereview.chromium.org/1660723002/diff/40001/win_toolchain/get_toolc... > File win_toolchain/get_toolchain_if_necessary.py (right): > > https://codereview.chromium.org/1660723002/diff/40001/win_toolchain/get_toolc... > win_toolchain/get_toolchain_if_necessary.py:115: print 'Calculating hash of > toolchain in %s. Please wait...' % root > On 2016/02/02 22:55:13, scottmg wrote: > > This will probably need a stdout.flush() or you won't see it when run from > > gclient. > > Done. > > https://codereview.chromium.org/1660723002/diff/40001/win_toolchain/package_f... > File win_toolchain/package_from_installed.py (left): > > https://codereview.chromium.org/1660723002/diff/40001/win_toolchain/package_f... > win_toolchain/package_from_installed.py:188: 'set > PATH=%~dp0..\\..\\Common7\\IDE;%PATH%\n' > On 2016/02/02 22:55:13, scottmg wrote: > > Are you sure this isn't needed any more? I thought it was for .. pdb? or dia? > or > > something? > > Yep, pretty sure. Adding it to the path is a NOP because we haven't packaged > that directory for a while. The directory that is needed for PDB (mspdb140.dll) > is vc\bin\amd64. > > https://codereview.chromium.org/1660723002/diff/40001/win_toolchain/package_f... > File win_toolchain/package_from_installed.py (right): > > https://codereview.chromium.org/1660723002/diff/40001/win_toolchain/package_f... > win_toolchain/package_from_installed.py:165: for dest_dir in [ > r"win_sdk\bin\x86", "sys32" ]: > On 2016/02/02 22:55:13, scottmg wrote: > > " -> ' > > D'oh! Done. > > https://codereview.chromium.org/1660723002/diff/40001/win_toolchain/package_f... > win_toolchain/package_from_installed.py:172: for dest_dir in [ > r"VC\bin\amd64_x86", r"VC\bin\amd64", > On 2016/02/02 22:55:13, scottmg wrote: > > and here > > Done.
https://codereview.chromium.org/1660723002/diff/40001/win_toolchain/package_f... File win_toolchain/package_from_installed.py (left): https://codereview.chromium.org/1660723002/diff/40001/win_toolchain/package_f... win_toolchain/package_from_installed.py:188: 'set PATH=%~dp0..\\..\\Common7\\IDE;%PATH%\n' On 2016/02/03 00:19:49, brucedawson wrote: > On 2016/02/02 22:55:13, scottmg wrote: > > Are you sure this isn't needed any more? I thought it was for .. pdb? or dia? > or > > something? > > Yep, pretty sure. Adding it to the path is a NOP because we haven't packaged > that directory for a while. The directory that is needed for PDB (mspdb140.dll) > is vc\bin\amd64. OK. That may have been for the x86 host tools, not x64 then.
I just uploaded my fixes based on your comments. On 2016/02/03 00:23:53, scottmg wrote: > On 2016/02/03 00:19:50, brucedawson wrote: > > I might fix the paths for 32-bit OS at the same time. Thoughts? > > If you want. I'd rather not have the 32 host tools in here, but I guess since we > do it might as well sorta-kinda-work. Regarding 32-bit OS, I was talking about crbug.com/549290, which I assumed that we wanted fix. Although, I'd like more clarity on what actually needs to be in the path in that case.
I think I'll do the 32-bit change separately - better to roll an extra package than to delay VS 2015 still more. My VM is slowly getting a chrome repo for a bit more testing, but how does this look? I could just commit the change to get_toolchain_if_necessary.py since that is the only thing that blocks testing.
lgtm https://codereview.chromium.org/1660723002/diff/60001/win_toolchain/package_f... File win_toolchain/package_from_installed.py (right): https://codereview.chromium.org/1660723002/diff/60001/win_toolchain/package_f... win_toolchain/package_from_installed.py:167: # Copy the x64 ucrt DLLs to all directories with 64-bit binaries that are nit; add a blank line before this comment
dpranke@chromium.org changed reviewers: + dpranke@chromium.org
It seems a bit odd to me that we'd want to spend any time on 32-bit hosts at all these days (on windows or anywhere else). I've posted a comment to that end to crbug.com/549290 and we can see what sbc@ says (that bug was filed before we decided to drop 32-bit linux, for example). I wouldn't do anything for 32-bit as part of this patch, at least, so it's good that you already said you'll separate that.
https://codereview.chromium.org/1660723002/diff/60001/win_toolchain/package_f... File win_toolchain/package_from_installed.py (right): https://codereview.chromium.org/1660723002/diff/60001/win_toolchain/package_f... win_toolchain/package_from_installed.py:167: # Copy the x64 ucrt DLLs to all directories with 64-bit binaries that are On 2016/02/03 04:12:00, scottmg wrote: > nit; add a blank line before this comment Done.
Description was changed from ========== Updates to package VS 2015 to not require UCRT This change packages the api-ms-* DLLs and the VS 2015 CRT DLLs in all of the VS package directories that we add to the path, so that they can run without having the UCRT installed. The Common7\IDE path was removed because it isn't actually packaged, in VS 2013 or VS 2015. In addition to changing the packaging script the installation script has to change in order to continue if the UCRT cannot be installed. It still makes sense to try to install it, and print a message saying where the installer is, for the convenience of Google developers who may want more flexibility in running VS 2015 binaries. Or maybe not? A 'calculating hash' message was added to make the mysterious hashing hangs less mysterious. BUG=440500 ========== to ========== Updates to package VS 2015 to not require UCRT This change packages the api-ms-* DLLs and the VS 2015 CRT DLLs in all of the VS package directories that we add to the path, so that they can run without having the UCRT installed. The Common7\IDE path was removed because it isn't actually packaged, in VS 2013 or VS 2015, so adding it to the path is purely confusing. In addition to changing the packaging script the installation script has to change in order to continue if the UCRT cannot be installed. It still makes sense to try to install it, and print a message saying where the installer is, for the convenience of Google developers who may want more flexibility in running VS 2015 binaries. A 'calculating hash' message was added to make the mysterious hashing hangs (which can be several minutes long) less mysterious. BUG=440500 ==========
The CQ bit was checked by brucedawson@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from scottmg@chromium.org Link to the patchset: https://codereview.chromium.org/1660723002/#ps80001 (title: "Add blank line per code review")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1660723002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1660723002/80001
Message was sent while issue was closed.
Description was changed from ========== Updates to package VS 2015 to not require UCRT This change packages the api-ms-* DLLs and the VS 2015 CRT DLLs in all of the VS package directories that we add to the path, so that they can run without having the UCRT installed. The Common7\IDE path was removed because it isn't actually packaged, in VS 2013 or VS 2015, so adding it to the path is purely confusing. In addition to changing the packaging script the installation script has to change in order to continue if the UCRT cannot be installed. It still makes sense to try to install it, and print a message saying where the installer is, for the convenience of Google developers who may want more flexibility in running VS 2015 binaries. A 'calculating hash' message was added to make the mysterious hashing hangs (which can be several minutes long) less mysterious. BUG=440500 ========== to ========== Updates to package VS 2015 to not require UCRT This change packages the api-ms-* DLLs and the VS 2015 CRT DLLs in all of the VS package directories that we add to the path, so that they can run without having the UCRT installed. The Common7\IDE path was removed because it isn't actually packaged, in VS 2013 or VS 2015, so adding it to the path is purely confusing. In addition to changing the packaging script the installation script has to change in order to continue if the UCRT cannot be installed. It still makes sense to try to install it, and print a message saying where the installer is, for the convenience of Google developers who may want more flexibility in running VS 2015 binaries. A 'calculating hash' message was added to make the mysterious hashing hangs (which can be several minutes long) less mysterious. BUG=440500 Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=298541 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as http://src.chromium.org/viewvc/chrome?view=rev&revision=298541 |
