Switch FE to use the libraries.json format.

Switch FE to use the libraries.json format.

This CL:
  * introduces the Dart API to operate over libraries specifications and describes
    the format we intend to use (see libraries_spec.dart)

  * implements serialization/deserialization for this API

  * switches over the front_end to use these APIs
    * public options accept a URI to the JSON file and no longer
       accept a `dartLibraries` map
    * internal code uses the LibrariesSpecification API

  * switches other dependencies on these APIs (resynthesizer_test and patch_sdk.dart)

This is the first step in migrating over to use the libraries.json format and
eventually remove the patched_sdk step. In particular, some of the next steps
include:
  * add a build step to generate .json files from .yaml files
  * add a libraries.yaml file for the sdk
  * split the patched_sdk step in two:
     * patching files
     * generating .dill files
  * add any missing support for patch-files in fasta
  * finally remove the patching files step, and only have a build step for generating
   .dill files

BUG=
R=ahe@google.com, paulberry@google.com, scheglov@google.com

Committed: https://github.com/dart-lang/sdk/commit/abf2d23af2315fae6dc688741147ef677ec34835
Committed: https://github.com/dart-lang/sdk/commit/b48584d3d0eb480943d72958104baf8e0e366074

[Siggi Cherem (dart-lang), 2017-08-03 02:55:40]

Description was changed from

==========
Switch FE to use the libraries.json format.

This CL:
  * describes in detail what the format looks like (see
    libraries_spec.dart)

  * introduces the Dart API to operate over libraries specifications (see
    libraries_spec.dart)

  * implements serialization/deserialization for this API

  * switches over the front_end to use these APIs
    * public options accept a URI to the JSON file
    * internal code uses the LibrariesSpecification API

This is the first step in migrating over to use the libraries.json format and
eventually remove the patched_sdk step. In particular, some of the next steps
include:
  * add a build step to generate .json files from .yaml files
  * add a libraries.yaml file for the sdk
  * split the patched_sdk step in two:
     * patching files
     * generating .dill files
  * add any missing support for patch-files in fasta
  * remove the patching files step, and only have a build step for generating
   .dill files

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

to

==========
Switch FE to use the libraries.json format.

This CL:
  * describes in detail what the format looks like (see
    libraries_spec.dart)

  * introduces the Dart API to operate over libraries specifications (see
    libraries_spec.dart)

  * implements serialization/deserialization for this API

  * switches over the front_end to use these APIs
    * public options accept a URI to the JSON file
    * internal code uses the LibrariesSpecification API

  * switches other dependencies on these APIs (resynthesizer_test and
patch_sdk.dart)

This is the first step in migrating over to use the libraries.json format and
eventually remove the patched_sdk step. In particular, some of the next steps
include:
  * add a build step to generate .json files from .yaml files
  * add a libraries.yaml file for the sdk
  * split the patched_sdk step in two:
     * patching files
     * generating .dill files
  * add any missing support for patch-files in fasta
  * remove the patching files step, and only have a build step for generating
   .dill files

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

[Siggi Cherem (dart-lang), 2017-08-03 02:57:36]

Patchset #2 (id:20001) has been deleted

[Siggi Cherem (dart-lang), 2017-08-03 02:57:42]

Patchset #1 (id:1) has been deleted

[Siggi Cherem (dart-lang), 2017-08-03 03:01:08]

Description was changed from

==========
Switch FE to use the libraries.json format.

This CL:
  * describes in detail what the format looks like (see
    libraries_spec.dart)

  * introduces the Dart API to operate over libraries specifications (see
    libraries_spec.dart)

  * implements serialization/deserialization for this API

  * switches over the front_end to use these APIs
    * public options accept a URI to the JSON file
    * internal code uses the LibrariesSpecification API

  * switches other dependencies on these APIs (resynthesizer_test and
patch_sdk.dart)

This is the first step in migrating over to use the libraries.json format and
eventually remove the patched_sdk step. In particular, some of the next steps
include:
  * add a build step to generate .json files from .yaml files
  * add a libraries.yaml file for the sdk
  * split the patched_sdk step in two:
     * patching files
     * generating .dill files
  * add any missing support for patch-files in fasta
  * remove the patching files step, and only have a build step for generating
   .dill files

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

to

==========
Switch FE to use the libraries.json format.

This CL:
  * introduces the Dart API to operate over libraries specifications and
describes
    the format we intend to use (see libraries_spec.dart)

  * implements serialization/deserialization for this API

  * switches over the front_end to use these APIs
    * public options accept a URI to the JSON file and no longer
       accept a `dartLibraries` map
    * internal code uses the LibrariesSpecification API

  * switches other dependencies on these APIs (resynthesizer_test and
patch_sdk.dart)

This is the first step in migrating over to use the libraries.json format and
eventually remove the patched_sdk step. In particular, some of the next steps
include:
  * add a build step to generate .json files from .yaml files
  * add a libraries.yaml file for the sdk
  * split the patched_sdk step in two:
     * patching files
     * generating .dill files
  * add any missing support for patch-files in fasta
  * finally remove the patching files step, and only have a build step for
