Add bazel worker support to the dev compiler.

Add bazel worker support to the dev compiler.

BUG=
R=jmesserly@google.com, vsm@google.com

Committed: https://github.com/dart-lang/dev_compiler/commit/99dca29d6ab8c94abd18cf405eae78f469c1d221

[jakemac, 2016-04-14 20:02:34]

jakemac@google.com changed reviewers:
+ jmesserly@google.com, vsm@google.com

[Jennifer Messerly, 2016-04-14 21:02:15]

Some suggestions but this looks awesome! It's nice how easy bazel_worker makes
it to plug in!

https://codereview.chromium.org/1884073003/diff/20001/bin/dartdevc.dart
File bin/dartdevc.dart (right):

https://codereview.chromium.org/1884073003/diff/20001/bin/dartdevc.dart#newco...
bin/dartdevc.dart:83: /// and add those as args.
add a comment that this is required by Bazel workers?

https://codereview.chromium.org/1884073003/diff/20001/lib/src/compiler/comman...
File lib/src/compiler/command.dart (right):

https://codereview.chromium.org/1884073003/diff/20001/lib/src/compiler/comman...
lib/src/compiler/command.dart:25: argParser.addFlag('persistent_worker',
Hmmm, we use dash "-" instead of underscore "_" for our other options
would be good to be consistent. I guess with Bazel we have no choice?

https://codereview.chromium.org/1884073003/diff/20001/lib/src/compiler/comman...
lib/src/compiler/command.dart:26: help: 'Whether or not we are running as a
persistent bazel worker. '
few UI comments:
* should "Bazel" be capitalized as it's a proper name?
* it can be nice to add links, especially if they're short: http://bazel.io/
* "args" does not automatically line break, so adding "\n" is good, makes help
more readable.
* short help strings are good. I'd probably reword this as:

"run in a persistent Bazel worker (http://bazel.io/)\n"

Although see comment below, about hiding the option :)

https://codereview.chromium.org/1884073003/diff/20001/lib/src/compiler/comman...
lib/src/compiler/command.dart:28: defaultsTo: false);
"defaultsTo: false" is not usually needed, but maybe it helps skip the "== true"
check below

Should we hide this option, as it does not need to be specified by users? That
would be my preference.

