Support JSON output for pub build.

Support JSON output for pub build.

BUG=https://code.google.com/p/dart/issues/detail?id=15018
R=nweiz@google.com

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

[Bob Nystrom, 2014-03-05 00:55:36]

https://codereview.chromium.org/184113007/diff/60001/editor/tools/plugins/com...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/utilities/ast/AstComparator.java
(right):

https://codereview.chromium.org/184113007/diff/60001/editor/tools/plugins/com...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/utilities/ast/AstComparator.java:1:
/*
This is the result of a dumb merge mistake on my part.

[nweiz, 2014-03-05 02:36:51]

https://codereview.chromium.org/184113007/diff/60001/sdk/lib/_internal/pub/li...
File sdk/lib/_internal/pub/lib/src/command.dart (right):

https://codereview.chromium.org/184113007/diff/60001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/command.dart:199: /// Throw an
[ApplicationException] for a data error of this command with
I don't understand what a "data error of this command" is or what it means to
throw an ApplicationException for one. It also seems weird that this is a
separate function rather than just having people write "throw new
DataException(...)". I get that it's similar to [usageError], but that adds
extra value where this doesn't.

https://codereview.chromium.org/184113007/diff/60001/sdk/lib/_internal/pub/li...
File sdk/lib/_internal/pub/lib/src/command/build.dart (right):

https://codereview.chromium.org/184113007/diff/60001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/command/build.dart:66: var logJson = [];
Nit: I'd call these "jsonErrors" and "jsonLog", since it reads more like English
and less like Hungarian notation.

https://codereview.chromium.org/184113007/diff/60001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/command/build.dart:80: if (log.json.enabled) {
For other log levels, we don't add explicit log-level checks outside of
log.dart. It would probably read cleaner and be more consistent if we avoided
that for JSON as well.

https://codereview.chromium.org/184113007/diff/60001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/command/build.dart:322: assetIdToMap(AssetId id) {
You don't actually use this anywhere.

https://codereview.chromium.org/184113007/diff/60001/sdk/lib/_internal/pub/li...
File sdk/lib/_internal/pub/lib/src/log.dart (right):

https://codereview.chromium.org/184113007/diff/60001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/log.dart:214: _printToStream(stderr, entry,
showLabel: true);
If you're keeping _logToStderrWithLabel, I don't understand this change. If
you're getting rid of it, you should delete it below.

https://codereview.chromium.org/184113007/diff/60001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/log.dart:390: /// Whether logging should use
machine-friendl JSON output or human-friendly
"friendl" -> "friendly"

https://codereview.chromium.org/184113007/diff/60001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/log.dart:393: /// If set to `true`, then all
regular logging is not printed. Logged
"no regular logging is printed"

https://codereview.chromium.org/184113007/diff/60001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/log.dart:399: /// is enabled.
Mention that this prints to stdout as opposed to stderr.

https://codereview.chromium.org/184113007/diff/60001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/log.dart:400: void error(error) {
This should take an optional StackTrace as well.

https://codereview.chromium.org/184113007/diff/60001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/log.dart:406: errorJson["stackTrace"] = trace;
We should [new Chain.forTrace] this.

https://codereview.chromium.org/184113007/diff/60001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/log.dart:416: // TODO(rnystrom): Should errors be
printed to stderr?
I think stdout is fine. Since the errors have identifying metadata anyway,
printing some to stderr and some to stdout just complicates things.

https://codereview.chromium.org/184113007/diff/60001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/log.dart:424: final json = new _JsonLogger();
Nit: I'd expect this to be at the top of the file, like other library variables.

https://codereview.chromium.org/184113007/diff/60001/sdk/lib/_internal/pub/li...
File sdk/lib/_internal/pub/lib/src/utils.dart (right):

https://codereview.chromium.org/184113007/diff/60001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/utils.dart:867: /// A class for command input data
exceptions.
This description is pretty terse. I assume it refers to things like invalid
command-line arguments, but it would be nice to have some more description here.

[Bob Nystrom, 2014-03-07 00:42:17]

Thanks!

https://codereview.chromium.org/184113007/diff/60001/sdk/lib/_internal/pub/li...
File sdk/lib/_internal/pub/lib/src/command.dart (right):

https://codereview.chromium.org/184113007/diff/60001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/command.dart:199: /// Throw an
[ApplicationException] for a data error of this command with
On 2014/03/05 02:36:52, nweiz wrote:
> I don't understand what a "data error of this command" is or what it means to
> throw an ApplicationException for one.

Reworded.

> It also seems weird that this is a
> separate function rather than just having people write "throw new
> DataException(...)". I get that it's similar to [usageError], but that adds
> extra value where this doesn't.

Yeah, I did it for consistency with usageError(). I think it makes the command
validate code a little cleaner to have a function for this.

https://codereview.chromium.org/184113007/diff/60001/sdk/lib/_internal/pub/li...
File sdk/lib/_internal/pub/lib/src/command/build.dart (right):

https://codereview.chromium.org/184113007/diff/60001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/command/build.dart:66: var logJson = [];
On 2014/03/05 02:36:52, nweiz wrote:
> Nit: I'd call these "jsonErrors" and "jsonLog", since it reads more like
English
> and less like Hungarian notation.

I considered that but "jsonErrors" to me sounds like "errors with the json" and
not "errors that happen to be reported in json format".

https://codereview.chromium.org/184113007/diff/60001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/command/build.dart:80: if (log.json.enabled) {
On 2014/03/05 02:36:52, nweiz wrote:
> For other log levels, we don't add explicit log-level checks outside of
> log.dart. It would probably read cleaner and be more consistent if we avoided
> that for JSON as well.

I'm not a big fan of this either, but it seemed dumb to accumulate the JSON
output in memory even when JSON output isn't going to be used.

If the JSON output was a streamed series of objects instead of one single output
at the end, I could have done something cleaner, but this was the simplest
efficient solution I could come up with.

https://codereview.chromium.org/184113007/diff/60001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/command/build.dart:322: assetIdToMap(AssetId id) {
On 2014/03/05 02:36:52, nweiz wrote:
> You don't actually use this anywhere.

Done.

https://codereview.chromium.org/184113007/diff/60001/sdk/lib/_internal/pub/li...
File sdk/lib/_internal/pub/lib/src/log.dart (right):

https://codereview.chromium.org/184113007/diff/60001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/log.dart:214: _printToStream(stderr, entry,
showLabel: true);
On 2014/03/05 02:36:52, nweiz wrote:
> If you're keeping _logToStderrWithLabel, I don't understand this change. If
> you're getting rid of it, you should delete it below.

Calling _printToStream() directly skips the if (json.enabled) check in
_logToStream. If the transcript gets dumped on a failure, we want to always dump
it, even in JSON mode.

https://codereview.chromium.org/184113007/diff/60001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/log.dart:390: /// Whether logging should use
machine-friendl JSON output or human-friendly
On 2014/03/05 02:36:52, nweiz wrote:
> "friendl" -> "friendly"

Done.

https://codereview.chromium.org/184113007/diff/60001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/log.dart:393: /// If set to `true`, then all
regular logging is not printed. Logged
On 2014/03/05 02:36:52, nweiz wrote:
> "no regular logging is printed"

Done.

https://codereview.chromium.org/184113007/diff/60001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/log.dart:399: /// is enabled.
On 2014/03/05 02:36:52, nweiz wrote:
> Mention that this prints to stdout as opposed to stderr.

Done.

https://codereview.chromium.org/184113007/diff/60001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/log.dart:400: void error(error) {
On 2014/03/05 02:36:52, nweiz wrote:
> This should take an optional StackTrace as well.

Done.

https://codereview.chromium.org/184113007/diff/60001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/log.dart:406: errorJson["stackTrace"] = trace;
On 2014/03/05 02:36:52, nweiz wrote:
> We should [new Chain.forTrace] this.

Done.

https://codereview.chromium.org/184113007/diff/60001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/log.dart:416: // TODO(rnystrom): Should errors be
printed to stderr?
On 2014/03/05 02:36:52, nweiz wrote:
> I think stdout is fine. Since the errors have identifying metadata anyway,
> printing some to stderr and some to stdout just complicates things.

Done.

https://codereview.chromium.org/184113007/diff/60001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/log.dart:424: final json = new _JsonLogger();
On 2014/03/05 02:36:52, nweiz wrote:
> Nit: I'd expect this to be at the top of the file, like other library
variables.

Done.

https://codereview.chromium.org/184113007/diff/60001/sdk/lib/_internal/pub/li...
File sdk/lib/_internal/pub/lib/src/utils.dart (right):

https://codereview.chromium.org/184113007/diff/60001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/utils.dart:867: /// A class for command input data
exceptions.
On 2014/03/05 02:36:52, nweiz wrote:
> This description is pretty terse. I assume it refers to things like invalid
> command-line arguments, but it would be nice to have some more description
here.

Reword a bit. It's basically an exception form of exit_codes.DATA.

[nweiz, 2014-03-07 00:57:13]

lgtm

https://codereview.chromium.org/184113007/diff/60001/sdk/lib/_internal/pub/li...
File sdk/lib/_internal/pub/lib/src/utils.dart (right):

https://codereview.chromium.org/184113007/diff/60001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/utils.dart:867: /// A class for command input data
exceptions.
On 2014/03/07 00:42:18, Bob Nystrom wrote:
> On 2014/03/05 02:36:52, nweiz wrote:
> > This description is pretty terse. I assume it refers to things like invalid
> > command-line arguments, but it would be nice to have some more description
> here.
> 
> Reword a bit. It's basically an exception form of exit_codes.DATA.

I still don't really understand how this should be used differently than
FormatError.

[Bob Nystrom, 2014-03-07 20:36:25]

Committed patchset #6 manually as r33450 (presubmit successful).

[Bob Nystrom, 2014-03-07 20:38:42]

https://codereview.chromium.org/184113007/diff/60001/sdk/lib/_internal/pub/li...
File sdk/lib/_internal/pub/lib/src/utils.dart (right):

https://codereview.chromium.org/184113007/diff/60001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/utils.dart:867: /// A class for command input data
exceptions.
On 2014/03/07 00:57:13, nweiz wrote:
> On 2014/03/07 00:42:18, Bob Nystrom wrote:
> > On 2014/03/05 02:36:52, nweiz wrote:
> > > This description is pretty terse. I assume it refers to things like
invalid
> > > command-line arguments, but it would be nice to have some more description
> > here.
> > 
> > Reword a bit. It's basically an exception form of exit_codes.DATA.
> 
> I still don't really understand how this should be used differently than
> FormatError.

I look at FormatException as "there's something wrong with the data you gave me,
so I'm bailing". dataError() means "I want pub to exit with exit_code.DATA now."

The fact that it throws an exception is an implementation detail. (And, in fact,
if we didn't have that annoying flush-before-exit stuff, it would just call
exit() and not throw anything.)