Skeleton code for running the forthcoming async/await compiler on pub.

Skeleton code for running the forthcoming async/await compiler on pub.

R=nweiz@google.com

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

[Bob Nystrom, 2014-08-21 22:06:25]

This gets the framework in place to let pub use async/await syntax. It runs a
"compiler" that compiles Dart with async/await down to vanilla Dart and then
executes that.

This ensures that happens everywhere that pub is run:

1. When you run the "pub" shell script directly from the Dart repo.
2. When you run pub's tests.
3. When you build the SDK and generate a snapshot for pub.

Since this compilation happens when the SDK is built, the shipped pub is just a
snapshot of vanilla Dart code.

[nweiz, 2014-08-22 19:29:16]

This sounds a little weird, but I think we should add an "ASYNC!" to some pub
file to make sure it doesn't break anything.

https://codereview.chromium.org/472173004/diff/1/sdk/bin/pub
File sdk/bin/pub (right):

https://codereview.chromium.org/472173004/diff/1/sdk/bin/pub#newcode1
sdk/bin/pub:1: #!/bin/bash
Don't we need to do this for pub.cmd as well? Otherwise, won't the bots break?

https://codereview.chromium.org/472173004/diff/1/sdk/lib/_internal/pub/bin/as...
File sdk/lib/_internal/pub/bin/async_compile.dart (right):

https://codereview.chromium.org/472173004/diff/1/sdk/lib/_internal/pub/bin/as...
sdk/lib/_internal/pub/bin/async_compile.dart:23: void main(List<String>
arguments) {
This needs to be more resilient to being run in parallel. test.dart will run
many test suites at once, which will all try to invoke this. Maybe it should
take out some sort of lock in the build output directory?

https://codereview.chromium.org/472173004/diff/1/sdk/lib/_internal/pub/bin/as...
sdk/lib/_internal/pub/bin/async_compile.dart:28: "Invalid argument
'${arguments[1]}");
Why not just use args? It would make this parsing clearer and make using this
more flexible from the command line.

https://codereview.chromium.org/472173004/diff/1/sdk/lib/_internal/pub/bin/as...
sdk/lib/_internal/pub/bin/async_compile.dart:31: // Create the build output
directory if not already there.
Nit: "if not" -> "if it's not"

https://codereview.chromium.org/472173004/diff/1/sdk/lib/_internal/pub/bin/as...
sdk/lib/_internal/pub/bin/async_compile.dart:35: }
This script lives in pub; I'd just import pub's io library and use ensureDir
here.