https://codereview.chromium.org/1884073003/diff/20001/lib/src/compiler/comman...
lib/src/compiler/command.dart:37: if (argResults['persistent_worker'] == true) {
if you have defaultsTo: false, do you also need "== true"

https://codereview.chromium.org/1884073003/diff/20001/lib/src/compiler/comman...
lib/src/compiler/command.dart:45: void runCompiler(ModuleCompiler compiler,
CompilerOptions compilerOptions,
suggestion: rename to "compile"

https://codereview.chromium.org/1884073003/diff/20001/lib/src/compiler/comman...
lib/src/compiler/command.dart:90: typedef void RunCompilerFn(ModuleCompiler
compiler, CompilerOptions options,
this can be removed if following suggestion below

https://codereview.chromium.org/1884073003/diff/20001/lib/src/compiler/comman...
lib/src/compiler/command.dart:95: class CompilerLoop extends SyncWorkerLoop {
"_CompilerWorker" would be a better name, IMO.
(also note: making it private)

https://codereview.chromium.org/1884073003/diff/20001/lib/src/compiler/comman...
lib/src/compiler/command.dart:98: final RunCompilerFn runCompilerFn;
This could just be a CompileCommand. That way we don't need to duplicate the
signature in RunCompilerFn.

https://codereview.chromium.org/1884073003/diff/20001/lib/src/compiler/comman...
lib/src/compiler/command.dart:109: runCompilerFn(compiler, new
CompilerOptions.fromArguments(argResults),
after suggestion above, this will become just "command.compile(...)"

https://codereview.chromium.org/1884073003/diff/20001/lib/src/compiler/comman...
lib/src/compiler/command.dart:115: } catch (e, s) {
Rather than catch all exceptions, it may be worth treating UsageException and
CompileErrorException differently:
https://github.com/dart-lang/dev_compiler/blob/854e24eddafa6376c0b95bea367a03...

In particular, the stack trace is not needed for those 2. They indicate problems
with the input. Other things should include a stack as they're likely compiler
bugs. BTW if it helps, feel free to refactor some of that logic from
bin/dartdevc.

https://codereview.chromium.org/1884073003/diff/20001/lib/src/compiler/comman...
lib/src/compiler/command.dart:117: ..exitCode = EXIT_CODE_ERROR
not sure if they have different exit codes for "tool error" vs "input error" but
if so it would be good to distinguish.

https://codereview.chromium.org/1884073003/diff/20001/test/worker/worker_test...
File test/worker/worker_test.dart (right):

https://codereview.chromium.org/1884073003/diff/20001/test/worker/worker_test...
test/worker/worker_test.dart:66: test('can compile in basic mode', () async {
If you don't mind, delete my little command_test.sh and test/command folder and
calling command_test from test.sh? since you cover it here

[jakemac, 2016-04-14 21:48:25]

https://codereview.chromium.org/1884073003/diff/20001/bin/dartdevc.dart
File bin/dartdevc.dart (right):

https://codereview.chromium.org/1884073003/diff/20001/bin/dartdevc.dart#newco...
bin/dartdevc.dart:83: /// and add those as args.
On 2016/04/14 21:02:14, John Messerly wrote:
> add a comment that this is required by Bazel workers?

Done.

https://codereview.chromium.org/1884073003/diff/20001/lib/src/compiler/comman...
File lib/src/compiler/command.dart (right):

https://codereview.chromium.org/1884073003/diff/20001/lib/src/compiler/comman...
lib/src/compiler/command.dart:25: argParser.addFlag('persistent_worker',
On 2016/04/14 21:02:14, John Messerly wrote:
> Hmmm, we use dash "-" instead of underscore "_" for our other options
> would be good to be consistent. I guess with Bazel we have no choice?

Ya, unfortunately we don't have any control over this one :(

https://codereview.chromium.org/1884073003/diff/20001/lib/src/compiler/comman...
lib/src/compiler/command.dart:26: help: 'Whether or not we are running as a
persistent bazel worker. '
On 2016/04/14 21:02:14, John Messerly wrote:
> few UI comments:
> * should "Bazel" be capitalized as it's a proper name?
> * it can be nice to add links, especially if they're short: http://bazel.io/
> * "args" does not automatically line break, so adding "\n" is good, makes help
> more readable.
> * short help strings are good. I'd probably reword this as:
> 
> "run in a persistent Bazel worker (http://bazel.io/)%5Cn%22
> 
> Although see comment below, about hiding the option :)

Done.

https://codereview.chromium.org/1884073003/diff/20001/lib/src/compiler/comman...
lib/src/compiler/command.dart:28: defaultsTo: false);
On 2016/04/14 21:02:15, John Messerly wrote:
> "defaultsTo: false" is not usually needed, but maybe it helps skip the "==
true"
> check below
> 
> Should we hide this option, as it does not need to be specified by users? That
> would be my preference.

Nice, I didn't realize there was an option to hide args.

I also left the `defaultsTo: false` and then removed the `== true` below.

https://codereview.chromium.org/1884073003/diff/20001/lib/src/compiler/comman...
lib/src/compiler/command.dart:37: if (argResults['persistent_worker'] == true) {
On 2016/04/14 21:02:15, John Messerly wrote:
> if you have defaultsTo: false, do you also need "== true"

Done, I was definitely being a bit overly defensive here.

https://codereview.chromium.org/1884073003/diff/20001/lib/src/compiler/comman...
lib/src/compiler/command.dart:45: void runCompiler(ModuleCompiler compiler,
CompilerOptions compilerOptions,
On 2016/04/14 21:02:15, John Messerly wrote:
> suggestion: rename to "compile"

Done.

https://codereview.chromium.org/1884073003/diff/20001/lib/src/compiler/comman...
lib/src/compiler/command.dart:90: typedef void RunCompilerFn(ModuleCompiler
compiler, CompilerOptions options,
On 2016/04/14 21:02:14, John Messerly wrote:
> this can be removed if following suggestion below

Done.

https://codereview.chromium.org/1884073003/diff/20001/lib/src/compiler/comman...
lib/src/compiler/command.dart:95: class CompilerLoop extends SyncWorkerLoop {
On 2016/04/14 21:02:15, John Messerly wrote:
> "_CompilerWorker" would be a better name, IMO.
> (also note: making it private)

Done.

https://codereview.chromium.org/1884073003/diff/20001/lib/src/compiler/comman...
lib/src/compiler/command.dart:98: final RunCompilerFn runCompilerFn;
On 2016/04/14 21:02:15, John Messerly wrote:
> This could just be a CompileCommand. That way we don't need to duplicate the
> signature in RunCompilerFn.

Done.

https://codereview.chromium.org/1884073003/diff/20001/lib/src/compiler/comman...
lib/src/compiler/command.dart:109: runCompilerFn(compiler, new
CompilerOptions.fromArguments(argResults),
On 2016/04/14 21:02:14, John Messerly wrote:
> after suggestion above, this will become just "command.compile(...)"

Done.

https://codereview.chromium.org/1884073003/diff/20001/lib/src/compiler/comman...
lib/src/compiler/command.dart:115: } catch (e, s) {
On 2016/04/14 21:02:15, John Messerly wrote:
> Rather than catch all exceptions, it may be worth treating UsageException and
> CompileErrorException differently:
>
https://github.com/dart-lang/dev_compiler/blob/854e24eddafa6376c0b95bea367a03...
> 
> In particular, the stack trace is not needed for those 2. They indicate
problems
> with the input. Other things should include a stack as they're likely compiler
> bugs. BTW if it helps, feel free to refactor some of that logic from
> bin/dartdevc.

Done, moved the error handling logic into a shared function.

https://codereview.chromium.org/1884073003/diff/20001/lib/src/compiler/comman...
lib/src/compiler/command.dart:117: ..exitCode = EXIT_CODE_ERROR
On 2016/04/14 21:02:15, John Messerly wrote:
> not sure if they have different exit codes for "tool error" vs "input error"
but
> if so it would be good to distinguish.

Afaik bazel does not do anything with the error code other than print it to the
user.

https://codereview.chromium.org/1884073003/diff/20001/test/worker/worker_test...
File test/worker/worker_test.dart (right):

https://codereview.chromium.org/1884073003/diff/20001/test/worker/worker_test...
test/worker/worker_test.dart:66: test('can compile in basic mode', () async {
On 2016/04/14 21:02:15, John Messerly wrote:
> If you don't mind, delete my little command_test.sh and test/command folder
and
> calling command_test from test.sh? since you cover it here

Done.

[Jennifer Messerly, 2016-04-14 22:07:28]

LGTM!

[vsm, 2016-04-14 22:20:24]

lgtm

[jakemac, 2016-04-15 13:51:44]

Description was changed from

==========
Add bazel worker support to the dev compiler.

BUG=
==========

to

==========
Add bazel worker support to the dev compiler.

BUG=
R=jmesserly@google.com, vsm@google.com

Committed:
https://github.com/dart-lang/dev_compiler/commit/99dca29d6ab8c94abd18cf405eae...
==========

[jakemac, 2016-04-15 13:51:46]

Committed patchset #3 (id:40001) manually as
99dca29d6ab8c94abd18cf405eae78f469c1d221 (presubmit successful).