Add support for building patched_sdk and platform.dill for dart2js:

Add support for building patched_sdk and platform.dill for dart2js:

 - defines a .gn target for patched_dart2js_sdk
 - changes patch_sdk.dart to handle a dart2js target
 - adds support in fasta to understand the `native` clause still used by dart2js
 - add option to fasta to know whether it is generating kernel for the vm or dart2js

I'm not excited about how I added the target distinction in fasta, the reason I
did so is that it affects the behavior in several ways: in how we handle
`native`, in the libraries that are included by default in the platform.dill file,
and in the transformations that need to be done.  I'm happy to change the
approach.

BUG=
R=ahe@google.com, vegorov@google.com, zra@google.com

Committed: https://github.com/dart-lang/sdk/commit/714523b103598d49295fa06b4ad13c6917c50399
Committed: https://github.com/dart-lang/sdk/commit/de36d118175ddf8de915a47b9b13d7c64d1c39c8

[Siggi Cherem (dart-lang), 2017-04-21 23:46:54]

Patchset #1 (id:1) has been deleted

[Siggi Cherem (dart-lang), 2017-04-21 23:51:08]

Patchset #1 (id:20001) has been deleted

[Siggi Cherem (dart-lang), 2017-04-21 23:51:48]

Patchset #1 (id:40001) has been deleted

[Siggi Cherem (dart-lang), 2017-04-21 23:53:53]

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

[Siggi Cherem (dart-lang), 2017-04-26 16:56:35]

friendly ping


Also - I have a few ideas for some improvements on what is currently hardcoded
in terms of dart2js and vm under this CL, but I'd like to do that on a separate
CL.

We can chat more offline about it, but here are some of my ideas:

- library locations: I believe this is where you where heading with
dart_server.json. I'd like to make the compiler take in a map specifying the
location of all the files (created from the json file).

- extraLibraries: when compiling the sdk we need to enqueue a few of the
internal libraries. I'd like to plumb this as a flag (something like
https://github.com/dart-lang/sdk/blob/f27b6cc11f582345e2e4228ca0ac81f78fd9d2b...)

- we could make fasta consume a "kernel.Target" or something similar, that will
do everything that is target specific. This includes: the extraLibraries above,
transformations, and possibly the logic about `native`.

On the other hand, I'd like to get rid of the use of `native` in dart2js (and
possibly the vm). We haven't gotten around to do it, but we should.

[ahe, 2017-04-27 11:48:31]

Two initial comments.

https://codereview.chromium.org/2832353002/diff/80001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/kernel/kernel_target.dart (right):

https://codereview.chromium.org/2832353002/diff/80001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/kernel/kernel_target.dart:94: class KernelTarget
extends TargetImplementation {
Would it make sense to have two subclasses of this: one for the Dart VM, and one
for dart2js?

Ideally we can avoid having the word "dart2js" in Fasta, and we can avoid
importing anything from package:compiler into fasta. Instead, package:compiler
would be able to import from Fasta.

https://codereview.chromium.org/2832353002/diff/80001/pkg/kernel/lib/target/t...
File pkg/kernel/lib/target/targets.dart (right):

https://codereview.chromium.org/2832353002/diff/80001/pkg/kernel/lib/target/t...
pkg/kernel/lib/target/targets.dart:34: 'dart2js': (TargetFlags flags) => new
Dart2jsTarget(flags),
Since targets is public, you can add this target from outside package:kernel. I
think that's preferable as package:compiler is clearly owned by a team that's
different from the kernel team.

Potentially, all the targets could live outside package:kernel, but right now
people working on package:kernel are also primarily working on the targets
listed in this file.

[ahe, 2017-04-27 13:34:35]

Slava: could you help us by reviewing tools/patch_sdk.dart and
utils/compiler/BUILD.gn?

I'm not strong in GN.

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

https://codereview.chromium.org/2832353002/diff/80001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/compile_platform.dart:69: new DillTarget(ticker,
uriTranslator, c.options.targetDart2js);
If DillTarget needs to know about dart2js, we should probably also make a
subclass for that.

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