generating
   .dill files

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

[Siggi Cherem (dart-lang), 2017-08-03 03:01:39]

sigmund@google.com changed reviewers:
+ ahe@google.com, paulberry@google.com, scheglov@google.com

[Siggi Cherem (dart-lang), 2017-08-03 03:01:39]

https://codereview.chromium.org/2986303003/diff/40001/pkg/front_end/test/src/...
File pkg/front_end/test/src/base/processed_options_test.dart (right):

https://codereview.chromium.org/2986303003/diff/40001/pkg/front_end/test/src/...
pkg/front_end/test/src/base/processed_options_test.dart:21:
CompilerContext.runWithDefaultOptions((_) {
FYI - this line is not needed for this CL, but helpful because otherwise a test
failure shows an error message about a missing context (that is caused when
trying to report the real error within the compiler).

I plan to revisit all of our tests to ensure we have a default context defined
in all of them and don't have this kind of mysterious messages.

[ahe, 2017-08-03 11:58:16]

Despite all the non-file file URIs, LGTM :-D

https://codereview.chromium.org/2986303003/diff/40001/pkg/analyzer/test/src/s...
File pkg/analyzer/test/src/summary/resynthesize_kernel_test.dart (right):

https://codereview.chromium.org/2986303003/diff/40001/pkg/analyzer/test/src/s...
pkg/analyzer/test/src/summary/resynthesize_kernel_test.dart:82: new
LibraryInfo(name, Uri.parse('file://$path'), const []);
This isn't a real file URI. Please don't use file URIs for things that aren't
files.

https://codereview.chromium.org/2986303003/diff/40001/pkg/front_end/lib/src/b...
File pkg/front_end/lib/src/base/libraries_spec.dart (right):

https://codereview.chromium.org/2986303003/diff/40001/pkg/front_end/lib/src/b...
pkg/front_end/lib/src/base/libraries_spec.dart:1: // Copyright (c) 2017, the
Dart project authors.  Please see the AUTHORS file
Consider renaming this file to libraries_specification.dart.

https://codereview.chromium.org/2986303003/diff/40001/pkg/front_end/lib/src/b...
pkg/front_end/lib/src/base/libraries_spec.dart:14: /// readability purposes):
I don't think using YAML is helpful if the format is JSON. It increases the
mental overhead and assumes familiarity with YAML and JSON, and how they map to
each other.

https://codereview.chromium.org/2986303003/diff/40001/pkg/front_end/lib/src/b...
pkg/front_end/lib/src/base/libraries_spec.dart:21: ///           # is relative
to this specification file
You're saying that `path` is a URI (that's relative). But below, you're
interpreting it as a file path.

https://codereview.chromium.org/2986303003/diff/40001/pkg/front_end/lib/src/b...
pkg/front_end/lib/src/base/libraries_spec.dart:60: /// The
[TargetLibrariesSpecification] for [targetName].
Isn't this kinda obvious?

https://codereview.chromium.org/2986303003/diff/40001/pkg/front_end/lib/src/b...
pkg/front_end/lib/src/base/libraries_spec.dart:66: static LibrariesSpecification
parse(Uri fileUri, String json) {
Consider renaming fileUri to base or baseUri.

https://codereview.chromium.org/2986303003/diff/40001/pkg/front_end/lib/src/b...
pkg/front_end/lib/src/base/libraries_spec.dart:68: Map jsonData =
JSON.decode(json);
May throw a format exception.

https://codereview.chromium.org/2986303003/diff/40001/pkg/front_end/lib/src/b...
pkg/front_end/lib/src/base/libraries_spec.dart:68: Map jsonData =
JSON.decode(json);
jsonData may not be a map.

https://codereview.chromium.org/2986303003/diff/40001/pkg/front_end/lib/src/b...
pkg/front_end/lib/src/base/libraries_spec.dart:70: jsonData.forEach((String
targetName, targetData) {
targetData here.

https://codereview.chromium.org/2986303003/diff/40001/pkg/front_end/lib/src/b...
pkg/front_end/lib/src/base/libraries_spec.dart:72: Map targetData =
jsonData[targetName];
And here.

https://codereview.chromium.org/2986303003/diff/40001/pkg/front_end/lib/src/b...
pkg/front_end/lib/src/base/libraries_spec.dart:72: Map targetData =
jsonData[targetName];
targetData may not be a map.

https://codereview.chromium.org/2986303003/diff/40001/pkg/front_end/lib/src/b...
pkg/front_end/lib/src/base/libraries_spec.dart:73: if (targetData != null) {
Consider:

if (targetData is Map) {
...
} else {
  report error.
}

https://codereview.chromium.org/2986303003/diff/40001/pkg/front_end/lib/src/b...
pkg/front_end/lib/src/base/libraries_spec.dart:74: Map librariesData =
targetData["libraries"];
librariesData may not be a map.

https://codereview.chromium.org/2986303003/diff/40001/pkg/front_end/lib/src/b...
pkg/front_end/lib/src/base/libraries_spec.dart:75: Uri resolve(String path) =>
fileUri.resolveUri(new Uri.file(path));
Why is it a filename, and not a URI? I'm pretty sure that this will catch
everyone by surprise on Windows.

https://codereview.chromium.org/2986303003/diff/40001/pkg/front_end/lib/src/b...
pkg/front_end/lib/src/base/libraries_spec.dart:101: 'path': pathFor(lib.uri),
This is written as a URI, not a file name.

https://codereview.chromium.org/2986303003/diff/40001/pkg/front_end/lib/src/b...
File pkg/front_end/lib/src/base/processed_options.dart (right):

https://codereview.chromium.org/2986303003/diff/40001/pkg/front_end/lib/src/b...
pkg/front_end/lib/src/base/processed_options.dart:292: ticker.logMs("Read
libraries file");
You need to make sure you know precisely when ticker.logMs is called. It doesn't
work when stuff is initialized by demand. There's an arbitrary amount of things
happening before reading the library specification.

I suggest that you compute the TargetSpecification explicitly before creating
the ProcessedOptions object.

Ideally, the flow of the code should be:

ticker.logMs("The previous phase").
TargetLibrariesSpecification specification = await
computeLibrarySpecification();
ticker.logMs("Read libraries specification");
ProcessedOptions options = ...;
ticker.logMs("Processed options");
// Invariant: all computations in processed options are done now.

https://codereview.chromium.org/2986303003/diff/40001/pkg/front_end/lib/src/b...
pkg/front_end/lib/src/base/processed_options.dart:302: // TODO(sigmund): eek! we
should get to the point where there is no
Eek (uppercase).
We (uppercase).

https://codereview.chromium.org/2986303003/diff/40001/pkg/front_end/lib/src/b...
pkg/front_end/lib/src/base/processed_options.dart:306: if (librariesSpecUri ==
null ||
This field should be named librariesSpecificationUri.

https://codereview.chromium.org/2986303003/diff/40001/pkg/front_end/messages....
File pkg/front_end/messages.yaml (right):

https://codereview.chromium.org/2986303003/diff/40001/pkg/front_end/messages....
pkg/front_end/messages.yaml:827: template: "SDK libraries specification not
found: #uri."
Consider adding something like this:

tip: "Normally, the specification is a file named 'libraries.json' in the Dart
SDK install location.

https://codereview.chromium.org/2986303003/diff/40001/pkg/front_end/test/fast...
File pkg/front_end/test/fasta/uri_translator_test.dart (right):

https://codereview.chromium.org/2986303003/diff/40001/pkg/front_end/test/fast...
pkg/front_end/test/fasta/uri_translator_test.dart:22: 'core',
Uri.parse('file:///sdk/core/core.dart'), const []),
These aren't names of real files. Please don't use file URIs for that.

https://codereview.chromium.org/2986303003/diff/40001/pkg/front_end/test/incr...
File pkg/front_end/test/incremental_kernel_generator_test.dart (right):

https://codereview.chromium.org/2986303003/diff/40001/pkg/front_end/test/incr...
pkg/front_end/test/incremental_kernel_generator_test.dart:48: ..librariesSpecUri
= Uri.parse('file:///sdk/lib/libraries.json');
Ditto.

https://codereview.chromium.org/2986303003/diff/40001/pkg/front_end/test/src/...
File pkg/front_end/test/src/base/libraries_spec_test.dart (right):

https://codereview.chromium.org/2986303003/diff/40001/pkg/front_end/test/src/...
pkg/front_end/test/src/base/libraries_spec_test.dart:15:
Uri.parse('org-dartlang-custom://one/two/f.json'), jsonString);
YAY!!! \o/ :-D

Use three slashes so "one" isn't interpreted as a host name.

https://codereview.chromium.org/2986303003/diff/40001/pkg/front_end/test/src/...
File pkg/front_end/test/src/incremental/mock_sdk.dart (right):

https://codereview.chromium.org/2986303003/diff/40001/pkg/front_end/test/src/...
pkg/front_end/test/src/incremental/mock_sdk.dart:274: Uri uri =
Uri.parse('file:///sdk/lib/libraries.json');
:-(

[Paul Berry, 2017-08-03 17:50:41]

lgtm

https://codereview.chromium.org/2986303003/diff/40001/pkg/front_end/lib/src/b...
File pkg/front_end/lib/src/base/libraries_spec.dart (right):

https://codereview.chromium.org/2986303003/diff/40001/pkg/front_end/lib/src/b...
pkg/front_end/lib/src/base/libraries_spec.dart:14: /// readability purposes):
On 2017/08/03 11:58:15, ahe wrote:
> I don't think using YAML is helpful if the format is JSON. It increases the
> mental overhead and assumes familiarity with YAML and JSON, and how they map
to
> each other.

I think I agree with this...it makes my brain hurt a bit to read this
pseudo-YAML :)

https://codereview.chromium.org/2986303003/diff/40001/pkg/front_end/lib/src/b...
pkg/front_end/lib/src/base/libraries_spec.dart:21: ///           # is relative
to this specification file
On 2017/08/03 11:58:15, ahe wrote:
> You're saying that `path` is a URI (that's relative). But below, you're
> interpreting it as a file path.

Yeah, since paths use different separators on Windows, let's be explicit here. 
I assume that this "path" should always use "/" (even on Windows) so that we
don't have to distribute different JSON files for use on Windows.

Also, it's not clear to me whether this path is *required* to be relative, or
whether it's allowed to be absolute.  If it's required to be relative, is it
allowed to contain ".."?  If it's allowed to be absolute, is it allowed to
declare a scheme?  The answers to these questions might have security
implications (because if we consider the contents of the JSON to be "untrusted
input" it could potentially be used to redirect us to open any file) so I think
we should start off conservative: require it to be a posix-style relative path
with no ".."s.

https://codereview.chromium.org/2986303003/diff/40001/pkg/front_end/lib/src/b...
pkg/front_end/lib/src/base/libraries_spec.dart:68: Map jsonData =
JSON.decode(json);
On 2017/08/03 11:58:15, ahe wrote:
> May throw a format exception.

I think it would be ok to address this possibility by declaring in the comment
above line 66 that if [json] is improperly formatted, an exception will be
raised.  (IMHO it's not necessary to catch the exception from JSON.decode and
wrap it)

https://codereview.chromium.org/2986303003/diff/40001/pkg/front_end/lib/src/b...
pkg/front_end/lib/src/base/libraries_spec.dart:68: Map jsonData =
JSON.decode(json);
On 2017/08/03 11:58:15, ahe wrote:
> jsonData may not be a map.

Agreed, and given that we're still straddling the line between strong and
non-strong semantics, this code might be run in non-strong mode with checked
assertions off, so the problem would not be caught until line 70, with a
potentially confusing exception message.

IMHO it wuld be sufficient to do:

var jsonData = JSON.decode(json) as Map;

https://codereview.chromium.org/2986303003/diff/40001/pkg/front_end/lib/src/b...
pkg/front_end/lib/src/base/libraries_spec.dart:75: Uri resolve(String path) =>
fileUri.resolveUri(new Uri.file(path));
On 2017/08/03 11:58:15, ahe wrote:
> Why is it a filename, and not a URI? I'm pretty sure that this will catch
> everyone by surprise on Windows.

Agreed.  And considering the security concern I brought up earlier, I think we
should be really conservative in what we accept.

https://codereview.chromium.org/2986303003/diff/40001/pkg/front_end/lib/src/b...
File pkg/front_end/lib/src/base/processed_options.dart (right):

https://codereview.chromium.org/2986303003/diff/40001/pkg/front_end/lib/src/b...
pkg/front_end/lib/src/base/processed_options.dart:292: ticker.logMs("Read
libraries file");
On 2017/08/03 11:58:16, ahe wrote:
> You need to make sure you know precisely when ticker.logMs is called. It
doesn't
> work when stuff is initialized by demand. There's an arbitrary amount of
things
> happening before reading the library specification.
> 
> I suggest that you compute the TargetSpecification explicitly before creating
> the ProcessedOptions object.
> 
> Ideally, the flow of the code should be:
> 
> ticker.logMs("The previous phase").
> TargetLibrariesSpecification specification = await
> computeLibrarySpecification();
> ticker.logMs("Read libraries specification");
> ProcessedOptions options = ...;
> ticker.logMs("Processed options");
> // Invariant: all computations in processed options are done now.

It seems like you're suggesting a flow where the client does some processing
(based on options) before creating the ProcessedOptions object.  I really want
to avoid that because the whole idea of ProcessedOptions is to make sure that
all the APIs in the front end deal with options in a uniform way; requiring
computation to be done before creating ProcessedOptions creates the danger of
drift between what the different implementations do before creating
ProcessedOptions.

Personally I don't have as big a concern about initializing stuff on demand.  As
long as we call ticker.logMs before and after doing things, we can safely do
things on demand because the order of operations will be clear in the log.

So I think all we need to do here is add the following line just before line
291:

ticker.logMs("Compute library specification");

[scheglov, 2017-08-03 18:14:14]

lgtm

[Siggi Cherem (dart-lang), 2017-08-05 00:40:47]

Patchset #2 (id:60001) has been deleted

[Siggi Cherem (dart-lang), 2017-08-05 00:41:02]

Thanks everyone!

https://codereview.chromium.org/2986303003/diff/40001/pkg/analyzer/test/src/s...
File pkg/analyzer/test/src/summary/resynthesize_kernel_test.dart (right):

https://codereview.chromium.org/2986303003/diff/40001/pkg/analyzer/test/src/s...
pkg/analyzer/test/src/summary/resynthesize_kernel_test.dart:82: new
LibraryInfo(name, Uri.parse('file://$path'), const []);
On 2017/08/03 11:58:15, ahe wrote:
> This isn't a real file URI. Please don't use file URIs for things that aren't
> files.

Note: we are tracking doing so here:
https://github.com/dart-lang/sdk/issues/30141

I would like to address the URI issue as separate CLs, that's why in general, I
am trying to avoid fixing them when I do edits like this one.  When adding new
tests, I'll try my best to use custom uris.

https://codereview.chromium.org/2986303003/diff/40001/pkg/front_end/lib/src/b...
File pkg/front_end/lib/src/base/libraries_spec.dart (right):

https://codereview.chromium.org/2986303003/diff/40001/pkg/front_end/lib/src/b...
pkg/front_end/lib/src/base/libraries_spec.dart:1: // Copyright (c) 2017, the
Dart project authors.  Please see the AUTHORS file
On 2017/08/03 11:58:15, ahe wrote:
> Consider renaming this file to libraries_specification.dart.

Done.

https://codereview.chromium.org/2986303003/diff/40001/pkg/front_end/lib/src/b...
pkg/front_end/lib/src/base/libraries_spec.dart:14: /// readability purposes):
On 2017/08/03 17:50:41, Paul Berry wrote:
> On 2017/08/03 11:58:15, ahe wrote:
> > I don't think using YAML is helpful if the format is JSON. It increases the
> > mental overhead and assumes familiarity with YAML and JSON, and how they map
> to
> > each other.
> 
> I think I agree with this...it makes my brain hurt a bit to read this
> pseudo-YAML :)

Done :)

https://codereview.chromium.org/2986303003/diff/40001/pkg/front_end/lib/src/b...
pkg/front_end/lib/src/base/libraries_spec.dart:21: ///           # is relative
to this specification file
On 2017/08/03 17:50:41, Paul Berry wrote:
> On 2017/08/03 11:58:15, ahe wrote:
> > You're saying that `path` is a URI (that's relative). But below, you're
> > interpreting it as a file path.
> 
> Yeah, since paths use different separators on Windows, let's be explicit here.

> I assume that this "path" should always use "/" (even on Windows) so that we
> don't have to distribute different JSON files for use on Windows.
> 
> Also, it's not clear to me whether this path is *required* to be relative, or
> whether it's allowed to be absolute.  If it's required to be relative, is it
> allowed to contain ".."?  If it's allowed to be absolute, is it allowed to
> declare a scheme?  The answers to these questions might have security
> implications (because if we consider the contents of the JSON to be "untrusted
> input" it could potentially be used to redirect us to open any file) so I
think
> we should start off conservative: require it to be a posix-style relative path
> with no ".."s.

Ok - I decided to match what we do here with what we do for .packages files. The
package-config specifies that the values should be uri references, so I updated
the text and logic below to do the same here.

Note: I also believe we need to support "../" because of how the sdk libraries
and patch files are laid out in the repo today (some of which are under sdk/lib/
some under runtime/...). That's also supported for .packages files, so accepting
this would also keep it consistent.

https://codereview.chromium.org/2986303003/diff/40001/pkg/front_end/lib/src/b...
pkg/front_end/lib/src/base/libraries_spec.dart:60: /// The
[TargetLibrariesSpecification] for [targetName].
On 2017/08/03 11:58:15, ahe wrote:
> Isn't this kinda obvious?

Rephrased a bit to indicate what happens if target is not specified in the file.

https://codereview.chromium.org/2986303003/diff/40001/pkg/front_end/lib/src/b...
pkg/front_end/lib/src/base/libraries_spec.dart:66: static LibrariesSpecification
parse(Uri fileUri, String json) {
On 2017/08/03 11:58:15, ahe wrote:
> Consider renaming fileUri to base or baseUri.

Done.

https://codereview.chromium.org/2986303003/diff/40001/pkg/front_end/lib/src/b...
pkg/front_end/lib/src/base/libraries_spec.dart:68: Map jsonData =
JSON.decode(json);
On 2017/08/03 17:50:41, Paul Berry wrote:
> On 2017/08/03 11:58:15, ahe wrote:
> > May throw a format exception.
> 
> I think it would be ok to address this possibility by declaring in the comment
> above line 66 that if [json] is improperly formatted, an exception will be
> raised.  (IMHO it's not necessary to catch the exception from JSON.decode and
> wrap it)

Done. I added more general error handling here and below, with some unit tests.

Paul - you might have not seen that discussion about .packages on an earlier CL,
here is more context: https://github.com/dart-lang/package_config/issues/40.
Given that, I decided to wrap the FormatException as well.

https://codereview.chromium.org/2986303003/diff/40001/pkg/front_end/lib/src/b...
pkg/front_end/lib/src/base/libraries_spec.dart:70: jsonData.forEach((String
targetName, targetData) {
On 2017/08/03 11:58:15, ahe wrote:
> targetData here.
Done

https://codereview.chromium.org/2986303003/diff/40001/pkg/front_end/lib/src/b...
pkg/front_end/lib/src/base/libraries_spec.dart:72: Map targetData =
jsonData[targetName];
On 2017/08/03 11:58:15, ahe wrote:
> And here.

Done.

https://codereview.chromium.org/2986303003/diff/40001/pkg/front_end/lib/src/b...
pkg/front_end/lib/src/base/libraries_spec.dart:73: if (targetData != null) {
On 2017/08/03 11:58:15, ahe wrote:
> Consider:
> 
> if (targetData is Map) {
> ...
> } else {
>   report error.
> }

Done.

https://codereview.chromium.org/2986303003/diff/40001/pkg/front_end/lib/src/b...
pkg/front_end/lib/src/base/libraries_spec.dart:74: Map librariesData =
targetData["libraries"];
On 2017/08/03 11:58:15, ahe wrote:
> librariesData may not be a map.

Done.

https://codereview.chromium.org/2986303003/diff/40001/pkg/front_end/lib/src/b...
pkg/front_end/lib/src/base/libraries_spec.dart:75: Uri resolve(String path) =>
fileUri.resolveUri(new Uri.file(path));
On 2017/08/03 17:50:41, Paul Berry wrote:
> On 2017/08/03 11:58:15, ahe wrote:
> > Why is it a filename, and not a URI? I'm pretty sure that this will catch
> > everyone by surprise on Windows.
> 
> Agreed.  And considering the security concern I brought up earlier, I think we
> should be really conservative in what we accept.

Done. this is now consistently handled as a Uri reference.

https://codereview.chromium.org/2986303003/diff/40001/pkg/front_end/lib/src/b...
pkg/front_end/lib/src/base/libraries_spec.dart:101: 'path': pathFor(lib.uri),
On 2017/08/03 11:58:15, ahe wrote:
> This is written as a URI, not a file name.

Done.

https://codereview.chromium.org/2986303003/diff/40001/pkg/front_end/lib/src/b...
File pkg/front_end/lib/src/base/processed_options.dart (right):

https://codereview.chromium.org/2986303003/diff/40001/pkg/front_end/lib/src/b...
pkg/front_end/lib/src/base/processed_options.dart:292: ticker.logMs("Read
libraries file");
On 2017/08/03 17:50:41, Paul Berry wrote:
> On 2017/08/03 11:58:16, ahe wrote:
> > You need to make sure you know precisely when ticker.logMs is called. It
> doesn't
> > work when stuff is initialized by demand. There's an arbitrary amount of
> things
> > happening before reading the library specification.
> > 
> > I suggest that you compute the TargetSpecification explicitly before
creating
> > the ProcessedOptions object.
> > 
> > Ideally, the flow of the code should be:
> > 
> > ticker.logMs("The previous phase").
> > TargetLibrariesSpecification specification = await
> > computeLibrarySpecification();
> > ticker.logMs("Read libraries specification");
> > ProcessedOptions options = ...;
> > ticker.logMs("Processed options");
> > // Invariant: all computations in processed options are done now.
> 
> It seems like you're suggesting a flow where the client does some processing
> (based on options) before creating the ProcessedOptions object.  I really want
> to avoid that because the whole idea of ProcessedOptions is to make sure that
> all the APIs in the front end deal with options in a uniform way; requiring
> computation to be done before creating ProcessedOptions creates the danger of
> drift between what the different implementations do before creating
> ProcessedOptions.
> 
> Personally I don't have as big a concern about initializing stuff on demand. 
As
> long as we call ticker.logMs before and after doing things, we can safely do
> things on demand because the order of operations will be clear in the log.
> 
> So I think all we need to do here is add the following line just before line
> 291:
> 
> ticker.logMs("Compute library specification");

Done. Added the extra tick as Paul suggested.

https://codereview.chromium.org/2986303003/diff/40001/pkg/front_end/lib/src/b...
pkg/front_end/lib/src/base/processed_options.dart:302: // TODO(sigmund): eek! we
should get to the point where there is no
On 2017/08/03 11:58:16, ahe wrote:
> Eek (uppercase).
> We (uppercase).

EEK! OK :)

https://codereview.chromium.org/2986303003/diff/40001/pkg/front_end/lib/src/b...
pkg/front_end/lib/src/base/processed_options.dart:306: if (librariesSpecUri ==
null ||
On 2017/08/03 11:58:15, ahe wrote:
> This field should be named librariesSpecificationUri.

Done.

https://codereview.chromium.org/2986303003/diff/40001/pkg/front_end/messages....
File pkg/front_end/messages.yaml (right):

https://codereview.chromium.org/2986303003/diff/40001/pkg/front_end/messages....
pkg/front_end/messages.yaml:827: template: "SDK libraries specification not
found: #uri."
On 2017/08/03 11:58:16, ahe wrote:
> Consider adding something like this:
> 
> tip: "Normally, the specification is a file named 'libraries.json' in the Dart
> SDK install location.

Done.

https://codereview.chromium.org/2986303003/diff/40001/pkg/front_end/test/fast...
File pkg/front_end/test/fasta/uri_translator_test.dart (right):

https://codereview.chromium.org/2986303003/diff/40001/pkg/front_end/test/fast...
pkg/front_end/test/fasta/uri_translator_test.dart:22: 'core',
Uri.parse('file:///sdk/core/core.dart'), const []),
On 2017/08/03 11:58:16, ahe wrote:
> These aren't names of real files. Please don't use file URIs for that.

Ack. See other comment, I left this as it was in the original test.

https://codereview.chromium.org/2986303003/diff/40001/pkg/front_end/test/incr...
File pkg/front_end/test/incremental_kernel_generator_test.dart (right):

https://codereview.chromium.org/2986303003/diff/40001/pkg/front_end/test/incr...
pkg/front_end/test/incremental_kernel_generator_test.dart:48: ..librariesSpecUri
= Uri.parse('file:///sdk/lib/libraries.json');
On 2017/08/03 11:58:16, ahe wrote:
> Ditto.

Similarly, this tests relies on the file://* scheme already

https://codereview.chromium.org/2986303003/diff/40001/pkg/front_end/test/src/...
File pkg/front_end/test/src/base/libraries_spec_test.dart (right):

https://codereview.chromium.org/2986303003/diff/40001/pkg/front_end/test/src/...
pkg/front_end/test/src/base/libraries_spec_test.dart:15:
Uri.parse('org-dartlang-custom://one/two/f.json'), jsonString);
On 2017/08/03 11:58:16, ahe wrote:
> YAY!!! \o/ :-D
> 
> Use three slashes so "one" isn't interpreted as a host name.

Done.

https://codereview.chromium.org/2986303003/diff/40001/pkg/front_end/test/src/...
File pkg/front_end/test/src/incremental/mock_sdk.dart (right):

https://codereview.chromium.org/2986303003/diff/40001/pkg/front_end/test/src/...
pkg/front_end/test/src/incremental/mock_sdk.dart:274: Uri uri =
Uri.parse('file:///sdk/lib/libraries.json');
On 2017/08/03 11:58:16, ahe wrote:
> :-(

ditto

[Siggi Cherem (dart-lang), 2017-08-05 00:41:34]

Description was changed from

==========
Switch FE to use the libraries.json format.

This CL:
  * introduces the Dart API to operate over libraries specifications and
describes
    the format we intend to use (see libraries_spec.dart)

  * implements serialization/deserialization for this API

  * switches over the front_end to use these APIs
    * public options accept a URI to the JSON file and no longer
       accept a `dartLibraries` map
    * internal code uses the LibrariesSpecification API

  * switches other dependencies on these APIs (resynthesizer_test and
patch_sdk.dart)

This is the first step in migrating over to use the libraries.json format and
eventually remove the patched_sdk step. In particular, some of the next steps
include:
  * add a build step to generate .json files from .yaml files
  * add a libraries.yaml file for the sdk
  * split the patched_sdk step in two:
     * patching files
     * generating .dill files
  * add any missing support for patch-files in fasta
  * finally remove the patching files step, and only have a build step for
generating
   .dill files

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

to

==========
Switch FE to use the libraries.json format.

This CL:
  * introduces the Dart API to operate over libraries specifications and
describes
    the format we intend to use (see libraries_spec.dart)

  * implements serialization/deserialization for this API

  * switches over the front_end to use these APIs
    * public options accept a URI to the JSON file and no longer
       accept a `dartLibraries` map
    * internal code uses the LibrariesSpecification API

  * switches other dependencies on these APIs (resynthesizer_test and
patch_sdk.dart)

This is the first step in migrating over to use the libraries.json format and
eventually remove the patched_sdk step. In particular, some of the next steps
include:
  * add a build step to generate .json files from .yaml files
  * add a libraries.yaml file for the sdk
  * split the patched_sdk step in two:
     * patching files
     * generating .dill files
  * add any missing support for patch-files in fasta
  * finally remove the patching files step, and only have a build step for
generating
   .dill files

BUG=
R=ahe@google.com, paulberry@google.com, scheglov@google.com

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

[Siggi Cherem (dart-lang), 2017-08-05 00:41:36]

Committed patchset #2 (id:80001) manually as
abf2d23af2315fae6dc688741147ef677ec34835 (presubmit successful).

[Siggi Cherem (dart-lang), 2017-08-05 03:06:15]

I had to revert, but just uploaded a new patchset with the fixes for the issues:
 - dart2js was providing compileSdk to workaround a bug we had in the validation
logic, now that is fixed and the flag was producing errors (as it should have
been)
 - patched_sdk.dart was generating an invalid .json (we used paths and not URIs,
so on windows this was causing trouble.

The changes were small, but I'll probably re-land it monday morning so I can
keep an eye on it.

[Siggi Cherem (dart-lang), 2017-08-07 15:41:34]

Description was changed from

==========
Switch FE to use the libraries.json format.

This CL:
  * introduces the Dart API to operate over libraries specifications and
describes
    the format we intend to use (see libraries_spec.dart)

  * implements serialization/deserialization for this API

  * switches over the front_end to use these APIs
    * public options accept a URI to the JSON file and no longer
       accept a `dartLibraries` map
    * internal code uses the LibrariesSpecification API

  * switches other dependencies on these APIs (resynthesizer_test and
patch_sdk.dart)

This is the first step in migrating over to use the libraries.json format and
eventually remove the patched_sdk step. In particular, some of the next steps
include:
  * add a build step to generate .json files from .yaml files
  * add a libraries.yaml file for the sdk
  * split the patched_sdk step in two:
     * patching files
     * generating .dill files
  * add any missing support for patch-files in fasta
  * finally remove the patching files step, and only have a build step for
generating
   .dill files

BUG=
R=ahe@google.com, paulberry@google.com, scheglov@google.com

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

to

==========
Switch FE to use the libraries.json format.

This CL:
  * introduces the Dart API to operate over libraries specifications and
describes
    the format we intend to use (see libraries_spec.dart)

  * implements serialization/deserialization for this API

  * switches over the front_end to use these APIs
    * public options accept a URI to the JSON file and no longer
       accept a `dartLibraries` map
    * internal code uses the LibrariesSpecification API

  * switches other dependencies on these APIs (resynthesizer_test and
patch_sdk.dart)

This is the first step in migrating over to use the libraries.json format and
eventually remove the patched_sdk step. In particular, some of the next steps
include:
  * add a build step to generate .json files from .yaml files
  * add a libraries.yaml file for the sdk
  * split the patched_sdk step in two:
     * patching files
     * generating .dill files
  * add any missing support for patch-files in fasta
  * finally remove the patching files step, and only have a build step for
generating
   .dill files

BUG=
R=ahe@google.com, paulberry@google.com, scheglov@google.com

Committed:
https://github.com/dart-lang/sdk/commit/abf2d23af2315fae6dc688741147ef677ec34835
Committed:
https://github.com/dart-lang/sdk/commit/b48584d3d0eb480943d72958104baf8e0e366074
==========

[Siggi Cherem (dart-lang), 2017-08-07 15:41:35]

Committed patchset #3 (id:100001) manually as
b48584d3d0eb480943d72958104baf8e0e366074 (presubmit successful).

[ahe, 2017-08-07 22:12:04]

https://codereview.chromium.org/2986303003/diff/40001/pkg/front_end/lib/src/b...
File pkg/front_end/lib/src/base/processed_options.dart (right):

https://codereview.chromium.org/2986303003/diff/40001/pkg/front_end/lib/src/b...
pkg/front_end/lib/src/base/processed_options.dart:292: ticker.logMs("Read
libraries file");
On 2017/08/05 00:41:02, Siggi Cherem (dart-lang) wrote:
> On 2017/08/03 17:50:41, Paul Berry wrote:
> > On 2017/08/03 11:58:16, ahe wrote:
> > > You need to make sure you know precisely when ticker.logMs is called. It
> > doesn't
> > > work when stuff is initialized by demand. There's an arbitrary amount of
> > things
> > > happening before reading the library specification.
> > > 
> > > I suggest that you compute the TargetSpecification explicitly before
> creating
> > > the ProcessedOptions object.
> > > 
> > > Ideally, the flow of the code should be:
> > > 
> > > ticker.logMs("The previous phase").
> > > TargetLibrariesSpecification specification = await
> > > computeLibrarySpecification();
> > > ticker.logMs("Read libraries specification");
> > > ProcessedOptions options = ...;
> > > ticker.logMs("Processed options");
> > > // Invariant: all computations in processed options are done now.
> > 
> > It seems like you're suggesting a flow where the client does some processing
> > (based on options) before creating the ProcessedOptions object.  I really
want
> > to avoid that because the whole idea of ProcessedOptions is to make sure
that
> > all the APIs in the front end deal with options in a uniform way; requiring
> > computation to be done before creating ProcessedOptions creates the danger
of
> > drift between what the different implementations do before creating
> > ProcessedOptions.
> > 
> > Personally I don't have as big a concern about initializing stuff on demand.

> As
> > long as we call ticker.logMs before and after doing things, we can safely do
> > things on demand because the order of operations will be clear in the log.
> > 
> > So I think all we need to do here is add the following line just before line
> > 291:
> > 
> > ticker.logMs("Compute library specification");
> 
> Done. Added the extra tick as Paul suggested.

I don't think that will work. This will print something like this:

Compute library specification in N ms.
Read libraries file in M ms.

Those N ms will apply to something else.

This is not how the ticker works, it only has one stopwatch. The alternatives
don't appeal to me:

* "measure" as known from dart2js can measure things that are on-demand and
asynchronous, but it adds a lot of overhead simply to measure.

* profiling, often statistically, and often measures short running programs
incorrectly. This makes it a poor tool for understanding start up performance
(which this code belongs to).

Unfortunately, it's not easy to make sure that the ticker is used correctly and
that all (potentially) expensive things are timed. But when we take care to do
it correctly, we see the correct timings all the time. This is important to me
as this is how I monitor performance. Golem is also nice, but not a substitute
for the in-your-face approach of Ticker.

Another option is to completely remove uses of Ticker from ProcessedOptions and
rely on the user of ProcessedOptions to initialize all the stuff that it needs
in a predicable sequence (by calling the corresponding getters).

FWIW, I wasn't suggesting that the client has to do computations before
initializing the ProcessedOptions object. The code snippet I provided could go
in a static async method on ProcessedOptions.