Read SDK and patches from a JSON file.

Read SDK and patches from a JSON file.

R=sigmund@google.com

Committed: https://github.com/dart-lang/sdk/commit/27761fd369b26f0b0c85fbd81172dda52bb2ae13

[ahe, 2017-05-22 15:29:41]

Patchset #1 (id:1) has been deleted

[ahe, 2017-05-22 15:33:53]

ahe@google.com changed reviewers:
+ sigmund@google.com, vegorov@google.com

[ahe, 2017-05-22 15:33:54]

This is ready for review, but I also need your help:

Siggi: could you help make sure that the dart2js version works as expected?

Slava: I'm creating another output file, do I need to update the build
dependencies?

Note: this CL has support for reading patch files, but it doesn't yet.

[Vyacheslav Egorov (Google), 2017-05-22 15:59:15]

If you want to declare this file as input to other targets then you need to
declare it as an output of the patched_sdk target. I don't think anything else
needs to be done.

[Siggi Cherem (dart-lang), 2017-05-22 16:21:16]

I didn't know you were about to do this, I was also working on this on Friday
afternoon. I'll drop what I have.

https://codereview.chromium.org/2895983002/diff/20001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/kernel/body_builder.dart (right):

https://codereview.chromium.org/2895983002/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/kernel/body_builder.dart:873: } else if
(enableNative &&
you probably don't need this one here

https://codereview.chromium.org/2895983002/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/kernel/body_builder.dart:877: member?.name ==
"_getMainClosure") {
actually - can this be deleted now that we no longer run the builtin
transformer?

https://codereview.chromium.org/2895983002/diff/20001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/translate_uri.dart (right):

https://codereview.chromium.org/2895983002/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/translate_uri.dart:21: final Map<String, List<Uri>>
patches;
BTW - I pinged Siva on Friday to see if we can change the VM's representation of
patches: I'd really like to have a single library Uri rather than a list of
files for this (like we do in dart2js).

https://codereview.chromium.org/2895983002/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/translate_uri.dart:31: Uri translateDartUri(Uri uri)
=> dartLibraries[uri.path];
Let's document the invariant that you told me about on Friday. Also, I'm
guessing we need to fix where Konstantin relied on the other behavior.

I can take on it separately if you prefer, in which case temporarily keep the
other behavior until we fix the use site.

https://codereview.chromium.org/2895983002/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/translate_uri.dart:48: //
`Uri.base.resolve("runtime/vm/libraries.json")`.
On the design doc, I was pushing for having a single file for declaring both the
library location and the patches. 

Some of the rationale: initially I thought it was nice to have a libraries.json
that is separate because so many libraries are available in more than one
platform, so technically we could share that .json file for all platforms. Then
I realized that every platform has at least a special library that is not
available in other platforms (vm-service for vm, js-helpers for dart2js, dart:ui
for flutter... ), so we end up needing a libraries.json per platform anyways. 

We can continue the discussion on that doc if you prefer.

I'm using YAML syntax here, but here is what I have in mind. If we have a single
Uri, it can look like:

libraries:
  async:
    path: uri1
    patch: uri2

If we must continue using a list of files for the VM then:

libraries:
  async:
    path: uri1
    patches:
      - uri2
      - uri3

https://codereview.chromium.org/2895983002/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/translate_uri.dart:88: Map<String, List<Uri>> result
= <String, List<Uri>>{};
until we add the support for patching and decide on how to encode it, can we
remove the body of this function and throw with an unimplemented error?

[ahe, 2017-05-22 16:26:47]

Thank you for taking a look, Siggi!

On 2017/05/22 16:21:16, Siggi Cherem (dart-lang) wrote:
> I didn't know you were about to do this, I was also working on this on Friday
> afternoon. I'll drop what I have.

Sorry about that.

https://codereview.chromium.org/2895983002/diff/20001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/kernel/body_builder.dart (right):

https://codereview.chromium.org/2895983002/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/kernel/body_builder.dart:873: } else if
(enableNative &&
On 2017/05/22 16:21:15, Siggi Cherem (dart-lang) wrote:
> you probably don't need this one here

It's an optimization.

https://codereview.chromium.org/2895983002/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/kernel/body_builder.dart:877: member?.name ==
"_getMainClosure") {
On 2017/05/22 16:21:15, Siggi Cherem (dart-lang) wrote:
> actually - can this be deleted now that we no longer run the builtin
> transformer?

I'll investigate.

https://codereview.chromium.org/2895983002/diff/20001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/translate_uri.dart (right):

https://codereview.chromium.org/2895983002/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/translate_uri.dart:21: final Map<String, List<Uri>>
patches;
On 2017/05/22 16:21:15, Siggi Cherem (dart-lang) wrote:
> BTW - I pinged Siva on Friday to see if we can change the VM's representation
of
> patches: I'd really like to have a single library Uri rather than a list of
> files for this (like we do in dart2js).

Agreed.

https://codereview.chromium.org/2895983002/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/translate_uri.dart:31: Uri translateDartUri(Uri uri)
=> dartLibraries[uri.path];
On 2017/05/22 16:21:15, Siggi Cherem (dart-lang) wrote:
> Let's document the invariant that you told me about on Friday. Also, I'm
> guessing we need to fix where Konstantin relied on the other behavior.
> 
> I can take on it separately if you prefer, in which case temporarily keep the
> other behavior until we fix the use site.

I'll take a further look at this.

https://codereview.chromium.org/2895983002/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/translate_uri.dart:48: //
`Uri.base.resolve("runtime/vm/libraries.json")`.
On 2017/05/22 16:21:15, Siggi Cherem (dart-lang) wrote:
> On the design doc, I was pushing for having a single file for declaring both
the
> library location and the patches. 
> 
> Some of the rationale: initially I thought it was nice to have a
libraries.json
> that is separate because so many libraries are available in more than one
> platform, so technically we could share that .json file for all platforms.
Then
> I realized that every platform has at least a special library that is not
> available in other platforms (vm-service for vm, js-helpers for dart2js,
dart:ui
> for flutter... ), so we end up needing a libraries.json per platform anyways. 
> 
> We can continue the discussion on that doc if you prefer.
> 
> I'm using YAML syntax here, but here is what I have in mind. If we have a
single
> Uri, it can look like:
> 
> libraries:
>   async:
>     path: uri1
>     patch: uri2
> 
> If we must continue using a list of files for the VM then:
> 
> libraries:
>   async:
>     path: uri1
>     patches:
>       - uri2
>       - uri3
> 

OK. That's great, in that case, I should delete runtime/vm/libraries.json.

https://codereview.chromium.org/2895983002/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/translate_uri.dart:88: Map<String, List<Uri>> result
= <String, List<Uri>>{};
On 2017/05/22 16:21:15, Siggi Cherem (dart-lang) wrote:
> until we add the support for patching and decide on how to encode it, can we
> remove the body of this function and throw with an unimplemented error?

Sure.

[ahe, 2017-05-24 16:09:46]

Siggi, I think I've addressed your comments. PTAL.

https://codereview.chromium.org/2895983002/diff/20001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/kernel/body_builder.dart (right):

https://codereview.chromium.org/2895983002/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/kernel/body_builder.dart:877: member?.name ==
"_getMainClosure") {
On 2017/05/22 16:26:47, ahe wrote:
> On 2017/05/22 16:21:15, Siggi Cherem (dart-lang) wrote:
> > actually - can this be deleted now that we no longer run the builtin
> > transformer?
> 
> I'll investigate.

No. The method still exists and refers to main (which isn't available in that
scope). So this suppresses an error.

https://codereview.chromium.org/2895983002/diff/20001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/translate_uri.dart (right):

https://codereview.chromium.org/2895983002/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/translate_uri.dart:31: Uri translateDartUri(Uri uri)
=> dartLibraries[uri.path];
On 2017/05/22 16:26:47, ahe wrote:
> On 2017/05/22 16:21:15, Siggi Cherem (dart-lang) wrote:
> > Let's document the invariant that you told me about on Friday. Also, I'm
> > guessing we need to fix where Konstantin relied on the other behavior.
> > 
> > I can take on it separately if you prefer, in which case temporarily keep
the
> > other behavior until we fix the use site.
> 
> I'll take a further look at this.

Let's go with the analyzer's approach. Personally, I have a strong preference
against implementing special rules for URI resolution. However, as long as we
keep them internal, it should matter.

[Siggi Cherem (dart-lang), 2017-05-24 16:24:04]

lgtm

https://codereview.chromium.org/2895983002/diff/60001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/source/source_library_builder.dart (right):

https://codereview.chromium.org/2895983002/diff/60001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/source/source_library_builder.dart:155: Uri
newFileUri;
when newFileUri is null is the underlying loader doing the translation from
resolvedUri? If so, and if you are adopting the analyzer-style to translate
dart-uris, then we could drop this argument entirely.

[ahe, 2017-05-24 16:51:41]

Thank you, Siggi.

https://codereview.chromium.org/2895983002/diff/60001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/source/source_library_builder.dart (right):

https://codereview.chromium.org/2895983002/diff/60001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/source/source_library_builder.dart:155: Uri
newFileUri;
On 2017/05/24 16:24:04, Siggi Cherem (dart-lang) wrote:
> when newFileUri is null is the underlying loader doing the translation from
> resolvedUri? If so, and if you are adopting the analyzer-style

I'm not adopting the analyzer style. I'm accepting that it exists, and making
sure that it remains supported by using resolveRelativeUri to compute unique
names for the parts instead of it being an accident.

> to translate
> dart-uris, then we could drop this argument entirely.

[Siggi Cherem (dart-lang), 2017-05-24 16:53:08]

On 2017/05/24 16:51:41, ahe wrote:
> Thank you, Siggi.
> 
>
https://codereview.chromium.org/2895983002/diff/60001/pkg/front_end/lib/src/f...
> File pkg/front_end/lib/src/fasta/source/source_library_builder.dart (right):
> 
>
https://codereview.chromium.org/2895983002/diff/60001/pkg/front_end/lib/src/f...
> pkg/front_end/lib/src/fasta/source/source_library_builder.dart:155: Uri
> newFileUri;
> On 2017/05/24 16:24:04, Siggi Cherem (dart-lang) wrote:
> > when newFileUri is null is the underlying loader doing the translation from
> > resolvedUri? If so, and if you are adopting the analyzer-style
> 
> I'm not adopting the analyzer style. I'm accepting that it exists, and making
> sure that it remains supported by using resolveRelativeUri to compute unique
> names for the parts instead of it being an accident.

Ah thanks - I misunderstood your comment earlier. Makes sense.
> 
> > to translate
> > dart-uris, then we could drop this argument entirely.

[ahe, 2017-05-24 21:10:46]

Alex, FYI

[ahe, 2017-05-25 13:58:04]

Description was changed from

==========
Read SDK and patches from a JSON file.
==========

to

==========
Read SDK and patches from a JSON file.

R=sigmund@google.com

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

[ahe, 2017-05-25 13:58:05]

Committed patchset #4 (id:80001) manually as
27761fd369b26f0b0c85fbd81172dda52bb2ae13 (presubmit successful).