https://codereview.chromium.org/2832353002/diff/80001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/compiler_command_line.dart:60: return
argumentError(usage, "Invalid target");
"Unknown target: '${options["--target"]}'."

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

https://codereview.chromium.org/2832353002/diff/80001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/kernel/body_builder.dart:2498: if
(library.loader.target.forDart2js) {
Can we move this to a subclass of BodyBuilder?

https://codereview.chromium.org/2832353002/diff/80001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/parser/dart2js_native.dart (right):

https://codereview.chromium.org/2832353002/diff/80001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/parser/dart2js_native.dart:1: // Copyright (c) 2017,
the Dart project authors.  Please see the AUTHORS file
Can this live in package:compiler somehow?

https://codereview.chromium.org/2832353002/diff/80001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/source/diet_listener.dart (right):

https://codereview.chromium.org/2832353002/diff/80001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/source/diet_listener.dart:488: if
(library.loader.target.forDart2js) {
Could be in a subclass?

[Vyacheslav Egorov (Google), 2017-04-27 14:07:22]

vegorov@google.com changed reviewers:
+ vegorov@google.com

[Vyacheslav Egorov (Google), 2017-04-27 14:07:23]

patch_sdk.dart & BUILD.gn LGTM

https://codereview.chromium.org/2832353002/diff/80001/tools/patch_sdk.dart
File tools/patch_sdk.dart (right):

https://codereview.chromium.org/2832353002/diff/80001/tools/patch_sdk.dart#ne...
tools/patch_sdk.dart:55: void usage(String mode) {
mode is not used in the body of this function

https://codereview.chromium.org/2832353002/diff/80001/tools/patch_sdk.dart#ne...
tools/patch_sdk.dart:81: Future _vmMain(List<String> argv) async {
vmMain and dart2jsMain have very similar prefix and suffix. We should consider
splitting as much shared parts out as possible.

https://codereview.chromium.org/2832353002/diff/80001/tools/patch_sdk.dart#ne...
tools/patch_sdk.dart:84: if (argv.length != 5 || argv.first != 'vm') {
argv.first != 'vm' can't happen anymore.

https://codereview.chromium.org/2832353002/diff/80001/utils/compiler/BUILD.gn
File utils/compiler/BUILD.gn (right):

https://codereview.chromium.org/2832353002/diff/80001/utils/compiler/BUILD.gn...
utils/compiler/BUILD.gn:80: action("patched_dart2js_sdk") {
It might be a good idea to move all shared GN rules for creating patched sdk
into a template:

template("patched_sdk") {
  // ...
}

then use it here and in the VM BUILD.gn:

patched_sdk("patched_dart2js_sdk") {
  deps = [...] 
  patches_dir = "..."
  patched_sdk_dir = "..."
}

[Siggi Cherem (dart-lang), 2017-04-28 20:17:48]

Patchset #3 (id:100001) has been deleted

[Siggi Cherem (dart-lang), 2017-04-28 20:17:55]

Patchset #3 (id:120001) has been deleted

[Siggi Cherem (dart-lang), 2017-04-28 20:21:21]

Patchset #3 (id:140001) has been deleted

[Siggi Cherem (dart-lang), 2017-04-28 21:15:40]

Patchset #3 (id:160001) has been deleted

[Siggi Cherem (dart-lang), 2017-04-28 21:31:00]

Patchset #3 (id:180001) has been deleted

[Siggi Cherem (dart-lang), 2017-04-28 21:37:21]

Thanks for all the comments. PTAL.

With your suggestion, this is much better:
 - fasta has nothing dart2js specific
 - I moved anything that depends on the target to TargetImplementation
 - in package:compiler I created subclasses for DillTarget and KernelTarget that
added the dart2js-specific logic. I'd still like to get rid of that extra logic
in package:compiler eventually, so I added a few TODOs for that.
 - I also added a helper script in pkg/compiler which is equivalent to `fasta
compile`

GN and patch_sdk is refactored to share more code. Thanks for the suggestion
Slava!

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

https://codereview.chromium.org/2832353002/diff/80001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/compile_platform.dart:69: new DillTarget(ticker,
uriTranslator, c.options.targetDart2js);
On 2017/04/27 13:34:34, ahe wrote:
> If DillTarget needs to know about dart2js, we should probably also make a
> subclass for that.

Done.

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

https://codereview.chromium.org/2832353002/diff/80001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/compiler_command_line.dart:60: return
argumentError(usage, "Invalid target");
On 2017/04/27 13:34:34, ahe wrote:
> "Unknown target: '${options["--target"]}'."

n/a anymore (flag is gone)

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

https://codereview.chromium.org/2832353002/diff/80001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/kernel/body_builder.dart:2498: if
(library.loader.target.forDart2js) {
On 2017/04/27 13:34:34, ahe wrote:
> Can we move this to a subclass of BodyBuilder?

this moved to the target, which is easier to override once in package:compiler.
I still would like to get rid of the extra logic for this, so I added some TODOs
about it.

https://codereview.chromium.org/2832353002/diff/80001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/kernel/kernel_target.dart (right):

https://codereview.chromium.org/2832353002/diff/80001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/kernel/kernel_target.dart:94: class KernelTarget
extends TargetImplementation {
On 2017/04/27 11:48:31, ahe wrote:
> Would it make sense to have two subclasses of this: one for the Dart VM, and
one
> for dart2js?
> 
> Ideally we can avoid having the word "dart2js" in Fasta, and we can avoid
> importing anything from package:compiler into fasta. Instead, package:compiler
> would be able to import from Fasta.

Done. Thanks for the suggestion. I made the change in this direction, but
eventually I'd like to shift to pass options and get rid of the subclasses.

There should be no reference to dart2js in fasta anymore

https://codereview.chromium.org/2832353002/diff/80001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/parser/dart2js_native.dart (right):

https://codereview.chromium.org/2832353002/diff/80001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/parser/dart2js_native.dart:1: // Copyright (c) 2017,
the Dart project authors.  Please see the AUTHORS file
On 2017/04/27 13:34:34, ahe wrote:
> Can this live in package:compiler somehow?

Done.

https://codereview.chromium.org/2832353002/diff/80001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/source/diet_listener.dart (right):

https://codereview.chromium.org/2832353002/diff/80001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/source/diet_listener.dart:488: if
(library.loader.target.forDart2js) {
On 2017/04/27 13:34:34, ahe wrote:
> Could be in a subclass?

Moved this into the target instead.

https://codereview.chromium.org/2832353002/diff/80001/pkg/kernel/lib/target/t...
File pkg/kernel/lib/target/targets.dart (right):

https://codereview.chromium.org/2832353002/diff/80001/pkg/kernel/lib/target/t...
pkg/kernel/lib/target/targets.dart:34: 'dart2js': (TargetFlags flags) => new
Dart2jsTarget(flags),
On 2017/04/27 11:48:31, ahe wrote:
> Since targets is public, you can add this target from outside package:kernel.
I
> think that's preferable as package:compiler is clearly owned by a team that's
> different from the kernel team.
> 
> Potentially, all the targets could live outside package:kernel, but right now
> people working on package:kernel are also primarily working on the targets
> listed in this file.

Done, actually gone :-) - I have no need to create one anymore.

https://codereview.chromium.org/2832353002/diff/80001/tools/patch_sdk.dart
File tools/patch_sdk.dart (right):

https://codereview.chromium.org/2832353002/diff/80001/tools/patch_sdk.dart#ne...
tools/patch_sdk.dart:55: void usage(String mode) {
On 2017/04/27 14:07:23, Vyacheslav Egorov (Google) wrote:
> mode is not used in the body of this function

oops. Done.

https://codereview.chromium.org/2832353002/diff/80001/tools/patch_sdk.dart#ne...
tools/patch_sdk.dart:81: Future _vmMain(List<String> argv) async {
On 2017/04/27 14:07:23, Vyacheslav Egorov (Google) wrote:
> vmMain and dart2jsMain have very similar prefix and suffix. We should consider
> splitting as much shared parts out as possible.

Done.

https://codereview.chromium.org/2832353002/diff/80001/tools/patch_sdk.dart#ne...
tools/patch_sdk.dart:84: if (argv.length != 5 || argv.first != 'vm') {
On 2017/04/27 14:07:23, Vyacheslav Egorov (Google) wrote:
> argv.first != 'vm' can't happen anymore. 

Done.

https://codereview.chromium.org/2832353002/diff/80001/utils/compiler/BUILD.gn
File utils/compiler/BUILD.gn (right):

https://codereview.chromium.org/2832353002/diff/80001/utils/compiler/BUILD.gn...
utils/compiler/BUILD.gn:80: action("patched_dart2js_sdk") {
On 2017/04/27 14:07:23, Vyacheslav Egorov (Google) wrote:
> It might be a good idea to move all shared GN rules for creating patched sdk
> into a template:
> 
> template("patched_sdk") {
>   // ...
> }
> 
> then use it here and in the VM BUILD.gn:
> 
> patched_sdk("patched_dart2js_sdk") {
>   deps = [...] 
>   patches_dir = "..."
>   patched_sdk_dir = "..."
> }

Done.

[ahe, 2017-05-01 12:52:29]

Please wait for Slava regarding patch_sdk.dart and the GN changes.

Everything else, LGTM!

[Vyacheslav Egorov (Google), 2017-05-01 13:50:45]

lgtm

[Siggi Cherem (dart-lang), 2017-05-01 17:11:48]

Description was changed from

==========
Add support for building patched_sdk and platform.dill for dart2js:

 - defines a .gn target for patched_dart2js_sdk
 - changes patch_sdk.dart to handle a dart2js target
 - adds support in fasta to understand the `native` clause still used by dart2js
 - add option to fasta to know whether it is generating kernel for the vm or
dart2js

I'm not excited about how I added the target distinction in fasta, the reason I
did so is that it affects the behavior in several ways: in how we handle
`native`, in the libraries that are included by default in the platform.dill
file,
and in the transformations that need to be done.  I'm happy to change the
approach.

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

to

==========
Add support for building patched_sdk and platform.dill for dart2js:

 - defines a .gn target for patched_dart2js_sdk
 - changes patch_sdk.dart to handle a dart2js target
 - adds support in fasta to understand the `native` clause still used by dart2js
 - add option to fasta to know whether it is generating kernel for the vm or
dart2js

I'm not excited about how I added the target distinction in fasta, the reason I
did so is that it affects the behavior in several ways: in how we handle
`native`, in the libraries that are included by default in the platform.dill
file,
and in the transformations that need to be done.  I'm happy to change the
approach.

BUG=
R=ahe@google.com, vegorov@google.com

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

[Siggi Cherem (dart-lang), 2017-05-01 17:11:49]

Committed patchset #3 (id:200001) manually as
714523b103598d49295fa06b4ad13c6917c50399 (presubmit successful).

[zra, 2017-05-01 20:29:41]

zra@google.com changed reviewers:
+ zra@google.com

[zra, 2017-05-01 20:29:42]

https://codereview.chromium.org/2832353002/diff/200001/utils/generate_patch_s...
File utils/generate_patch_sdk.gni (right):

https://codereview.chromium.org/2832353002/diff/200001/utils/generate_patch_s...
utils/generate_patch_sdk.gni:32: deps +=
["$_dart_root/runtime/bin:dart_bootstrap($host_toolchain)"]
Unfortunately, GN appears to treat a file system absolute path to a target as
distinct from a source absolute path to a target, so for example:

/home/zra/dart/sdk/runtime/bin:dart_bootstrap

is different from

//runtime/bin:dart_bootstrap

Since GN thinks these are different, it tries to load up the BUILD.gn file for
//runtime/bin twice. It fails on colliding build arguments when loading up their
respective dependencies.

The solution is to do:

_dart_root = get_path_info("..", "abspath")

and saying rebase_path("$_dart_root/...") only where needed.

This mistake appears everywhere we're currently defining something like
_dart_root, but the issue is only triggered by this CL because this is the first
time _dart_root from rebase_path() is appearing explicitly in a deps list,
AFAICT.

[Siggi Cherem (dart-lang), 2017-05-01 20:39:39]

Patchset #4 (id:220001) has been deleted

[Siggi Cherem (dart-lang), 2017-05-01 20:53:41]

https://codereview.chromium.org/2832353002/diff/200001/utils/generate_patch_s...
File utils/generate_patch_sdk.gni (right):

https://codereview.chromium.org/2832353002/diff/200001/utils/generate_patch_s...
utils/generate_patch_sdk.gni:32: deps +=
["$_dart_root/runtime/bin:dart_bootstrap($host_toolchain)"]
On 2017/05/01 20:29:42, zra wrote:
> Unfortunately, GN appears to treat a file system absolute path to a target as
> distinct from a source absolute path to a target, so for example:
> 
> /home/zra/dart/sdk/runtime/bin:dart_bootstrap
> 
> is different from
> 
> //runtime/bin:dart_bootstrap
> 
> Since GN thinks these are different, it tries to load up the BUILD.gn file for
> //runtime/bin twice. It fails on colliding build arguments when loading up
their
> respective dependencies.
> 
> The solution is to do:
> 
> _dart_root = get_path_info("..", "abspath")
> 
> and saying rebase_path("$_dart_root/...") only where needed.
> 
> This mistake appears everywhere we're currently defining something like
> _dart_root, but the issue is only triggered by this CL because this is the
first
> time _dart_root from rebase_path() is appearing explicitly in a deps list,
> AFAICT.

Thanks so much for looking into this. I just updated a new patch following your
suggestion.

Turns out that everywhere I used _dart_root it was sufficient to use it without
rebase_path

[zra, 2017-05-01 21:58:13]

lgtm

I've patched this in on Windows and into the Fuchsia build,
and the build no longer fails.

[Siggi Cherem (dart-lang), 2017-05-02 00:23:10]

Description was changed from

==========
Add support for building patched_sdk and platform.dill for dart2js:

 - defines a .gn target for patched_dart2js_sdk
 - changes patch_sdk.dart to handle a dart2js target
 - adds support in fasta to understand the `native` clause still used by dart2js
 - add option to fasta to know whether it is generating kernel for the vm or
dart2js

I'm not excited about how I added the target distinction in fasta, the reason I
did so is that it affects the behavior in several ways: in how we handle
`native`, in the libraries that are included by default in the platform.dill
file,
and in the transformations that need to be done.  I'm happy to change the
approach.

BUG=
R=ahe@google.com, vegorov@google.com

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

to

==========
Add support for building patched_sdk and platform.dill for dart2js:

 - defines a .gn target for patched_dart2js_sdk
 - changes patch_sdk.dart to handle a dart2js target
 - adds support in fasta to understand the `native` clause still used by dart2js
 - add option to fasta to know whether it is generating kernel for the vm or
dart2js

I'm not excited about how I added the target distinction in fasta, the reason I
did so is that it affects the behavior in several ways: in how we handle
`native`, in the libraries that are included by default in the platform.dill
file,
and in the transformations that need to be done.  I'm happy to change the
approach.

BUG=
R=ahe@google.com, vegorov@google.com, zra@google.com

Committed:
https://github.com/dart-lang/sdk/commit/714523b103598d49295fa06b4ad13c6917c50399
Committed:
https://github.com/dart-lang/sdk/commit/de36d118175ddf8de915a47b9b13d7c64d1c39c8
==========

[Siggi Cherem (dart-lang), 2017-05-02 00:23:11]

Committed patchset #4 (id:240001) manually as
de36d118175ddf8de915a47b9b13d7c64d1c39c8 (presubmit successful).

[Siggi Cherem (dart-lang), 2017-05-02 00:27:53]

On 2017/05/01 21:58:13, zra wrote:
> lgtm
> 
> I've patched this in on Windows and into the Fuchsia build,
> and the build no longer fails.

awesome, thanks for checking!

[ahe, 2017-05-02 06:35:53]

On 2017/05/02 00:27:53, Siggi Cherem (dart-lang) wrote:
> On 2017/05/01 21:58:13, zra wrote:
> > lgtm
> > 
> > I've patched this in on Windows and into the Fuchsia build,
> > and the build no longer fails.
> 
> awesome, thanks for checking!

Thank you for helping out, Zach!