Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1859013002:
Copy cdb.exe into telemetry_chrome_test's isolate. (Closed)
|
Created:
4 years, 8 months ago by Ken Russell (switch to Gerrit) Modified:
4 years, 8 months ago CC:
chromium-reviews, telemetry-reviews_chromium.org, Vadim Sh., Dirk Pranke Base URL:
https://chromium.googlesource.com/chromium/src.git@fix-windows-stack-symbolization Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCopy cdb.exe into telemetry_chrome_test's isolate.
This binary needs to be available on the Swarming bots, but only
Google employees and the bots have access to the hermetic Windows
toolchain. Since the isolates are only built by these entities, only
copy and bundle them in this situation.
BUG=561763
R=scottmg@chromium.org, nednguyen@chromium.org
TBR=maruel@chromium.org
Committed: https://crrev.com/52685e82ecadd2824732bd6990b8c377142b8b81
Cr-Commit-Position: refs/heads/master@{#385880}
Patch Set 1 #
Total comments: 12
Patch Set 2 : Addressed review feedback from scottmg and maruel. #
Total comments: 2
Patch Set 3 : Put cdb.exe and their helper DLLs into a subdirectory. #Patch Set 4 : Fixed permissions and dependencies. #Patch Set 5 : Improve detection of whether to copy files thanks to dpranke@. #
Total comments: 1
Messages
Total messages: 43 (17 generated)
kbr@chromium.org changed reviewers: + dyen@chromium.org, maruel@chromium.org, nednguyen@google.com, scottmg@chromium.org, thakis@chromium.org
Please take a look. I'd like to know whether this approach is acceptable. Please see https://codereview.chromium.org/1854413002 for the companion CL for catapult. That would have to land and be rolled in along with this for it to work and for the stack trace test to be enabled on the bots. (I tested this locally, though not via isolate, and it works.)
https://codereview.chromium.org/1859013002/diff/1/build/win/copy_cdb_to_outpu... File build/win/copy_cdb_to_output.py (right): https://codereview.chromium.org/1859013002/diff/1/build/win/copy_cdb_to_outpu... build/win/copy_cdb_to_output.py:45: vs_runtime_dll_dirs = vs_toolchain.SetEnvironmentAndGetRuntimeDllDirs() Does this work when it finds the directory in c:\program files? I unfamiliar with vs_toolchain.
https://codereview.chromium.org/1859013002/diff/1/build/win/copy_cdb_to_outpu... File build/win/copy_cdb_to_output.py (right): https://codereview.chromium.org/1859013002/diff/1/build/win/copy_cdb_to_outpu... build/win/copy_cdb_to_output.py:31: os.stat(target).st_mtime != os.stat(source).st_mtime)): +1 space on this line, otherwise it's a bit misleading https://codereview.chromium.org/1859013002/diff/1/build/win/copy_cdb_to_outpu... build/win/copy_cdb_to_output.py:45: vs_runtime_dll_dirs = vs_toolchain.SetEnvironmentAndGetRuntimeDllDirs() You don't use the return value here, just remove the assignment. https://codereview.chromium.org/1859013002/diff/1/build/win/copy_cdb_to_outpu... build/win/copy_cdb_to_output.py:46: win_sdk_dir = _NormalizePath(os.environ['WINDOWSSDKDIR']) How about `os.path.normpath(os.environ['WINDOWSSDKDIR'])` instead? https://codereview.chromium.org/1859013002/diff/1/build/win/copy_cdb_to_outpu... build/win/copy_cdb_to_output.py:56: _CopyImpl('cdb.exe', output_dir, src_dir) Hopefully this set is sufficient. Note here that the .gyp outputs needs to be kept in sync. https://codereview.chromium.org/1859013002/diff/1/build/win/copy_cdb_to_outpu... build/win/copy_cdb_to_output.py:65: print >>sys.stderr, 'Usage: copy_cdb_to_output.py [output_dir] ' + \ nit; normally [] indicate optional arguments, but these aren't optional, use <> or nothing instead.
https://codereview.chromium.org/1859013002/diff/20001/tools/perf/core/stacktr... File tools/perf/core/stacktrace_unittest.py (right): https://codereview.chromium.org/1859013002/diff/20001/tools/perf/core/stacktr... tools/perf/core/stacktrace_unittest.py:16: @decorators.Enabled('mac', 'linux') Enable this on win as well?
https://codereview.chromium.org/1859013002/diff/1/build/win/copy_cdb_to_outpu... File build/win/copy_cdb_to_output.py (right): https://codereview.chromium.org/1859013002/diff/1/build/win/copy_cdb_to_outpu... build/win/copy_cdb_to_output.py:31: os.stat(target).st_mtime != os.stat(source).st_mtime)): On 2016/04/05 16:41:28, scottmg wrote: > +1 space on this line, otherwise it's a bit misleading Done. https://codereview.chromium.org/1859013002/diff/1/build/win/copy_cdb_to_outpu... build/win/copy_cdb_to_output.py:45: vs_runtime_dll_dirs = vs_toolchain.SetEnvironmentAndGetRuntimeDllDirs() On 2016/04/05 10:41:06, M-A Ruel wrote: > Does this work when it finds the directory in c:\program files? I unfamiliar > with vs_toolchain. It shouldn't find it there -- that script is what sets up depot_tools' copy of the toolchain. https://codereview.chromium.org/1859013002/diff/1/build/win/copy_cdb_to_outpu... build/win/copy_cdb_to_output.py:45: vs_runtime_dll_dirs = vs_toolchain.SetEnvironmentAndGetRuntimeDllDirs() On 2016/04/05 16:41:28, scottmg wrote: > You don't use the return value here, just remove the assignment. Done. https://codereview.chromium.org/1859013002/diff/1/build/win/copy_cdb_to_outpu... build/win/copy_cdb_to_output.py:46: win_sdk_dir = _NormalizePath(os.environ['WINDOWSSDKDIR']) On 2016/04/05 16:41:28, scottmg wrote: > How about `os.path.normpath(os.environ['WINDOWSSDKDIR'])` instead? Sure. I was just following the pattern in vs_toolchain.py. Done (and removed _NormalizePath). https://codereview.chromium.org/1859013002/diff/1/build/win/copy_cdb_to_outpu... build/win/copy_cdb_to_output.py:56: _CopyImpl('cdb.exe', output_dir, src_dir) On 2016/04/05 16:41:28, scottmg wrote: > Hopefully this set is sufficient. Note here that the .gyp outputs needs to be > kept in sync. Added a note. Will update this set, and the isolate, if testing shows it's insufficient. https://codereview.chromium.org/1859013002/diff/1/build/win/copy_cdb_to_outpu... build/win/copy_cdb_to_output.py:65: print >>sys.stderr, 'Usage: copy_cdb_to_output.py [output_dir] ' + \ On 2016/04/05 16:41:28, scottmg wrote: > nit; normally [] indicate optional arguments, but these aren't optional, use <> > or nothing instead. Changed to use <>. https://codereview.chromium.org/1859013002/diff/20001/tools/perf/core/stacktr... File tools/perf/core/stacktrace_unittest.py (right): https://codereview.chromium.org/1859013002/diff/20001/tools/perf/core/stacktr... tools/perf/core/stacktrace_unittest.py:16: @decorators.Enabled('mac', 'linux') On 2016/04/05 17:14:06, nednguyen wrote: > Enable this on win as well? It doesn't pass yet. The stack trace doesn't contain "Thread 0 (crashed)"; it instead contains just the symbolized stack from the one thread. I'd like to get the other test enabled before debugging it further.
build lgtm
lgtm
This depends on https://codereview.chromium.org/1854413002/ rolling into Chromium; but once it does, this should pass try jobs. Tested locally on Windows 8.1 via isolate. cdb.isolate might need more runtime DLLs for more Windows versions; we'll find out, and I'll update it as necessary.
Description was changed from ========== Copy cdb.exe into telemetry_chrome_test's isolate. This binary needs to be available on the Swarming bots, but only Google employees and the bots have access to the hermetic Windows toolchain. Since the isolates are only built by these entities, only copy and bundle them in this situation. BUG=561763 ========== to ========== Copy cdb.exe into telemetry_chrome_test's isolate. This binary needs to be available on the Swarming bots, but only Google employees and the bots have access to the hermetic Windows toolchain. Since the isolates are only built by these entities, only copy and bundle them in this situation. BUG=561763 R=scottmg@chromium.org, nednguyen@chromium.org TBR=maruel@chromium.org ==========
On 2016/04/06 00:59:16, Ken Russell wrote: > This depends on https://codereview.chromium.org/1854413002/ rolling into > Chromium; but once it does, this should pass try jobs. Tested locally on Windows > 8.1 via isolate. cdb.isolate might need more runtime DLLs for more Windows > versions; we'll find out, and I'll update it as necessary. TBR'ing to maruel@ for .isolate changes. CQ'ing now; should work.
The CQ bit was checked by kbr@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nednguyen@google.com, scottmg@chromium.org Link to the patchset: https://codereview.chromium.org/1859013002/#ps40001 (title: "Put cdb.exe and their helper DLLs into a subdirectory.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1859013002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1859013002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
copy_cdb_to_output must not be referenced on OSX.
On 2016/04/06 17:51:35, M-A Ruel wrote: > copy_cdb_to_output must not be referenced on OSX. Fixed.
The CQ bit was checked by kbr@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nednguyen@google.com, scottmg@chromium.org Link to the patchset: https://codereview.chromium.org/1859013002/#ps60001 (title: "Fixed permissions and dependencies.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1859013002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1859013002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
dpranke@: thanks for your help earlier. Instead of writing a stamp file, just used the hex digest as you suggested, and used the current time rather than the original file's last modified time. I think this will work correctly. Re-CQ'ing.
The CQ bit was checked by kbr@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nednguyen@google.com, scottmg@chromium.org Link to the patchset: https://codereview.chromium.org/1859013002/#ps80001 (title: "Improve detection of whether to copy files thanks to dpranke@.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1859013002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1859013002/80001
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_...)
On 2016/04/07 00:53:22, commit-bot: I haz the power wrote: > 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_...) Hmm, this is actually failing for me locally as well. I can't figure out what is wrong though, I tried copying the entire debugger folder into the out/cdb folder and it still doesn't work locally.
On 2016/04/07 01:38:43, David Yen wrote: > On 2016/04/07 00:53:22, commit-bot: I haz the power wrote: > > 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_...) > > Hmm, this is actually failing for me locally as well. I can't figure out what is > wrong though, I tried copying the entire debugger folder into the out/cdb folder > and it still doesn't work locally. It looks like we were simply missing another PDB file, I added it here: https://codereview.chromium.org/1871623002/ I've tested this using a dependency and it looks like it worked: https://codereview.chromium.org/1862123005/ I'll go ahead and check the CQ once my CL passes.
On 2016/04/07 18:15:51, David Yen wrote: > On 2016/04/07 01:38:43, David Yen wrote: > > On 2016/04/07 00:53:22, commit-bot: I haz the power wrote: > > > 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_...) > > > > Hmm, this is actually failing for me locally as well. I can't figure out what > is > > wrong though, I tried copying the entire debugger folder into the out/cdb > folder > > and it still doesn't work locally. > > It looks like we were simply missing another PDB file, I added it here: > > https://codereview.chromium.org/1871623002/ > > I've tested this using a dependency and it looks like it worked: > > https://codereview.chromium.org/1862123005/ > > I'll go ahead and check the CQ once my CL passes. Thanks David for tracking this down. I tested locally with the component build, which is probably why I missed this. Appreciate your help getting this landed and the test turned on.
The CQ bit was checked by dyen@chromium.org
The CQ bit was unchecked by dyen@chromium.org
The CQ bit was checked by dyen@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1859013002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1859013002/80001
Message was sent while issue was closed.
Description was changed from ========== Copy cdb.exe into telemetry_chrome_test's isolate. This binary needs to be available on the Swarming bots, but only Google employees and the bots have access to the hermetic Windows toolchain. Since the isolates are only built by these entities, only copy and bundle them in this situation. BUG=561763 R=scottmg@chromium.org, nednguyen@chromium.org TBR=maruel@chromium.org ========== to ========== Copy cdb.exe into telemetry_chrome_test's isolate. This binary needs to be available on the Swarming bots, but only Google employees and the bots have access to the hermetic Windows toolchain. Since the isolates are only built by these entities, only copy and bundle them in this situation. BUG=561763 R=scottmg@chromium.org, nednguyen@chromium.org TBR=maruel@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
dpranke@chromium.org changed reviewers: + dpranke@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/1859013002/diff/80001/build/win/copy_cdb_to_o... File build/win/copy_cdb_to_output.py (right): https://codereview.chromium.org/1859013002/diff/80001/build/win/copy_cdb_to_o... build/win/copy_cdb_to_output.py:20: afile = open(file_name, 'rb') nit: you need to close() afile.
Message was sent while issue was closed.
Description was changed from ========== Copy cdb.exe into telemetry_chrome_test's isolate. This binary needs to be available on the Swarming bots, but only Google employees and the bots have access to the hermetic Windows toolchain. Since the isolates are only built by these entities, only copy and bundle them in this situation. BUG=561763 R=scottmg@chromium.org, nednguyen@chromium.org TBR=maruel@chromium.org ========== to ========== Copy cdb.exe into telemetry_chrome_test's isolate. This binary needs to be available on the Swarming bots, but only Google employees and the bots have access to the hermetic Windows toolchain. Since the isolates are only built by these entities, only copy and bundle them in this situation. BUG=561763 R=scottmg@chromium.org, nednguyen@chromium.org TBR=maruel@chromium.org Committed: https://crrev.com/52685e82ecadd2824732bd6990b8c377142b8b81 Cr-Commit-Position: refs/heads/master@{#385880} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/52685e82ecadd2824732bd6990b8c377142b8b81 Cr-Commit-Position: refs/heads/master@{#385880}
Message was sent while issue was closed.
On 2016/04/07 21:38:39, Dirk Pranke wrote: > https://codereview.chromium.org/1859013002/diff/80001/build/win/copy_cdb_to_o... > File build/win/copy_cdb_to_output.py (right): > > https://codereview.chromium.org/1859013002/diff/80001/build/win/copy_cdb_to_o... > build/win/copy_cdb_to_output.py:20: afile = open(file_name, 'rb') > nit: you need to close() afile. Oops, thanks for pointing that out. Fixing in https://codereview.chromium.org/1873493003 . |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
