Support for removing left over temporary directories from dart processes

Support for removing left over temporary directories from dart processes

VM developers have asked for a way to make test.py automatically
delete left-over temporary directories at the start.

This CL will enable them to do so, but they need to opt-in for it, by setting
the environment variable DART_TESTING_DELETE_TEMPORARY_DIRECTORIES to 1.

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

[ricow1, 2013-04-15 13:35:47]

LGTM

https://codereview.chromium.org/14244009/diff/4001/tools/test.dart
File tools/test.dart (right):

https://codereview.chromium.org/14244009/diff/4001/tools/test.dart#newcode261
tools/test.dart:261: } catch (error) {}
maybe print this to our debug log?

https://codereview.chromium.org/14244009/diff/4001/tools/test.dart#newcode264
tools/test.dart:264: }, onDone: completer.complete(null));
don't give argument to complete

https://codereview.chromium.org/14244009/diff/4001/tools/test.dart#newcode266
tools/test.dart:266: completer.complete(null);
no need to return null here

https://codereview.chromium.org/14244009/diff/4001/tools/test.dart#newcode272
tools/test.dart:272: deleteTemporaryDartDirectories().then((_) {
remove argument in closure

[ahe, 2013-04-17 13:05:32]

I wonder if we can avoid opting in?

For example, if test.dart can recognize its own temporary directories, it can
remove them when they are more than a day old.

[kustermann, 2013-04-17 13:28:12]

On 2013/04/17 13:05:32, ahe wrote:
> I wonder if we can avoid opting in?
> 
> For example, if test.dart can recognize its own temporary directories, it can
> remove them when they are more than a day old.

Currently there is no way to reliably tell which temporary directories are from
previous runs of test.py an which ones are from other dart processes. We would
probably need to make a naming convention in order to support that and rely on
people following it when they write tests.

I think we SHOULD do it in a 'opting in' way.

[kustermann, 2013-04-17 14:04:49]

https://codereview.chromium.org/14244009/diff/4001/tools/test.dart
File tools/test.dart (right):

https://codereview.chromium.org/14244009/diff/4001/tools/test.dart#newcode261
tools/test.dart:261: } catch (error) {}
On 2013/04/15 13:35:47, ricow1 wrote:
> maybe print this to our debug log?
I can do it, but then people will get these messages when they run test.py (the
DebugLogger writes it only to a debug log if '--write-debug-log' was specified).

https://codereview.chromium.org/14244009/diff/4001/tools/test.dart#newcode264
tools/test.dart:264: }, onDone: completer.complete(null));
On 2013/04/15 13:35:47, ricow1 wrote:
> don't give argument to complete

Done.

https://codereview.chromium.org/14244009/diff/4001/tools/test.dart#newcode266
tools/test.dart:266: completer.complete(null);
On 2013/04/15 13:35:47, ricow1 wrote:
> no need to return null here

Done.

