Add support for copying coredumps to /tmp

Add support for copying coredumps to /tmp

This will be used on the bots to be able to archive dumps from unexpectec crashes.

The tools/archive_crash.py tool will be used to actually archive the dumps to gcs and delete the local copies. The reason for the split is to enable developers to actually utilize this locally as well (run tools/test.py in a loop but don't archive the expected crashes). Additionally, this puts the bot specific code in the small python script.

R=whesse@google.com

Committed: https://code.google.com/p/dart/source/detail?r=32111

[Bill Hesse, 2014-01-29 07:01:52]

LGTM, with comments.

https://codereview.chromium.org/145273024/diff/1/tools/archive_crash.py
File tools/archive_crash.py (right):

https://codereview.chromium.org/145273024/diff/1/tools/archive_crash.py#newco...
tools/archive_crash.py:19: GSC_FOLDER = 'dart-crashes'
Why is this GSC_FOLDER instead of GCS_FOLDER?

https://codereview.chromium.org/145273024/diff/1/tools/archive_crash.py#newco...
tools/archive_crash.py:29: cmd = ['gsutil', 'cp', filename, gs_location]
So people (all non-google users?  all non-buildbot users?) without gsutil set
up, with access to dart-crashes, should not run with the crash-dumps option?

https://codereview.chromium.org/145273024/diff/1/tools/archive_crash.py#newco...
tools/archive_crash.py:46: os.unlink(tarname)
Will we not reach this if the gsutil command fails?  should we make this always
run?

https://codereview.chromium.org/145273024/diff/1/tools/testing/dart/test_prog...
File tools/testing/dart/test_progress.dart (right):

https://codereview.chromium.org/145273024/diff/1/tools/testing/dart/test_prog...
tools/testing/dart/test_progress.dart:290: if (file.existsSync()) {
I think this is always true, since it was true above.  Do you mean if
binFile.existsSync?

https://codereview.chromium.org/145273024/diff/1/tools/testing/dart/test_prog...
tools/testing/dart/test_progress.dart:291: var dir = new
Path(TestUtils.mkdirRecursive(new Path('/tmp'),
We don't want to use Directory.systemTemp for this?

https://codereview.chromium.org/145273024/diff/1/tools/testing/dart/test_prog...
tools/testing/dart/test_progress.dart:294: TestUtils.copyFile(new Path(binName),
dir.append(binBaseName));
I hate to see all these new uses of Path go in, but they will be easy to change
whenever we decide to get rid of Path.

https://codereview.chromium.org/145273024/diff/1/tools/testing/dart/test_prog...
tools/testing/dart/test_progress.dart:311: 
Add a line here?

https://codereview.chromium.org/145273024/diff/1/tools/testing/dart/test_runn...
File tools/testing/dart/test_runner.dart (right):

https://codereview.chromium.org/145273024/diff/1/tools/testing/dart/test_runn...
tools/testing/dart/test_runner.dart:899: int this.pid) {
Can an initializing argument in a constructor not be an optional positional
argument?  It looks like you would want a default value 0 here in most cases.

[ricow1, 2014-01-29 09:41:38]

https://codereview.chromium.org/145273024/diff/1/tools/archive_crash.py
File tools/archive_crash.py (right):

https://codereview.chromium.org/145273024/diff/1/tools/archive_crash.py#newco...
tools/archive_crash.py:19: GSC_FOLDER = 'dart-crashes'
On 2014/01/29 07:01:52, Bill Hesse wrote:
> Why is this GSC_FOLDER instead of GCS_FOLDER? 

Unintentional, fixed

https://codereview.chromium.org/145273024/diff/1/tools/archive_crash.py#newco...
tools/archive_crash.py:29: cmd = ['gsutil', 'cp', filename, gs_location]
On 2014/01/29 07:01:52, Bill Hesse wrote:
> So people (all non-google users?  all non-buildbot users?) without gsutil set
> up, with access to dart-crashes, should not run with the crash-dumps option?

This script is _not_ called from tools/test.dart - test.dart will copy the
coredumps/binaries to /tmp/coredump_PID - that is it.
This is meant to be called as a separate step on the bot

https://codereview.chromium.org/145273024/diff/1/tools/archive_crash.py#newco...
tools/archive_crash.py:46: os.unlink(tarname)
On 2014/01/29 07:01:52, Bill Hesse wrote:
> Will we not reach this if the gsutil command fails?  should we make this
always
> run?

Again, this is meant to be called on the bot, if we fail to do it the bot will
go red, we still have a valid crash dump that we probably want to look at (we
can manually fetch it)

https://codereview.chromium.org/145273024/diff/1/tools/testing/dart/test_prog...
File tools/testing/dart/test_progress.dart (right):

https://codereview.chromium.org/145273024/diff/1/tools/testing/dart/test_prog...
tools/testing/dart/test_progress.dart:290: if (file.existsSync()) {
On 2014/01/29 07:01:52, Bill Hesse wrote:
> I think this is always true, since it was true above.  Do you mean if
> binFile.existsSync?
yes, thank you

https://codereview.chromium.org/145273024/diff/1/tools/testing/dart/test_prog...
tools/testing/dart/test_progress.dart:291: var dir = new
Path(TestUtils.mkdirRecursive(new Path('/tmp'),
On 2014/01/29 07:01:52, Bill Hesse wrote:
> We don't want to use Directory.systemTemp for this?

Done.

https://codereview.chromium.org/145273024/diff/1/tools/testing/dart/test_prog...
tools/testing/dart/test_progress.dart:294: TestUtils.copyFile(new Path(binName),
dir.append(binBaseName));
On 2014/01/29 07:01:52, Bill Hesse wrote:
> I hate to see all these new uses of Path go in, but they will be easy to
change
> whenever we decide to get rid of Path.

Yes I thought about that, but our copy functions take paths so I left it for
consistency

https://codereview.chromium.org/145273024/diff/1/tools/testing/dart/test_prog...
tools/testing/dart/test_progress.dart:311: 
On 2014/01/29 07:01:52, Bill Hesse wrote:
> Add a line here?

Done.

https://codereview.chromium.org/145273024/diff/1/tools/testing/dart/test_runn...
File tools/testing/dart/test_runner.dart (right):

https://codereview.chromium.org/145273024/diff/1/tools/testing/dart/test_runn...
tools/testing/dart/test_runner.dart:899: int this.pid) {
On 2014/01/29 07:01:52, Bill Hesse wrote:
> Can an initializing argument in a constructor not be an optional positional
> argument?  It looks like you would want a default value 0 here in most cases.
I tried that, but failed, it seems that when we have a mixin in one of the sub
classes it does not work

[ricow1, 2014-01-29 09:41:54]

Committed patchset #2 manually as r32111 (presubmit successful).