Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
306543006:
Bring Syzygy binaries in using a script rather than as a dependency. (Closed)
|
Created:
6 years, 6 months ago by chrisha Modified:
6 years, 6 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionBring Syzygy binaries in using a script rather than as a dependency.
BUG=
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=273353
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=277048
Patch Set 1 #Patch Set 2 : Fix to script to handle read-only git files. #
Total comments: 4
Patch Set 3 : Addressed Siggi's nit. #Patch Set 4 : Only run on win32. #Patch Set 5 : Rebased. #Patch Set 6 : Bugfix. #Messages
Total messages: 24 (0 generated)
PTAL
Nice! https://codereview.chromium.org/306543006/diff/20001/DEPS File DEPS (right): https://codereview.chromium.org/306543006/diff/20001/DEPS#newcode773 DEPS:773: "--revision=b08fb72610963d31cc3eae33f746a04e263bd860", Sorry to be a ninny, but everything else above indirects the sha1 through a file. Is there reason to do likewise here? It's going to be much easier to review and commit a file, and this will lend itself better to automated rolling, if ever it comes to that... https://codereview.chromium.org/306543006/diff/20001/build/get_syzygy_binarie... File build/get_syzygy_binaries.py (right): https://codereview.chromium.org/306543006/diff/20001/build/get_syzygy_binarie... build/get_syzygy_binaries.py:186: _LOGGER.debug('Removing read-only file: %s', path) nit: ...or directory...?
Another look? https://codereview.chromium.org/306543006/diff/20001/DEPS File DEPS (right): https://codereview.chromium.org/306543006/diff/20001/DEPS#newcode773 DEPS:773: "--revision=b08fb72610963d31cc3eae33f746a04e263bd860", On 2014/05/28 15:24:12, Sigurður Ásgeirsson wrote: > Sorry to be a ninny, but everything else above indirects the sha1 through a > file. Is there reason to do likewise here? > It's going to be much easier to review and commit a file, and this will lend > itself better to automated rolling, if ever it comes to that... There's no real advantage to one vs the other for us, as either way its simply changing the value of a single hash. The other mechanisms use download_from_google_storage, which will download foo.exe alongside an existing foo.exe.sha1 file. Due to the sheer number of files we download and install using this mechanism would be expensive and awkward for us (require changing dozens of .sha1 files per roll). Instead we opted for something similar in spirit (a script that downloads from common data storage), but that gets all the binaries in a single go from a single specified hash. https://codereview.chromium.org/306543006/diff/20001/build/get_syzygy_binarie... File build/get_syzygy_binaries.py (right): https://codereview.chromium.org/306543006/diff/20001/build/get_syzygy_binarie... build/get_syzygy_binaries.py:186: _LOGGER.debug('Removing read-only file: %s', path) On 2014/05/28 15:24:12, Sigurður Ásgeirsson wrote: > nit: ...or directory...? Done.
lgtm - I guess you'll discover it soon enough if the DEPS file becomes a bottleneck for rolling.
The CQ bit was checked by chrisha@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/chrisha@chromium.org/306543006/30001
Message was sent while issue was closed.
Change committed as 273353
Message was sent while issue was closed.
A revert of this CL has been created in https://codereview.chromium.org/305903002/ by tzik@chromium.org. The reason for reverting is: This CL broke check_licenses on Android Builder: http://build.chromium.org/p/chromium.linux/builders/Android%20Builder%20%28db....
Made this Windows only. PTAL?
lgtm
Thanks, trying this a second time :)
The CQ bit was checked by chrisha@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/chrisha@chromium.org/306543006/50001
Looks like the patch is failing to apply cleanly... rebasing and trying again.
The CQ bit was unchecked by chrisha@chromium.org
The CQ bit was checked by chrisha@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/chrisha@chromium.org/306543006/150001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...) win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...) win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...) win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/19337)
The CQ bit was unchecked by chrisha@chromium.org
The CQ bit was checked by chrisha@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/chrisha@chromium.org/306543006/170001
Message was sent while issue was closed.
Change committed as 277048
Message was sent while issue was closed.
This break the SyzyASan LKGR build because this doesn't put syzyasan_rtl.dll.pdb . While it's not a file so useful for the developers (they can get the symbols on the symbol server) we put them in the SyzyASan LKGR archives. We could probably remove it from them, but I have to check if i's used by Clusterfuzz first... I'll revert this CL in the meantime.
Message was sent while issue was closed.
A revert of this CL has been created in https://codereview.chromium.org/337903002/ by sebmarchand@chromium.org. The reason for reverting is: This break the SyzyASan LKGR build because this doesn't put syzyasan_rtl.dll.pdb . While it's not a file so useful for the developers (they can get the symbols on the symbol server) we put them in the SyzyASan LKGR archives. We could probably remove it from them, but I have to check if i's used by Clusterfuzz first... I'll revert this CL in the meantime.. |
