add bazel worker for summary generation

add bazel worker for summary generation

BUG=
R=sigmund@google.com

Committed: https://github.com/dart-lang/sdk/commit/649345392a4ed96a9203a393a8bf7c64b8930099

[jakemac, 2017-08-18 17:29:31]

jakemac@google.com changed reviewers:
+ sigmund@google.com

[Siggi Cherem (dart-lang), 2017-08-21 20:16:49]

thanks Jake!

lgtm with minor comments.

https://codereview.chromium.org/2999263002/diff/1/pkg/front_end/tool/bazel/wo...
File pkg/front_end/tool/bazel/worker.dart (right):

https://codereview.chromium.org/2999263002/diff/1/pkg/front_end/tool/bazel/wo...
pkg/front_end/tool/bazel/worker.dart:13: main(List<String> args) async {
I'm still contemplating whether to put this here or in a new `bazel_integration`
package. I personally prefer the latter, but we can debate further if you prefer
(I'm also OK making it a TODO for now).

https://codereview.chromium.org/2999263002/diff/1/pkg/front_end/tool/bazel/wo...
pkg/front_end/tool/bazel/worker.dart:93: options.setExitCodeOnProblem = false;
this is already false by default :)

In your case I recommend to not use this option entirely, instead just rely on
the onError handler below like you do in the worker case.

https://codereview.chromium.org/2999263002/diff/1/pkg/front_end/tool/bazel/wo...
pkg/front_end/tool/bazel/worker.dart:101: throw error;
`throw` might not be the most user-friendly response when using this as a bazel
aspect. How about using an onError handler that records all errors and sets an
exit code instead?

Same below for the `else` case (instead of throwOnWarnigns)

https://codereview.chromium.org/2999263002/diff/1/pkg/front_end/tool/bazel/wo...
pkg/front_end/tool/bazel/worker.dart:105: options.throwOnWarnings = true;
our default error reporting throws and prints to the console. The printing tries
to use colors, but if you want to tune this further, you'd need to use the
onError callabck.

https://codereview.chromium.org/2999263002/diff/1/pkg/front_end/tool/bazel/wo...
pkg/front_end/tool/bazel/worker.dart:108: var sources =
parsedArgs['source'].map(Uri.parse).toList();
we intend to fix this, but we've had issues in the past when some URis don't
have a scheme. I believe it is not an issue with summaries, but it can be an
issue with the sdk-root and source files. In that case, you may want to use
(Uri.base.resolve) here.

I have a feeling thought that you are always passing Uris as arguments anyways
(package:* or multi-root:///*), correct? If so, feel free to ignore this.

[jakemac, 2017-08-21 21:16:38]

https://codereview.chromium.org/2999263002/diff/1/pkg/front_end/tool/bazel/wo...
File pkg/front_end/tool/bazel/worker.dart (right):

https://codereview.chromium.org/2999263002/diff/1/pkg/front_end/tool/bazel/wo...
pkg/front_end/tool/bazel/worker.dart:13: main(List<String> args) async {
On 2017/08/21 20:16:49, Siggi Cherem (dart-lang) wrote:
> I'm still contemplating whether to put this here or in a new
`bazel_integration`
> package. I personally prefer the latter, but we can debate further if you
prefer
> (I'm also OK making it a TODO for now).

I don't feel that strongly, but analyzer/ddc both natively support bazel in
their main binary, and it is useful (pub uses the bazel worker protocol for
ddc/analyzer as well).

Since there isn't really a canonical entrypoint for front_end (that I could
find) I just created this one but ideally it would be merged into some more
general one at some point?

https://codereview.chromium.org/2999263002/diff/1/pkg/front_end/tool/bazel/wo...
pkg/front_end/tool/bazel/worker.dart:93: options.setExitCodeOnProblem = false;
On 2017/08/21 20:16:49, Siggi Cherem (dart-lang) wrote:
> this is already false by default :)
> 
> In your case I recommend to not use this option entirely, instead just rely on
> the onError handler below like you do in the worker case.

done

https://codereview.chromium.org/2999263002/diff/1/pkg/front_end/tool/bazel/wo...
pkg/front_end/tool/bazel/worker.dart:101: throw error;
On 2017/08/21 20:16:49, Siggi Cherem (dart-lang) wrote:
> `throw` might not be the most user-friendly response when using this as a
bazel
> aspect. How about using an onError handler that records all errors and sets an
> exit code instead?
> 
> Same below for the `else` case (instead of throwOnWarnigns)

The bazel worker handles errors and sends a proper response etc automatically.

https://codereview.chromium.org/2999263002/diff/1/pkg/front_end/tool/bazel/wo...
pkg/front_end/tool/bazel/worker.dart:105: options.throwOnWarnings = true;
On 2017/08/21 20:16:49, Siggi Cherem (dart-lang) wrote:
> our default error reporting throws and prints to the console. The printing
tries
> to use colors, but if you want to tune this further, you'd need to use the
> onError callabck.

I think the default handling is fine

https://codereview.chromium.org/2999263002/diff/1/pkg/front_end/tool/bazel/wo...
pkg/front_end/tool/bazel/worker.dart:108: var sources =
parsedArgs['source'].map(Uri.parse).toList();
On 2017/08/21 20:16:49, Siggi Cherem (dart-lang) wrote:
> we intend to fix this, but we've had issues in the past when some URis don't
> have a scheme. I believe it is not an issue with summaries, but it can be an
> issue with the sdk-root and source files. In that case, you may want to use
> (Uri.base.resolve) here.
> 
> I have a feeling thought that you are always passing Uris as arguments anyways
> (package:* or multi-root:///*), correct? If so, feel free to ignore this.
> 

These will be real uris with schemes yes

[jakemac, 2017-08-21 21:23:02]

Description was changed from

==========
add bazel worker for summary generation

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

to

==========
add bazel worker for summary generation

BUG=
R=sigmund@google.com

Committed:
https://github.com/dart-lang/sdk/commit/649345392a4ed96a9203a393a8bf7c64b8930099
==========

[jakemac, 2017-08-21 21:23:04]

Committed patchset #2 (id:20001) manually as
649345392a4ed96a9203a393a8bf7c64b8930099 (presubmit successful).

[ahe, 2017-08-24 11:14:34]

ahe@google.com changed reviewers:
+ ahe@google.com

[ahe, 2017-08-24 11:14:35]

https://codereview.chromium.org/2999263002/diff/20001/pkg/front_end/tool/baze...
File pkg/front_end/tool/bazel/worker.dart (right):

https://codereview.chromium.org/2999263002/diff/20001/pkg/front_end/tool/baze...
pkg/front_end/tool/bazel/worker.dart:101: options.throwOnWarnings = true;
This is an internal option that makes Fasta crash prematurely. It don't think
you want that.

[jakemac, 2017-08-29 15:08:53]

On 2017/08/24 11:14:35, ahe wrote:
>
https://codereview.chromium.org/2999263002/diff/20001/pkg/front_end/tool/baze...
> File pkg/front_end/tool/bazel/worker.dart (right):
> 
>
https://codereview.chromium.org/2999263002/diff/20001/pkg/front_end/tool/baze...
> pkg/front_end/tool/bazel/worker.dart:101: options.throwOnWarnings = true;
> This is an internal option that makes Fasta crash prematurely. It don't think
> you want that.

In general we do want to fail on warnings as well as errors internally, and the
earlier the better.

In worker mode we wrap this in a try/catch so it won't crash the worker, and in
standalone mode throwing with a stack trace is the desired behavior.

Is your concern that we won't report all errors since we error immediately?

[ahe, 2017-08-29 15:16:26]

On 2017/08/29 15:08:53, jakemac wrote:
> On 2017/08/24 11:14:35, ahe wrote:
> >
>
https://codereview.chromium.org/2999263002/diff/20001/pkg/front_end/tool/baze...
> > File pkg/front_end/tool/bazel/worker.dart (right):
> > 
> >
>
https://codereview.chromium.org/2999263002/diff/20001/pkg/front_end/tool/baze...
> > pkg/front_end/tool/bazel/worker.dart:101: options.throwOnWarnings = true;
> > This is an internal option that makes Fasta crash prematurely. It don't
think
> > you want that.
> 
> In general we do want to fail on warnings as well as errors internally, and
the
> earlier the better.
> 
> In worker mode we wrap this in a try/catch so it won't crash the worker, and
in
> standalone mode throwing with a stack trace is the desired behavior.
> 
> Is your concern that we won't report all errors since we error immediately?

My concern is that you're using a debugging option that probably shouldn't be
exposed this way in the API.

It would be more future proof if you use onError.