Start trying to compile platform from unpatched sources.

Start trying to compile platform from unpatched sources.

R=sigmund@google.com

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

[ahe, 2017-08-24 14:49:20]

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

[Siggi Cherem (dart-lang), 2017-08-24 19:27:02]

some suggestions below, but otherwise lgtm

https://codereview.chromium.org/3009503002/diff/1/pkg/front_end/test/fasta/co...
File pkg/front_end/test/fasta/compile_platform_test.dart (right):

https://codereview.chromium.org/3009503002/diff/1/pkg/front_end/test/fasta/co...
pkg/front_end/test/fasta/compile_platform_test.dart:23: await
compilePlatform(<String>[
Alternatively, should we use the public APIs here instead?

We already do so in`patch_sdk`, so it would be good to bypass the helper scripts
and command-line argument parsing since it's not a relevant piece of this test.

https://codereview.chromium.org/3009503002/diff/1/pkg/front_end/test/fasta/co...
pkg/front_end/test/fasta/compile_platform_test.dart:36: "sdk/",
it might be worth checking first that this folder is resolved and exists?

Some bots run with `--use-sdk`, in which case the folder you want might be in a
different location (or we may want to not run in those bots)

https://codereview.chromium.org/3009503002/diff/1/pkg/front_end/test/fasta/co...
pkg/front_end/test/fasta/compile_platform_test.dart:56: await
tmp.delete(recursive: true);
if we switch to use the public API, we should also be able to get rid of the
temp directory (we don't have to emit the files to disk, we would just create
the kernel program and toss it).

https://codereview.chromium.org/3009503002/diff/1/sdk/lib/libraries.yaml
File sdk/lib/libraries.yaml (right):

https://codereview.chromium.org/3009503002/diff/1/sdk/lib/libraries.yaml#newc...
sdk/lib/libraries.yaml:8: #     python ./tools/yaml2json.py
sdk/lib/libraries.yaml sdk/lib/libraries.json
btw - the current plan is to add a build rule that generates the .json file
automatically as a pre-requisite of building platform.dill.

One detail that is trickky is that the generated .json would be in an out/*
folder, so we'd need to adjust all relative URIs to the new location (or emit
them as absolute uris)

[ahe, 2017-08-25 08:43:17]

Patchset #2 (id:20001) has been deleted

[ahe, 2017-08-25 08:45:51]

Thank you, Siggi. I've uploaded a new patch set, but not really addressed any of
your comments.

https://codereview.chromium.org/3009503002/diff/1/pkg/front_end/test/fasta/co...
File pkg/front_end/test/fasta/compile_platform_test.dart (right):

https://codereview.chromium.org/3009503002/diff/1/pkg/front_end/test/fasta/co...
pkg/front_end/test/fasta/compile_platform_test.dart:23: await
compilePlatform(<String>[
On 2017/08/24 19:27:02, Siggi Cherem (dart-lang) wrote:
> Alternatively, should we use the public APIs here instead?
> 
> We already do so in`patch_sdk`, so it would be good to bypass the helper
scripts
> and command-line argument parsing since it's not a relevant piece of this
test.

I think it's a great idea to rewrite tool/_fasta/compile_platform.dart in terms
of the public API (I don't consider myself an owner of that task).

However, the main purpose of this test is to test that the tool works, not to
exercise the API. It is to address the lack of positive tests in 
pkg/front_end/test/fasta/tool_test.dart.

As soon as the tool stops crashing, I should probably switch to running it via
Process.

https://codereview.chromium.org/3009503002/diff/1/pkg/front_end/test/fasta/co...
pkg/front_end/test/fasta/compile_platform_test.dart:36: "sdk/",
On 2017/08/24 19:27:02, Siggi Cherem (dart-lang) wrote:
> it might be worth checking first that this folder is resolved and exists?
> 
> Some bots run with `--use-sdk`, in which case the folder you want might be in
a
> different location (or we may want to not run in those bots)

That will lead to a different error and this test will fail. But all tests in
this directory are already skipped in that configuration.

https://codereview.chromium.org/3009503002/diff/1/pkg/front_end/test/fasta/co...
pkg/front_end/test/fasta/compile_platform_test.dart:56: await
tmp.delete(recursive: true);
On 2017/08/24 19:27:02, Siggi Cherem (dart-lang) wrote:
> if we switch to use the public API, we should also be able to get rid of the
> temp directory (we don't have to emit the files to disk, we would just create
> the kernel program and toss it).

Yes, but that wouldn't test the tool. Perhaps we should have both tests.

https://codereview.chromium.org/3009503002/diff/1/sdk/lib/libraries.yaml
File sdk/lib/libraries.yaml (right):

https://codereview.chromium.org/3009503002/diff/1/sdk/lib/libraries.yaml#newc...
sdk/lib/libraries.yaml:8: #     python ./tools/yaml2json.py
sdk/lib/libraries.yaml sdk/lib/libraries.json
On 2017/08/24 19:27:02, Siggi Cherem (dart-lang) wrote:
> btw - the current plan is to add a build rule that generates the .json file
> automatically as a pre-requisite of building platform.dill.

Let's discuss this plan at our next 1:1.

[Siggi Cherem (dart-lang), 2017-08-25 14:41:48]

lgtm, thanks for clarifying.

https://codereview.chromium.org/3009503002/diff/1/pkg/front_end/test/fasta/co...
File pkg/front_end/test/fasta/compile_platform_test.dart (right):

https://codereview.chromium.org/3009503002/diff/1/pkg/front_end/test/fasta/co...
pkg/front_end/test/fasta/compile_platform_test.dart:23: await
compilePlatform(<String>[
On 2017/08/25 08:45:51, ahe wrote:
> On 2017/08/24 19:27:02, Siggi Cherem (dart-lang) wrote:
> > Alternatively, should we use the public APIs here instead?
> > 
> > We already do so in`patch_sdk`, so it would be good to bypass the helper
> scripts
> > and command-line argument parsing since it's not a relevant piece of this
> test.
> 
> I think it's a great idea to rewrite tool/_fasta/compile_platform.dart in
terms
> of the public API (I don't consider myself an owner of that task).
> 
> However, the main purpose of this test is to test that the tool works, not to
> exercise the API. It is to address the lack of positive tests in 
> pkg/front_end/test/fasta/tool_test.dart.
> 
> As soon as the tool stops crashing, I should probably switch to running it via
> Process.

OK - I misunderstood, I thought the goal was to have coverage for building the
platform with patch files, so we know that such functionality is working
correctly. Then maybe my suggestion is: when we start adding support for @patch,
it might be nice to have some unit test coverage that is testing that in
isolation if possible.

https://codereview.chromium.org/3009503002/diff/1/pkg/front_end/test/fasta/co...
pkg/front_end/test/fasta/compile_platform_test.dart:56: await
tmp.delete(recursive: true);
On 2017/08/25 08:45:51, ahe wrote:
> On 2017/08/24 19:27:02, Siggi Cherem (dart-lang) wrote:
> > if we switch to use the public API, we should also be able to get rid of the
> > temp directory (we don't have to emit the files to disk, we would just
create
> > the kernel program and toss it).
> 
> Yes, but that wouldn't test the tool. Perhaps we should have both tests.

yes :)

[ahe, 2017-08-29 14:09:29]

Description was changed from

==========
Start trying to compile platform from unpatched sources.
==========

to

==========
Start trying to compile platform from unpatched sources.

R=sigmund@google.com

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

[ahe, 2017-08-29 14:09:31]

Committed patchset #4 (id:80001) manually as
37d3c369d89644fa1c86e642cfe2e1c7b39a091d (presubmit successful).

[ahe, 2017-08-29 14:13:34]

Thank you, Siggi!

https://codereview.chromium.org/3009503002/diff/80001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/builder/library_builder.dart (right):

https://codereview.chromium.org/3009503002/diff/80001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/builder/library_builder.dart:73: bool get isPatch =>
false;
Had to add this before committing.