Use rmdir to delete pkgbuild directories on windows

Use rmdir to delete pkgbuild directories on windows

We are seeing issues with this all the time. I validated that this works on my windows box (and our current setup does not)

It sucks that we need to us runInShell, but rmdir is not part of the normal path (i.e., "where rmdir" says it can't find it, but e.g., "where xcopy" works fine)
Committed: https://code.google.com/p/dart/source/detail?r=39071

[ricow1, 2014-08-11 08:14:30]

Anders: fyi, I don't know why rmdir would succeed and we would not?

[Bill Hesse, 2014-08-11 09:24:13]

https://chromiumcodereview.appspot.com/462523002/diff/20001/tools/testing/dar...
File tools/testing/dart/test_suite.dart (right):

https://chromiumcodereview.appspot.com/462523002/diff/20001/tools/testing/dar...
tools/testing/dart/test_suite.dart:1957: if (Platform.operatingSystem ==
'windows') {
Can we include a bug number or TODO about making delete(recursive: true) work
flawlessly on Windows, and removing this?

https://chromiumcodereview.appspot.com/462523002/diff/20001/tools/testing/dar...
tools/testing/dart/test_suite.dart:1964: throw new Exception('Can\'t delete path
$native_path. '
Maybe you should check the length of the path before suggesting it is the
problem.  It will be bad if people get this message for C:\temp\foo.  Beacuse
ends not in t.

https://chromiumcodereview.appspot.com/462523002/diff/40001/tools/testing/dar...
File tools/testing/dart/test_suite.dart (right):

https://chromiumcodereview.appspot.com/462523002/diff/40001/tools/testing/dar...
tools/testing/dart/test_suite.dart:1954: static Future deleteDirectory(String
path) {
Are there other places in the test scripts where we are already using
Process.run('rmdir'...), and should call this instead?  Even if not, there are
probably more places we should use this function.

[ricow1, 2014-08-11 09:37:31]

https://chromiumcodereview.appspot.com/462523002/diff/20001/tools/testing/dar...
File tools/testing/dart/test_suite.dart (right):

https://chromiumcodereview.appspot.com/462523002/diff/20001/tools/testing/dar...
tools/testing/dart/test_suite.dart:1957: if (Platform.operatingSystem ==
'windows') {
On 2014/08/11 09:24:13, Bill Hesse wrote:
> Can we include a bug number or TODO about making delete(recursive: true) work
> flawlessly on Windows, and removing this?
Done

https://chromiumcodereview.appspot.com/462523002/diff/20001/tools/testing/dar...
tools/testing/dart/test_suite.dart:1964: throw new Exception('Can\'t delete path
$native_path. '
On 2014/08/11 09:24:13, Bill Hesse wrote:
> Maybe you should check the length of the path before suggesting it is the
> problem.  It will be bad if people get this message for C:\temp\foo.  Beacuse
> ends not in t.

That will actually throw an ProcessException, we will not get in here

https://chromiumcodereview.appspot.com/462523002/diff/40001/tools/testing/dar...
File tools/testing/dart/test_suite.dart (right):

https://chromiumcodereview.appspot.com/462523002/diff/40001/tools/testing/dar...
tools/testing/dart/test_suite.dart:1954: static Future deleteDirectory(String
path) {
On 2014/08/11 09:24:13, Bill Hesse wrote:
> Are there other places in the test scripts where we are already using
> Process.run('rmdir'...), and should call this instead?
Nope:
ricow@nobbo:~/src/dart/dart/tools$ find -name "*.dart" | xargs grep rmdir
ricow@nobbo:~/src/dart/dart/tools$

>  Even if not, there are
> probably more places we should use this function.
If we see issues we can convert to this

[Bill Hesse, 2014-08-11 10:22:17]

lgtm

https://chromiumcodereview.appspot.com/462523002/diff/60001/tools/testing/dar...
File tools/testing/dart/test_suite.dart (right):

https://chromiumcodereview.appspot.com/462523002/diff/60001/tools/testing/dar...
tools/testing/dart/test_suite.dart:1966: 'This may be because the path is to
long');
If you are going to say this, it should say "This might be because the path
might be too long', or just "The path might be too long".

[ricow1, 2014-08-11 10:41:26]

https://chromiumcodereview.appspot.com/462523002/diff/60001/tools/testing/dar...
File tools/testing/dart/test_suite.dart (right):

https://chromiumcodereview.appspot.com/462523002/diff/60001/tools/testing/dar...
tools/testing/dart/test_suite.dart:1966: 'This may be because the path is to
long');
On 2014/08/11 10:22:17, Bill Hesse wrote:
> If you are going to say this, it should say "This might be because the path
> might be too long', or just "The path might be too long".

Done.