Add support for SDK outline in KernelDriver.

Add support for SDK outline in KernelDriver.

I had to weaken file system createDirectory() to avoid conflicts when
we re-create SDK, and attempt to re-recreate its directory.

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

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

[Paul Berry, 2017-08-04 22:24:39]

lgtm

[Siggi Cherem (dart-lang), 2017-08-05 03:22:47]

lgtm 

+aam for a question on vmservice below

https://codereview.chromium.org/2993093003/diff/1/pkg/front_end/lib/src/incre...
File pkg/front_end/lib/src/incremental/file_state.dart (right):

https://codereview.chromium.org/2993093003/diff/1/pkg/front_end/lib/src/incre...
pkg/front_end/lib/src/incremental/file_state.dart:340: // We don't need to
process SDK libraries if we have SK outline.
nit: "SK" => "an SDK"

https://codereview.chromium.org/2993093003/diff/1/pkg/front_end/lib/src/incre...
pkg/front_end/lib/src/incremental/file_state.dart:341: if (skipSdkLibraries &&
absoluteUri.isScheme('dart')) {
rather than skipping any `dart:*` library, could we instead store which
libraries to skip?

I am thinking about this because dart:vmservice is not part of the vm's
platform.dill, but it is compiled separately. I don't how whether that comes up
in practice though, so it might be OK to have that as a TODO.

Alex - can you clarify if that's a scenario we need to support?

https://codereview.chromium.org/2993093003/diff/1/pkg/front_end/lib/src/incre...
File pkg/front_end/lib/src/incremental/kernel_driver.dart (right):

https://codereview.chromium.org/2993093003/diff/1/pkg/front_end/lib/src/incre...
pkg/front_end/lib/src/incremental/kernel_driver.dart:368:
_fsState.skipSdkLibraries = true;
if we track which libraries to skip, then I was thinking here you could provide
the uris of program.libraries

https://codereview.chromium.org/2993093003/diff/1/pkg/front_end/test/src/incr...
File pkg/front_end/test/src/incremental/kernel_driver_test.dart (right):

https://codereview.chromium.org/2993093003/diff/1/pkg/front_end/test/src/incr...
pkg/front_end/test/src/incremental/kernel_driver_test.dart:687: var result =
await generateKernel(
any reason why not use the public API instead? (`summaryFor(inputs, options)`)?

https://codereview.chromium.org/2993093003/diff/1/pkg/front_end/test/src/incr...
File pkg/front_end/test/src/incremental/mock_sdk.dart (right):

https://codereview.chromium.org/2993093003/diff/1/pkg/front_end/test/src/incr...
pkg/front_end/test/src/incremental/mock_sdk.dart:7: final _ASYNC = r'''
Yay - thank you, even with syntax highlighting, having these long string
constants as top levels really helps navigate the code of `createSdkFiles`
easier.

[scheglov, 2017-08-06 21:04:06]

https://codereview.chromium.org/2993093003/diff/1/pkg/front_end/lib/src/incre...
File pkg/front_end/lib/src/incremental/file_state.dart (right):

https://codereview.chromium.org/2993093003/diff/1/pkg/front_end/lib/src/incre...
pkg/front_end/lib/src/incremental/file_state.dart:340: // We don't need to
process SDK libraries if we have SK outline.
On 2017/08/05 03:22:47, Siggi Cherem (dart-lang) wrote:
> nit: "SK" => "an SDK"

Done.

https://codereview.chromium.org/2993093003/diff/1/pkg/front_end/lib/src/incre...
pkg/front_end/lib/src/incremental/file_state.dart:341: if (skipSdkLibraries &&
absoluteUri.isScheme('dart')) {
On 2017/08/05 03:22:47, Siggi Cherem (dart-lang) wrote:
> rather than skipping any `dart:*` library, could we instead store which
> libraries to skip?
> 
> I am thinking about this because dart:vmservice is not part of the vm's
> platform.dill, but it is compiled separately. I don't how whether that comes
up
> in practice though, so it might be OK to have that as a TODO.
> 
> Alex - can you clarify if that's a scenario we need to support?

I'm adding TODO for now.

https://codereview.chromium.org/2993093003/diff/1/pkg/front_end/lib/src/incre...
File pkg/front_end/lib/src/incremental/kernel_driver.dart (right):

https://codereview.chromium.org/2993093003/diff/1/pkg/front_end/lib/src/incre...
pkg/front_end/lib/src/incremental/kernel_driver.dart:368:
_fsState.skipSdkLibraries = true;
On 2017/08/05 03:22:47, Siggi Cherem (dart-lang) wrote:
> if we track which libraries to skip, then I was thinking here you could
provide
> the uris of program.libraries

ACK, I will make the change if we will find out that outline does not include
everything and we need to be able to compile other VM libraries.

https://codereview.chromium.org/2993093003/diff/1/pkg/front_end/test/src/incr...
File pkg/front_end/test/src/incremental/kernel_driver_test.dart (right):

https://codereview.chromium.org/2993093003/diff/1/pkg/front_end/test/src/incr...
pkg/front_end/test/src/incremental/kernel_driver_test.dart:687: var result =
await generateKernel(
On 2017/08/05 03:22:47, Siggi Cherem (dart-lang) wrote:
> any reason why not use the public API instead? (`summaryFor(inputs,
options)`)?

Well, this is what build uses.
And an attempt to use summaryFor() results in an exception.
---
file:///sdk/lib/core/core.dart: Error: Invalid access to
file:///sdk/lib/core/core.dart: the file is accessed in a modular hermetic
build, but it was not explicitly listed as an input.
---

[aam, 2017-08-07 14:41:34]

https://codereview.chromium.org/2993093003/diff/1/pkg/front_end/lib/src/incre...
File pkg/front_end/lib/src/incremental/file_state.dart (right):

https://codereview.chromium.org/2993093003/diff/1/pkg/front_end/lib/src/incre...
pkg/front_end/lib/src/incremental/file_state.dart:341: if (skipSdkLibraries &&
absoluteUri.isScheme('dart')) {
On 2017/08/05 03:22:47, Siggi Cherem (dart-lang) wrote:
> rather than skipping any `dart:*` library, could we instead store which
> libraries to skip?
> 
> I am thinking about this because dart:vmservice is not part of the vm's
> platform.dill, but it is compiled separately. I don't how whether that comes
up
> in practice though, so it might be OK to have that as a TODO.
> 
> Alex - can you clarify if that's a scenario we need to support?

Good point! Yes, it seems it won't work with how vmservice is compiled here: 
https://github.com/dart-lang/sdk/blob/master/tools/patch_sdk.dart#L145.

https://codereview.chromium.org/2993093003/diff/20001/pkg/front_end/lib/src/i...
File pkg/front_end/lib/src/incremental/kernel_driver.dart (right):

https://codereview.chromium.org/2993093003/diff/20001/pkg/front_end/lib/src/i...
pkg/front_end/lib/src/incremental/kernel_driver.dart:76: /// Might be `null` if
the byte are not provided, or if not loaded yet.
'bytes'

[Siggi Cherem (dart-lang), 2017-08-07 15:29:52]

lgtm

https://codereview.chromium.org/2993093003/diff/1/pkg/front_end/lib/src/incre...
File pkg/front_end/lib/src/incremental/file_state.dart (right):

https://codereview.chromium.org/2993093003/diff/1/pkg/front_end/lib/src/incre...
pkg/front_end/lib/src/incremental/file_state.dart:341: if (skipSdkLibraries &&
absoluteUri.isScheme('dart')) {
On 2017/08/07 14:41:34, aam wrote:
> On 2017/08/05 03:22:47, Siggi Cherem (dart-lang) wrote:
> > rather than skipping any `dart:*` library, could we instead store which
> > libraries to skip?
> > 
> > I am thinking about this because dart:vmservice is not part of the vm's
> > platform.dill, but it is compiled separately. I don't how whether that comes
> up
> > in practice though, so it might be OK to have that as a TODO.
> > 
> > Alex - can you clarify if that's a scenario we need to support?
> 
> Good point! Yes, it seems it won't work with how vmservice is compiled here: 
> https://github.com/dart-lang/sdk/blob/master/tools/patch_sdk.dart#L145.

Konstantin: I was thinking more about this over the weekend, and I think pushing
this further will help make the public API more intuitive and possibly make this
code more agnostic of the SDK.

In particular, I'm thinking that we should allow users to provide a
`CompilerOptions` with `sdkSummary` `inputSummaries` or `linkedDependencies`,
and we will use all those .dill files here. Then, assuming we use the
program.libraries directly, we will be able to ignore any of the import-Uris
here in getFile.

What I especially like about it is that we no longer have a special rule about
the "SDK", we simply have a special rule to attach an existing outline/dill file
and treat it as if all the sources mentioned there are never modified. The logic
to combine all those .dill files together will be exactly the same we use in
`generateKernel` today.

WDYT?

Alex - just to clarify, I assume in patch_sdk.dart we will continue to use the
regular batch compiler and not the incremental compiler. Do we expect to build
vmservice also in kernel-service, or only ahead of time when building the sdk
like we do now?

https://codereview.chromium.org/2993093003/diff/1/pkg/front_end/test/src/incr...
File pkg/front_end/test/src/incremental/kernel_driver_test.dart (right):

https://codereview.chromium.org/2993093003/diff/1/pkg/front_end/test/src/incr...
pkg/front_end/test/src/incremental/kernel_driver_test.dart:687: var result =
await generateKernel(
On 2017/08/06 21:04:06, scheglov wrote:
> On 2017/08/05 03:22:47, Siggi Cherem (dart-lang) wrote:
> > any reason why not use the public API instead? (`summaryFor(inputs,
> options)`)?
> 
> Well, this is what build uses.
> And an attempt to use summaryFor() results in an exception.
> ---
> file:///sdk/lib/core/core.dart: Error: Invalid access to
> file:///sdk/lib/core/core.dart: the file is accessed in a modular hermetic
> build, but it was not explicitly listed as an input.
> ---

Hopefully just setting `options.chaseDependencies = true` should fix that (the
default is false when using the `summaryFor` API).

[aam, 2017-08-07 15:42:11]

https://codereview.chromium.org/2993093003/diff/1/pkg/front_end/lib/src/incre...
File pkg/front_end/lib/src/incremental/file_state.dart (right):

https://codereview.chromium.org/2993093003/diff/1/pkg/front_end/lib/src/incre...
pkg/front_end/lib/src/incremental/file_state.dart:341: if (skipSdkLibraries &&
absoluteUri.isScheme('dart')) {
On 2017/08/07 15:29:52, Siggi Cherem (dart-lang) wrote:
> Alex - just to clarify, I assume in patch_sdk.dart we will continue to use the
> regular batch compiler and not the incremental compiler. Do we expect to build
> vmservice also in kernel-service, or only ahead of time when building the sdk
> like we do now?

Ah, true, I don't see patch_sdk.dart using incremental compiler in the nearest
future.

[scheglov, 2017-08-07 15:51:26]

https://codereview.chromium.org/2993093003/diff/1/pkg/front_end/lib/src/incre...
File pkg/front_end/lib/src/incremental/file_state.dart (right):

https://codereview.chromium.org/2993093003/diff/1/pkg/front_end/lib/src/incre...
pkg/front_end/lib/src/incremental/file_state.dart:341: if (skipSdkLibraries &&
absoluteUri.isScheme('dart')) {
On 2017/08/07 15:29:52, Siggi Cherem (dart-lang) wrote:
> On 2017/08/07 14:41:34, aam wrote:
> > On 2017/08/05 03:22:47, Siggi Cherem (dart-lang) wrote:
> > > rather than skipping any `dart:*` library, could we instead store which
> > > libraries to skip?
> > > 
> > > I am thinking about this because dart:vmservice is not part of the vm's
> > > platform.dill, but it is compiled separately. I don't how whether that
comes
> > up
> > > in practice though, so it might be OK to have that as a TODO.
> > > 
> > > Alex - can you clarify if that's a scenario we need to support?
> > 
> > Good point! Yes, it seems it won't work with how vmservice is compiled here:

> > https://github.com/dart-lang/sdk/blob/master/tools/patch_sdk.dart#L145.
> 
> Konstantin: I was thinking more about this over the weekend, and I think
pushing
> this further will help make the public API more intuitive and possibly make
this
> code more agnostic of the SDK.
> 
> In particular, I'm thinking that we should allow users to provide a
> `CompilerOptions` with `sdkSummary` `inputSummaries` or `linkedDependencies`,
> and we will use all those .dill files here. Then, assuming we use the
> program.libraries directly, we will be able to ignore any of the import-Uris
> here in getFile.
> 
> What I especially like about it is that we no longer have a special rule about
> the "SDK", we simply have a special rule to attach an existing outline/dill
file
> and treat it as if all the sources mentioned there are never modified. The
logic
> to combine all those .dill files together will be exactly the same we use in
> `generateKernel` today.
> 
> WDYT?

The first problem is that we need to make sure that signatures of results
continue depending on all inputs, including input kernels. It is convenient to
do when you have bytes of these kernels, so you can mix them into the salt, and
load libraries from them. OTOH, Library or Program objects from CompilerOptions
don't allow this.

Them main question is however whether Kernel Driver is going to be used in this
way. My understanding is that it will be used as Analysis Driver is used - in
interactive environment, where the driver manages its dependencies and cached
results. For batch compilation, with external dependencies management, there is
the batch compiler.

https://codereview.chromium.org/2993093003/diff/1/pkg/front_end/lib/src/incre...
pkg/front_end/lib/src/incremental/file_state.dart:341: if (skipSdkLibraries &&
absoluteUri.isScheme('dart')) {
On 2017/08/07 15:42:11, aam wrote:
> On 2017/08/07 15:29:52, Siggi Cherem (dart-lang) wrote:
> > Alex - just to clarify, I assume in patch_sdk.dart we will continue to use
the
> > regular batch compiler and not the incremental compiler. Do we expect to
build
> > vmservice also in kernel-service, or only ahead of time when building the
sdk
> > like we do now?
> 
> Ah, true, I don't see patch_sdk.dart using incremental compiler in the nearest
> future.
> 

So, is there still a reason to change the way `dart` libraries are skipped?

https://codereview.chromium.org/2993093003/diff/1/pkg/front_end/test/src/incr...
File pkg/front_end/test/src/incremental/kernel_driver_test.dart (right):

https://codereview.chromium.org/2993093003/diff/1/pkg/front_end/test/src/incr...
pkg/front_end/test/src/incremental/kernel_driver_test.dart:687: var result =
await generateKernel(
On 2017/08/07 15:29:52, Siggi Cherem (dart-lang) wrote:
> On 2017/08/06 21:04:06, scheglov wrote:
> > On 2017/08/05 03:22:47, Siggi Cherem (dart-lang) wrote:
> > > any reason why not use the public API instead? (`summaryFor(inputs,
> > options)`)?
> > 
> > Well, this is what build uses.
> > And an attempt to use summaryFor() results in an exception.
> > ---
> > file:///sdk/lib/core/core.dart: Error: Invalid access to
> > file:///sdk/lib/core/core.dart: the file is accessed in a modular hermetic
> > build, but it was not explicitly listed as an input.
> > ---
> 
> Hopefully just setting `options.chaseDependencies = true` should fix that (the
> default is false when using the `summaryFor` API).

Yes, this fixed the problem.

https://codereview.chromium.org/2993093003/diff/20001/pkg/front_end/lib/src/i...
File pkg/front_end/lib/src/incremental/kernel_driver.dart (right):

https://codereview.chromium.org/2993093003/diff/20001/pkg/front_end/lib/src/i...
pkg/front_end/lib/src/incremental/kernel_driver.dart:76: /// Might be `null` if
the byte are not provided, or if not loaded yet.
On 2017/08/07 14:41:34, aam wrote:
> 'bytes'

Done.

[aam, 2017-08-07 16:04:39]

https://codereview.chromium.org/2993093003/diff/1/pkg/front_end/lib/src/incre...
File pkg/front_end/lib/src/incremental/file_state.dart (right):

https://codereview.chromium.org/2993093003/diff/1/pkg/front_end/lib/src/incre...
pkg/front_end/lib/src/incremental/file_state.dart:341: if (skipSdkLibraries &&
absoluteUri.isScheme('dart')) {
On 2017/08/07 15:51:25, scheglov wrote:
> On 2017/08/07 15:42:11, aam wrote:
> > On 2017/08/07 15:29:52, Siggi Cherem (dart-lang) wrote:
> > > Alex - just to clarify, I assume in patch_sdk.dart we will continue to use
> the
> > > regular batch compiler and not the incremental compiler. Do we expect to
> build
> > > vmservice also in kernel-service, or only ahead of time when building the
> sdk
> > > like we do now?
> > 
> > Ah, true, I don't see patch_sdk.dart using incremental compiler in the
nearest
> > future.
> > 
> 
> So, is there still a reason to change the way `dart` libraries are skipped?

Not sure, about short term, but in long term, to me it sounds surprising  that
the only way to compile dart:vmservice will be with batch compiler, not with
IKG. I would expect that both batch compiler and 1st run of IKG behave similarly
on the same input.

[Siggi Cherem (dart-lang), 2017-08-07 16:27:43]

https://codereview.chromium.org/2993093003/diff/1/pkg/front_end/lib/src/incre...
File pkg/front_end/lib/src/incremental/file_state.dart (right):

https://codereview.chromium.org/2993093003/diff/1/pkg/front_end/lib/src/incre...
pkg/front_end/lib/src/incremental/file_state.dart:341: if (skipSdkLibraries &&
absoluteUri.isScheme('dart')) {
On 2017/08/07 15:51:25, scheglov wrote:
> On 2017/08/07 15:29:52, Siggi Cherem (dart-lang) wrote:
> > On 2017/08/07 14:41:34, aam wrote:
> > > On 2017/08/05 03:22:47, Siggi Cherem (dart-lang) wrote:
> > > > rather than skipping any `dart:*` library, could we instead store which
> > > > libraries to skip?
> > > > 
> > > > I am thinking about this because dart:vmservice is not part of the vm's
> > > > platform.dill, but it is compiled separately. I don't how whether that
> comes
> > > up
> > > > in practice though, so it might be OK to have that as a TODO.
> > > > 
> > > > Alex - can you clarify if that's a scenario we need to support?
> > > 
> > > Good point! Yes, it seems it won't work with how vmservice is compiled
here:
> 
> > > https://github.com/dart-lang/sdk/blob/master/tools/patch_sdk.dart#L145.
> > 
> > Konstantin: I was thinking more about this over the weekend, and I think
> pushing
> > this further will help make the public API more intuitive and possibly make
> this
> > code more agnostic of the SDK.
> > 
> > In particular, I'm thinking that we should allow users to provide a
> > `CompilerOptions` with `sdkSummary` `inputSummaries` or
`linkedDependencies`,
> > and we will use all those .dill files here. Then, assuming we use the
> > program.libraries directly, we will be able to ignore any of the import-Uris
> > here in getFile.
> > 
> > What I especially like about it is that we no longer have a special rule
about
> > the "SDK", we simply have a special rule to attach an existing outline/dill
> file
> > and treat it as if all the sources mentioned there are never modified. The
> logic
> > to combine all those .dill files together will be exactly the same we use in
> > `generateKernel` today.
> > 
> > WDYT?
> 
> The first problem is that we need to make sure that signatures of results
> continue depending on all inputs, including input kernels. It is convenient to
> do when you have bytes of these kernels, so you can mix them into the salt,
and
> load libraries from them. OTOH, Library or Program objects from
CompilerOptions
> don't allow this.

So basically we need ProcessedOptions to expose the bytes or the URI (and not
just the parsed program) for all these input .dill files, correct?

Note that we need to do this already (I believe): after this CL we need to
expose a way to specify the sdk outline in the IKG API. I thought we will use
CompilerOptions.sdkSummary to specify the URI location, just like we do in all
other APIs, and make ProcessedOptions expose to you internally anything else
that you need.


> 
> Them main question is however whether Kernel Driver is going to be used in
this
> way. My understanding is that it will be used as Analysis Driver is used - in
> interactive environment, where the driver manages its dependencies and cached
> results. For batch compilation, with external dependencies management, there
is
> the batch compiler.

Good question - I don't know yet. Alex's new use case seems to be somewhere in
between: a batch compiler with the ability to support hot-reload.

Even without this answer, I think there is also value in having all of our
compiler options working consistently across our different APIs. Maybe this will
become more clear when we add the IKG API to expose this change in kernel
driver. Given how the logic to support it is practically the same, I am inclined
to support it as well.
 If we don't allow providing inputSummary and linkedDependency as flags, then
let's report an error for those in the IKG saying that they are not supported at
this time. 

Independently of what we do in IKG - I thought you preferred to make
kernel-driver more agnostic of the SDK. If so, just saying there is a list of
bytes that we load in order, and any library from those is not compiled from
sources, seems to me like a clean way to achieve that.

[scheglov, 2017-08-07 17:46:49]

Description was changed from

==========
Add support for SDK outline in KernelDriver.

I had to weaken file system createDirectory() to avoid conflicts when
we re-create SDK, and attempt to re-recreate its directory.

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

to

==========
Add support for SDK outline in KernelDriver.

I had to weaken file system createDirectory() to avoid conflicts when
we re-create SDK, and attempt to re-recreate its directory.

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

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

[scheglov, 2017-08-07 17:46:51]

Committed patchset #5 (id:80001) manually as
c28419bce0e2bbd615a63b874f37b32a8c0687c7 (presubmit successful).