Add a vmservice_sdk directory in runtime/bin.

Add a vmservice_sdk directory in runtime/bin.

This directory has a libraries.json file pointing to vmservice_io and
_vmservice in the source tree. The script tools/patch_sdk.dart has been
updated to use this new directory as the sdk directory when compiling
dart:vmservice_io. This way, to build vmservice_io.dill, we
do not need to copy the dart files pertaining to vmservice_io and
_vmservice.

Fixes #29859

R=sigmund@google.com

Committed: https://github.com/dart-lang/sdk/commit/65a5707189dec6c5e0174cbc821acfb90f0c9b3f

[sivachandra, 2017-06-15 00:05:45]

sivachandra@google.com changed reviewers:
+ ahe@google.com, asiva@google.com, sigmund@google.com

[sivachandra, 2017-06-15 00:05:46]

See the discussion around this change here:
https://github.com/dart-lang/sdk/issues/29859

[Siggi Cherem (dart-lang), 2017-06-15 01:10:19]

lgtm, but please wait for Peter to chime in as well.

+aam - just FYI, this change will slightly modify how vmservice_sky is built, so
you might need to test it against flutter-engine after it's rebased on top of my
other change.

https://codereview.chromium.org/2938903003/diff/1/tools/patch_sdk.dart
File tools/patch_sdk.dart (right):

https://codereview.chromium.org/2938903003/diff/1/tools/patch_sdk.dart#newcod...
tools/patch_sdk.dart:123: Uri sdkDir =
nit: rename to `repositoryDir`? `sdkDir` might be confused with the many other
sdk dirs we have (patched_sdk, vmservice_sdk).

https://codereview.chromium.org/2938903003/diff/1/tools/patch_sdk.dart#newcod...
tools/patch_sdk.dart:125: Uri vmserviceSdk =
sdkDir.resolve('runtime/bin/vmservice_sdk/');
I'd like to eventually get rid of the fake vmservice_sdk folder. I believe you
need this because fasta doesn't have a flag to provide the libraries.json
directly, but you need to provide a --sdk flag instead.

For now, I'm ok adding a TODO referring to
https://github.com/dart-lang/sdk/issues/29882. 