https://codereview.chromium.org/472173004/diff/1/sdk/lib/_internal/pub/bin/as...
sdk/lib/_internal/pub/bin/async_compile.dart:59: if (!silent) print("Compiled
${sourceFile.path}");
Nit: add a period.

https://codereview.chromium.org/472173004/diff/1/sdk/lib/_internal/pub/bin/as...
sdk/lib/_internal/pub/bin/async_compile.dart:66: final _compilerPattern = new
RegExp(r'(\.\./)+compiler');
I'd feel a little better if this at least started with ^import.

https://codereview.chromium.org/472173004/diff/1/sdk/lib/_internal/pub/bin/as...
sdk/lib/_internal/pub/bin/async_compile.dart:79: var relative =
p.relative(compilerDir, from: destDir.path);
Use p.url for manipulating import paths.

https://codereview.chromium.org/472173004/diff/1/sdk/lib/_internal/pub/test/t...
File sdk/lib/_internal/pub/test/test_pub.dart (right):

https://codereview.chromium.org/472173004/diff/1/sdk/lib/_internal/pub/test/t...
sdk/lib/_internal/pub/test/test_pub.dart:622: // Run the async/await compiler if
needed.
When I first read this, I was confused as to why every test suite was running
the compiler. Add a comment here explaining that async_compile will be a no-op
if the code is up-to-date.

[ricow1, 2014-08-25 06:22:04]

dbc

https://codereview.chromium.org/472173004/diff/1/tools/create_pub_snapshot.py
File tools/create_pub_snapshot.py (right):

https://codereview.chromium.org/472173004/diff/1/tools/create_pub_snapshot.py...
tools/create_pub_snapshot.py:43:
'../../sdk/lib/_internal/pub/bin/async_compile.dart',
please add a comment about what this is relative to, or even better, pass this
in from the gyp file where it is obvious

https://codereview.chromium.org/472173004/diff/1/tools/create_pub_snapshot.py...
tools/create_pub_snapshot.py:50: subprocess.call([
set status = this call, otherwise the exitcode is ignored

[Bob Nystrom, 2014-08-25 17:40:01]

> This sounds a little weird, but I think we should add an "ASYNC!" to some pub
file to make sure it doesn't break anything.

That's what I did locally in my testing, but I didn't want to commit that just
yet. I want to get the Windows batch script working and make sure everything
else here is OK. Once this lands and the bots are happy, I'll add an ASYNC! and
see what happens. (My guess is it will give the analyzer bots a bad day.)

https://codereview.chromium.org/472173004/diff/1/sdk/bin/pub
File sdk/bin/pub (right):

https://codereview.chromium.org/472173004/diff/1/sdk/bin/pub#newcode1
sdk/bin/pub:1: #!/bin/bash
On 2014/08/22 19:29:15, nweiz wrote:
> Don't we need to do this for pub.cmd as well? 

Yes, I'll do the Windows side of things in a separate patch. (It's too much of a
pain to switch between machines in one patch.)

> Otherwise, won't the bots break?

This patch doesn't *rely* on the async compiler yet, so I think the Windows bots
will be OK.

https://codereview.chromium.org/472173004/diff/1/sdk/lib/_internal/pub/bin/as...
File sdk/lib/_internal/pub/bin/async_compile.dart (right):

https://codereview.chromium.org/472173004/diff/1/sdk/lib/_internal/pub/bin/as...
sdk/lib/_internal/pub/bin/async_compile.dart:23: void main(List<String>
arguments) {
On 2014/08/22 19:29:15, nweiz wrote:
> This needs to be more resilient to being run in parallel. test.dart will run
> many test suites at once, which will all try to invoke this. Maybe it should
> take out some sort of lock in the build output directory?

Done.

It doesn't check the directory for existence before creating it since that's a
potential race condition. Instead, it just always calls
directory.create(recursive: true), which will do nothing if the directory
already exists.

When it writes the output file, it catches and ignores an IO exception there
since it likely means another compiler is writing the same file.

I think that covers all of the cases. I haven't seen any issues running the
tests locally, though that doesn't necessarily mean that much. I'll keep an eye
on the bots after this lands.

https://codereview.chromium.org/472173004/diff/1/sdk/lib/_internal/pub/bin/as...
sdk/lib/_internal/pub/bin/async_compile.dart:28: "Invalid argument
'${arguments[1]}");
On 2014/08/22 19:29:15, nweiz wrote:
> Why not just use args? It would make this parsing clearer and make using this
> more flexible from the command line.

It felt like more trouble than it's worth. This script is temporary and isn't
something I expect any end users to use.

https://codereview.chromium.org/472173004/diff/1/sdk/lib/_internal/pub/bin/as...
sdk/lib/_internal/pub/bin/async_compile.dart:31: // Create the build output
directory if not already there.
On 2014/08/22 19:29:15, nweiz wrote:
> Nit: "if not" -> "if it's not"

Done.

https://codereview.chromium.org/472173004/diff/1/sdk/lib/_internal/pub/bin/as...
sdk/lib/_internal/pub/bin/async_compile.dart:35: }
On 2014/08/22 19:29:15, nweiz wrote:
> This script lives in pub; I'd just import pub's io library and use ensureDir
> here.

We'll probably want to use async syntax in io.dart before too long. :)

This script, by design, has minimal dependencies since anything it imports can't
use async/await.

https://codereview.chromium.org/472173004/diff/1/sdk/lib/_internal/pub/bin/as...
sdk/lib/_internal/pub/bin/async_compile.dart:59: if (!silent) print("Compiled
${sourceFile.path}");
On 2014/08/22 19:29:15, nweiz wrote:
> Nit: add a period.

Done.

https://codereview.chromium.org/472173004/diff/1/sdk/lib/_internal/pub/bin/as...
sdk/lib/_internal/pub/bin/async_compile.dart:66: final _compilerPattern = new
RegExp(r'(\.\./)+compiler');
On 2014/08/22 19:29:15, nweiz wrote:
> I'd feel a little better if this at least started with ^import.

Done.

https://codereview.chromium.org/472173004/diff/1/sdk/lib/_internal/pub/bin/as...
sdk/lib/_internal/pub/bin/async_compile.dart:79: var relative =
p.relative(compilerDir, from: destDir.path);
On 2014/08/22 19:29:15, nweiz wrote:
> Use p.url for manipulating import paths.

Done.

https://codereview.chromium.org/472173004/diff/1/sdk/lib/_internal/pub/test/t...
File sdk/lib/_internal/pub/test/test_pub.dart (right):

https://codereview.chromium.org/472173004/diff/1/sdk/lib/_internal/pub/test/t...
sdk/lib/_internal/pub/test/test_pub.dart:622: // Run the async/await compiler if
needed.
On 2014/08/22 19:29:16, nweiz wrote:
> When I first read this, I was confused as to why every test suite was running
> the compiler. Add a comment here explaining that async_compile will be a no-op
> if the code is up-to-date.

Done.

https://codereview.chromium.org/472173004/diff/1/tools/create_pub_snapshot.py
File tools/create_pub_snapshot.py (right):

https://codereview.chromium.org/472173004/diff/1/tools/create_pub_snapshot.py...
tools/create_pub_snapshot.py:43:
'../../sdk/lib/_internal/pub/bin/async_compile.dart',
On 2014/08/25 06:22:03, ricow1 wrote:
> please add a comment about what this is relative to, or even better, pass this
> in from the gyp file where it is obvious

Done.

https://codereview.chromium.org/472173004/diff/1/tools/create_pub_snapshot.py...
tools/create_pub_snapshot.py:50: subprocess.call([
On 2014/08/25 06:22:03, ricow1 wrote:
> set status = this call, otherwise the exitcode is ignored

Oops, good catch! Fixed.

[nweiz, 2014-08-25 19:40:56]

lgtm

https://codereview.chromium.org/472173004/diff/1/sdk/lib/_internal/pub/bin/as...
File sdk/lib/_internal/pub/bin/async_compile.dart (right):

https://codereview.chromium.org/472173004/diff/1/sdk/lib/_internal/pub/bin/as...
sdk/lib/_internal/pub/bin/async_compile.dart:23: void main(List<String>
arguments) {
On 2014/08/25 17:40:00, Bob Nystrom wrote:
> On 2014/08/22 19:29:15, nweiz wrote:
> > This needs to be more resilient to being run in parallel. test.dart will run
> > many test suites at once, which will all try to invoke this. Maybe it should
> > take out some sort of lock in the build output directory?
> 
> Done.
> 
> It doesn't check the directory for existence before creating it since that's a
> potential race condition. Instead, it just always calls
> directory.create(recursive: true), which will do nothing if the directory
> already exists.
> 
> When it writes the output file, it catches and ignores an IO exception there
> since it likely means another compiler is writing the same file.
> 
> I think that covers all of the cases. I haven't seen any issues running the
> tests locally, though that doesn't necessarily mean that much. I'll keep an
eye
> on the bots after this lands.

This is probably sufficient to avoid crashes, but multiple instances of
async_compile will still probably do a lot of redundant work.

[Bob Nystrom, 2014-08-25 20:34:12]

On 2014/08/25 19:40:56, nweiz wrote:
> lgtm
> 
>
https://codereview.chromium.org/472173004/diff/1/sdk/lib/_internal/pub/bin/as...
> File sdk/lib/_internal/pub/bin/async_compile.dart (right):
> 
>
https://codereview.chromium.org/472173004/diff/1/sdk/lib/_internal/pub/bin/as...
> sdk/lib/_internal/pub/bin/async_compile.dart:23: void main(List<String>
> arguments) {
> On 2014/08/25 17:40:00, Bob Nystrom wrote:
> > On 2014/08/22 19:29:15, nweiz wrote:
> > > This needs to be more resilient to being run in parallel. test.dart will
run
> > > many test suites at once, which will all try to invoke this. Maybe it
should
> > > take out some sort of lock in the build output directory?
> > 
> > Done.
> > 
> > It doesn't check the directory for existence before creating it since that's
a
> > potential race condition. Instead, it just always calls
> > directory.create(recursive: true), which will do nothing if the directory
> > already exists.
> > 
> > When it writes the output file, it catches and ignores an IO exception there
> > since it likely means another compiler is writing the same file.
> > 
> > I think that covers all of the cases. I haven't seen any issues running the
> > tests locally, though that doesn't necessarily mean that much. I'll keep an
> eye
> > on the bots after this lands.
> 
> This is probably sufficient to avoid crashes, but multiple instances of
> async_compile will still probably do a lot of redundant work.

I could be wrong, but I don't think it will be too bad. They'll only do
redundant work when:

1. They are starting up at the same time.
2. Some .dart source files have changed and actually need to be recompiled.

In the common case where nothing has changed, no work will happen.

- bob

[Bob Nystrom, 2014-08-25 20:39:09]

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