Add bazel worker support to the dev compiler.

lib/src/compiler/command.dart

Index: lib/src/compiler/command.dart
diff --git a/lib/src/compiler/command.dart b/lib/src/compiler/command.dart
index 2b0eca1c297132f40c324d4c4c14dccb066bb00d..50f526c98427d4eabbb02a8fc87205c71c0b9219 100644
--- a/lib/src/compiler/command.dart
+++ b/lib/src/compiler/command.dart
@@ -4,10 +4,12 @@
 
 import 'dart:convert' show JSON;
 import 'dart:io';
+import 'package:args/args.dart';
 import 'package:args/command_runner.dart';
 import 'package:analyzer/src/generated/source.dart' show Source;
 import 'package:analyzer/src/summary/package_bundle_reader.dart'
     show InSummarySource;
+import 'package:bazel_worker/bazel_worker.dart';
 import 'compiler.dart'
     show BuildUnit, CompilerOptions, JSModuleFile, ModuleCompiler;
 import '../analyzer/context.dart' show AnalyzerOptions;
@@ -20,26 +22,38 @@ class CompileCommand extends Command {
 
   CompileCommand() {
     argParser.addOption('out', abbr: 'o', help: 'Output file (required)');
+    argParser.addFlag('persistent_worker',

[Jennifer Messerly, 2016/04/14 21:02:14]

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?

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

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

+        help: 'Whether or not we are running as a persistent bazel worker. '

[Jennifer Messerly, 2016/04/14 21:02:14]

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 :)

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

Done.

+            'This is automatically added by bazel when running in worker mode.',
+        defaultsTo: false);

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

"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.

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

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

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

     CompilerOptions.addArguments(argParser);
     AnalyzerOptions.addArguments(argParser);
   }
 
   @override
   void run() {
-    var compilerOptions = new CompilerOptions.fromArguments(argResults);
     var compiler =
         new ModuleCompiler(new AnalyzerOptions.fromArguments(argResults));
+    if (argResults['persistent_worker'] == true) {

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

if you have defaultsTo: false, do you also need "== true"

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

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

+      new CompilerLoop(compiler, argParser, runCompiler, argResults.rest).run();
+    } else {
+      runCompiler(compiler, new CompilerOptions.fromArguments(argResults),
+          argResults['out'], argResults.rest);
+    }
+  }
 
-    var outPath = argResults['out'];
+  void runCompiler(ModuleCompiler compiler, CompilerOptions compilerOptions,

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

suggestion: rename to "compile"

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

Done.

+      String outPath, List<String> extraArgs,
+      {void forEachError(String error): print}) {
     if (outPath == null) {
       usageException('Please include the output file location. For example:\n'
           '    -o PATH/TO/OUTPUT_FILE.js');
     }
-    var unit = new BuildUnit(path.basenameWithoutExtension(outPath),
-        argResults.rest, _moduleForLibrary);
+    var unit = new BuildUnit(
+        path.basenameWithoutExtension(outPath), extraArgs, _moduleForLibrary);
 
     JSModuleFile module = compiler.compile(unit, compilerOptions);
-    module.errors.forEach(print);
+    module.errors.forEach(forEachError);
 
     if (!module.isValid) throw new CompileErrorException();
 
@@ -72,3 +86,36 @@ class CompileCommand extends Command {
 class CompileErrorException implements Exception {
   toString() => '\nPlease fix all errors before compiling (warnings are okay).';
 }
+
+typedef void RunCompilerFn(ModuleCompiler compiler, CompilerOptions options,

[Jennifer Messerly, 2016/04/14 21:02:14]

this can be removed if following suggestion below

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

Done.

+    String outPath, List<String> extraArgs,
+    {void forEachError(String error)});
+
+/// Runs the compiler worker loop.
+class CompilerLoop extends SyncWorkerLoop {

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

"_CompilerWorker" would be a better name, IMO.
(also note: making it private)

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

Done.

+  final ModuleCompiler compiler;
+  final ArgParser argParser;
+  final RunCompilerFn runCompilerFn;

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

This could just be a CompileCommand. That way we don't need to duplicate the
signature in RunCompilerFn.

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

Done.

+  final List<String> extraArgs;
+
+  CompilerLoop(
+      this.compiler, this.argParser, this.runCompilerFn, this.extraArgs)
+      : super();
+
+  WorkResponse performRequest(WorkRequest request) {
+    var argResults = argParser.parse(request.arguments);
+    var output = new StringBuffer();
+    try {
+      runCompilerFn(compiler, new CompilerOptions.fromArguments(argResults),

[Jennifer Messerly, 2016/04/14 21:02:14]

after suggestion above, this will become just "command.compile(...)"

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

Done.

+          argResults['out'], new List.from(argResults.rest)..addAll(extraArgs),
+          forEachError: output.writeln);
+      return new WorkResponse()
+        ..exitCode = EXIT_CODE_OK
+        ..output = output.toString();
+    } catch (e, s) {

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

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.

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

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

+      return new WorkResponse()
+        ..exitCode = EXIT_CODE_ERROR

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

not sure if they have different exit codes for "tool error" vs "input error" but
if so it would be good to distinguish.

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

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

+        ..output = '$output\n$e\n$s';
+    }
+  }
+}