Eventually the fix might be to add that flag so you can use libraries.json
directly next to vmservice_io.dart, or, we can use a different API here and
provide the contents of libraries.json in memory instead. For example, instead
of calling fasta.compile, you would invoke the public API in
package:front_end/kernel_generator.dart. That public API has an option to
specify the dartLibraries
(https://github.com/dart-lang/sdk/blob/master/pkg/front_end/lib/compiler_optio...)
explicitly.

I prefer the latter approach and had the intention of anyways migrating this
entire file to use the public APIs in the near future. I'm happy to switch this
call here together with the rest.

https://codereview.chromium.org/2938903003/diff/1/tools/patch_sdk.dart#newcod...
tools/patch_sdk.dart:126: await fasta.compile([
now that this is independent of building the patched_sdk, I wonder if we should
pull this out of the patch_sdk.dart script entirely, and instead use a separate
build rule to generate the vmservice.dill file. WDYT?

https://codereview.chromium.org/2938903003/diff/1/tools/patch_sdk.dart#newcod...
tools/patch_sdk.dart:127: "--sdk=${vmserviceSdk.toString()}",
FYI - I happen to just land a CL that updates these lines slightly, so you may
have a conflict here.

Note also that you can simply write '$vmserviceSdk' here. The .toString is
implicitly called when using interpolation. I also prefer writing '$foo' instead
of foo.toString() (e.g. like the in the last argument below)

[sivachandra, 2017-06-15 06:10:54]

Addressed comments in patch set 2.

https://codereview.chromium.org/2938903003/diff/1/tools/patch_sdk.dart
File tools/patch_sdk.dart (right):

https://codereview.chromium.org/2938903003/diff/1/tools/patch_sdk.dart#newcod...
tools/patch_sdk.dart:123: Uri sdkDir =
On 2017/06/15 01:10:19, Siggi Cherem (dart-lang) wrote:
> nit: rename to `repositoryDir`? `sdkDir` might be confused with the many other
> sdk dirs we have (patched_sdk, vmservice_sdk).

Done.

https://codereview.chromium.org/2938903003/diff/1/tools/patch_sdk.dart#newcod...
tools/patch_sdk.dart:125: Uri vmserviceSdk =
sdkDir.resolve('runtime/bin/vmservice_sdk/');
On 2017/06/15 01:10:19, Siggi Cherem (dart-lang) wrote:
> I'd like to eventually get rid of the fake vmservice_sdk folder. I believe you
> need this because fasta doesn't have a flag to provide the libraries.json
> directly, but you need to provide a --sdk flag instead.
> 
> For now, I'm ok adding a TODO referring to
> https://github.com/dart-lang/sdk/issues/29882. 
> 
> Eventually the fix might be to add that flag so you can use libraries.json
> directly next to vmservice_io.dart, or, we can use a different API here and
> provide the contents of libraries.json in memory instead. For example, instead
> of calling fasta.compile, you would invoke the public API in
> package:front_end/kernel_generator.dart. That public API has an option to
> specify the dartLibraries
>
(https://github.com/dart-lang/sdk/blob/master/pkg/front_end/lib/compiler_optio...)
> explicitly.
> 
> I prefer the latter approach and had the intention of anyways migrating this
> entire file to use the public APIs in the near future. I'm happy to switch
this
> call here together with the rest.

Added the TODO.

https://codereview.chromium.org/2938903003/diff/1/tools/patch_sdk.dart#newcod...
tools/patch_sdk.dart:126: await fasta.compile([
On 2017/06/15 01:10:19, Siggi Cherem (dart-lang) wrote:
> now that this is independent of building the patched_sdk, I wonder if we
should
> pull this out of the patch_sdk.dart script entirely, and instead use a
separate
> build rule to generate the vmservice.dill file. WDYT?

I considered that approach but decided against it because of:

1. We want to co-locate platform.dill and vmservice_io.dill in a single
directory to avoid multiple options to the VM. See
https://codereview.chromium.org/2933203004/
2. Dealing with *.dill files directly is only a temporary measure. We want to
move over to snapshots as soon as possible (which might be a few weeks away).

https://codereview.chromium.org/2938903003/diff/1/tools/patch_sdk.dart#newcod...
tools/patch_sdk.dart:127: "--sdk=${vmserviceSdk.toString()}",
On 2017/06/15 01:10:19, Siggi Cherem (dart-lang) wrote:
> FYI - I happen to just land a CL that updates these lines slightly, so you may
> have a conflict here.
> 
> Note also that you can simply write '$vmserviceSdk' here. The .toString is
> implicitly called when using interpolation. I also prefer writing '$foo'
instead
> of foo.toString() (e.g. like the in the last argument below)

Done.

[Siggi Cherem (dart-lang), 2017-06-15 17:43:39]

lgtm

I think Peter just didn't get the chance to reply, but I talked informally with
him this morning and he is supportive of landing this. Let's go ahead and if he
has any additional suggestions we can address them as a follow up.

https://codereview.chromium.org/2938903003/diff/20001/tools/patch_sdk.dart
File tools/patch_sdk.dart (right):

https://codereview.chromium.org/2938903003/diff/20001/tools/patch_sdk.dart#ne...
tools/patch_sdk.dart:133: Uri vmserviceSdk =
repositoryDir.resolve('runtime/bin/vmservice_sdk/');
Sorry I didn't notice this earlier, I just realized this now that you rebased
the CL. This Uri here might not work when building in the flutter-engine repo.

I'm not sure that Alex is using this right now, so for now let's only call
`compile` below when `!forFlutter`.

[sivachandra, 2017-06-15 18:34:21]

https://codereview.chromium.org/2938903003/diff/20001/tools/patch_sdk.dart
File tools/patch_sdk.dart (right):

https://codereview.chromium.org/2938903003/diff/20001/tools/patch_sdk.dart#ne...
tools/patch_sdk.dart:133: Uri vmserviceSdk =
repositoryDir.resolve('runtime/bin/vmservice_sdk/');
On 2017/06/15 17:43:39, Siggi Cherem (dart-lang) wrote:
> Sorry I didn't notice this earlier, I just realized this now that you rebased
> the CL. This Uri here might not work when building in the flutter-engine repo.
> 
> I'm not sure that Alex is using this right now, so for now let's only call
> `compile` below when `!forFlutter`.

How is it in patch set 3?

[Siggi Cherem (dart-lang), 2017-06-15 19:43:25]

small change below, otherwise looks good!

https://codereview.chromium.org/2938903003/diff/40001/tools/patch_sdk.dart
File tools/patch_sdk.dart (right):

https://codereview.chromium.org/2938903003/diff/40001/tools/patch_sdk.dart#ne...
tools/patch_sdk.dart:132: if (!forFlutter) {
change to `forVm` (since it's now outside the other if, we don't want to run
this step when forDart2js is true)

[sivachandra, 2017-06-15 20:08:15]

Description was changed from

==========
Add a vmservice_sdk directory in runtime/bin.

This directory has a libraries.json file pointing to vmservice_io and
_vmservice in the source tree. The script tools/patch_sdk.dart has been
updated to use this new directory as the sdk directory when compiling
dart:vmservice_io. This way, to build vmservice_io.dill, we
do not need to copy the dart files pertaining to vmservice_io and
_vmservice.

Fixes #29859
==========

to

==========
Add a vmservice_sdk directory in runtime/bin.

This directory has a libraries.json file pointing to vmservice_io and
_vmservice in the source tree. The script tools/patch_sdk.dart has been
updated to use this new directory as the sdk directory when compiling
dart:vmservice_io. This way, to build vmservice_io.dill, we
do not need to copy the dart files pertaining to vmservice_io and
_vmservice.

Fixes #29859

R=sigmund@google.com

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

[sivachandra, 2017-06-15 20:08:16]

Committed patchset #4 (id:60001) manually as
65a5707189dec6c5e0174cbc821acfb90f0c9b3f (presubmit successful).