https://codereview.chromium.org/14244009/diff/4001/tools/test.dart#newcode272
tools/test.dart:272: deleteTemporaryDartDirectories().then((_) {
On 2013/04/15 13:35:47, ricow1 wrote:
> remove argument in closure

Future.then takes a closure with exactly one argument as far as I know.

[ahe, 2013-04-17 15:08:13]

LGTM if this is really what you want to do.

I don't think this is the right solution to the problem.  Users should not have
to opt-in to not have their tmp dir fill up.

Cheers,
Peter

https://codereview.chromium.org/14244009/diff/4001/tools/test.dart
File tools/test.dart (right):

https://codereview.chromium.org/14244009/diff/4001/tools/test.dart#newcode238
tools/test.dart:238: if
(environment['DART_TESTING_DELETE_TEMPORARY_DIRECTORIES'] == '1') {
if (environment['DART_TESTING_DELETE_TEMPORARY_DIRECTORIES'] != '1') {
  return completer.complete(null);
}

Then you can avoid nesting the rest of the code.

https://codereview.chromium.org/14244009/diff/4001/tools/test.dart#newcode241
tools/test.dart:241: var dir = new Directory('').createTempSync();
I don't understand why it is OK to use synchronous API here.

https://codereview.chromium.org/14244009/diff/4001/tools/test.dart#newcode261
tools/test.dart:261: } catch (error) {}
On 2013/04/17 14:04:49, kustermann wrote:
> On 2013/04/15 13:35:47, ricow1 wrote:
> > maybe print this to our debug log?
> I can do it, but then people will get these messages when they run test.py
(the
> DebugLogger writes it only to a debug log if '--write-debug-log' was
specified).

Never ever catch all exceptions and ignore them :-)

Is there a more specific exception you can catch? OMG, no you can't. This is a
horrible API.

[ricow1, 2013-04-18 07:07:59]

The problem with doing this by default is that you have to communicate this,
people may run other code that will produce these temporary directories, and
what is in there (e.g., when they crash) may be important. If I needed to debug
a flaky tests I would run it in a loop for a few days and there may be leftover
temp dirs with exactly the info I need.

If we where to do this we would need to somehow be able to prefix the temp
directories created by a specific run of test.dart.

https://codereview.chromium.org/14244009/diff/4001/tools/test.dart
File tools/test.dart (right):

https://codereview.chromium.org/14244009/diff/4001/tools/test.dart#newcode272
tools/test.dart:272: deleteTemporaryDartDirectories().then((_) {
On 2013/04/17 14:04:49, kustermann wrote:
> On 2013/04/15 13:35:47, ricow1 wrote:
> > remove argument in closure
> 
> Future.then takes a closure with exactly one argument as far as I know.
Yes that is true

[kustermann, 2013-04-18 09:18:37]

Committed patchset #3 manually as r21669 (presubmit successful).

[kustermann, 2013-04-18 09:24:08]

https://codereview.chromium.org/14244009/diff/4001/tools/test.dart
File tools/test.dart (right):

https://codereview.chromium.org/14244009/diff/4001/tools/test.dart#newcode238
tools/test.dart:238: if
(environment['DART_TESTING_DELETE_TEMPORARY_DIRECTORIES'] == '1') {
On 2013/04/17 15:08:13, ahe wrote:
> if (environment['DART_TESTING_DELETE_TEMPORARY_DIRECTORIES'] != '1') {
>   return completer.complete(null);
> }
> 
> Then you can avoid nesting the rest of the code.

Unfortunatly, I saw that comment too late. I committed it a few minutes ago.
Personally I find both ways equivalent.

https://codereview.chromium.org/14244009/diff/4001/tools/test.dart#newcode241
tools/test.dart:241: var dir = new Directory('').createTempSync();
On 2013/04/17 15:08:13, ahe wrote:
> I don't understand why it is OK to use synchronous API here.

I don't understand why we have to use async API here. Using the synchronous API
is probably faster and it's easier to read. (test.dart uses synchronous APIs in
other places as well)

https://codereview.chromium.org/14244009/diff/4001/tools/test.dart#newcode261
tools/test.dart:261: } catch (error) {}
On 2013/04/17 15:08:13, ahe wrote:
> On 2013/04/17 14:04:49, kustermann wrote:
> > On 2013/04/15 13:35:47, ricow1 wrote:
> > > maybe print this to our debug log?
> > I can do it, but then people will get these messages when they run test.py
> (the
> > DebugLogger writes it only to a debug log if '--write-debug-log' was
> specified).
> 
> Never ever catch all exceptions and ignore them :-)
> 
> Is there a more specific exception you can catch? OMG, no you can't. This is a
> horrible API.

I added a DebugLogger.error() statment now.

[ahe, 2013-04-18 09:26:44]

https://codereview.chromium.org/14244009/diff/4001/tools/test.dart
File tools/test.dart (right):

https://codereview.chromium.org/14244009/diff/4001/tools/test.dart#newcode241
tools/test.dart:241: var dir = new Directory('').createTempSync();
On 2013/04/18 09:24:08, kustermann wrote:
> On 2013/04/17 15:08:13, ahe wrote:
> > I don't understand why it is OK to use synchronous API here.
> 
> I don't understand why we have to use async API here. Using the synchronous
API
> is probably faster and it's easier to read. (test.dart uses synchronous APIs
in
> other places as well)
> 

Why are you using async API then?  Why does deleteTemporaryDartDirectories
return a Future?  It seems to me that the rest of main will not start before
these directories are deleted, so why not use synchronous API?

[kustermann, 2013-04-18 09:29:32]

> Why are you using async API then?  Why does deleteTemporaryDartDirectories
> return a Future?  It seems to me that the rest of main will not start before
> these directories are deleted, so why not use synchronous API?

Short answer: I didn't know there was a 'listSync' method.