Dart: Adds a program to create snapshots of Mojo apps.

Dart: Adds a program to create snapshots of Mojo apps.

BUG=
R=johnmccutchan@google.com

Committed: https://chromium.googlesource.com/external/mojo/+/3da94c7529a35c612adc1695c728be7a5b6f260e

[zra, 2015-08-10 20:23:29]

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

[zra, 2015-08-10 20:23:29]

This imports and adapts Sky's snapshotting program.

[Cutch, 2015-08-10 20:45:02]

https://codereview.chromium.org/1273743005/diff/1/examples/dart/hello_world/h...
File examples/dart/hello_world/hello/pubspec.yaml (right):

https://codereview.chromium.org/1273743005/diff/1/examples/dart/hello_world/h...
examples/dart/hello_world/hello/pubspec.yaml:1: name: hello
Should we give these packages a prefix or suffix to avoid colliding with real
packages?

https://codereview.chromium.org/1273743005/diff/1/mojo/dart/dart_snapshotter/...
File mojo/dart/dart_snapshotter/loader.cc (right):

https://codereview.chromium.org/1273743005/diff/1/mojo/dart/dart_snapshotter/...
mojo/dart/dart_snapshotter/loader.cc:26: //if (path.IsAbsolute()) return path;
Un-comment or remove before committing

https://codereview.chromium.org/1273743005/diff/1/mojo/dart/dart_snapshotter/...
mojo/dart/dart_snapshotter/loader.cc:113: Dart_Handle
HandleLibraryTag(Dart_LibraryTag tag,
Why can't you just use:

tonic::DartLibraryLoader::HandleLibraryTag and tonic::DartLibraryProviderFiles ?

https://codereview.chromium.org/1273743005/diff/1/mojo/dart/dart_snapshotter/...
File mojo/dart/dart_snapshotter/logging.h (right):

https://codereview.chromium.org/1273743005/diff/1/mojo/dart/dart_snapshotter/...
mojo/dart/dart_snapshotter/logging.h:12: bool LogIfError(Dart_Handle handle);
//tonic/dart_error.h
//tonic/dart_converter.h

https://codereview.chromium.org/1273743005/diff/1/mojo/dart/dart_snapshotter/...
File mojo/dart/dart_snapshotter/scope.h (right):

https://codereview.chromium.org/1273743005/diff/1/mojo/dart/dart_snapshotter/...
mojo/dart/dart_snapshotter/scope.h:18: ~DartIsolateScope() { Dart_ExitIsolate();
}
Use //tonic/dart_api_scope.h and //tonic/dart_isolate_scope.h

[zra, 2015-08-10 22:59:22]

https://codereview.chromium.org/1273743005/diff/1/examples/dart/hello_world/h...
File examples/dart/hello_world/hello/pubspec.yaml (right):

https://codereview.chromium.org/1273743005/diff/1/examples/dart/hello_world/h...
examples/dart/hello_world/hello/pubspec.yaml:1: name: hello
On 2015/08/10 20:45:02, Cutch wrote:
> Should we give these packages a prefix or suffix to avoid colliding with real
> packages?

Changed this one to mojo_dart_hello. Will change others as I touch them in
subsequent CLs.

https://codereview.chromium.org/1273743005/diff/1/mojo/dart/dart_snapshotter/...
File mojo/dart/dart_snapshotter/loader.cc (right):

https://codereview.chromium.org/1273743005/diff/1/mojo/dart/dart_snapshotter/...
mojo/dart/dart_snapshotter/loader.cc:26: //if (path.IsAbsolute()) return path;
On 2015/08/10 20:45:02, Cutch wrote:
> Un-comment or remove before committing 

Done.

https://codereview.chromium.org/1273743005/diff/1/mojo/dart/dart_snapshotter/...
mojo/dart/dart_snapshotter/loader.cc:113: Dart_Handle
HandleLibraryTag(Dart_LibraryTag tag,
On 2015/08/10 20:45:02, Cutch wrote:
> Why can't you just use:
> 
> tonic::DartLibraryLoader::HandleLibraryTag and tonic::DartLibraryProviderFiles
?

I tried doing this. I'm not sure it's a good trade-off for the added complexity
of setting up the Mojo machinery. I'd like to leave this as-is.

https://codereview.chromium.org/1273743005/diff/1/mojo/dart/dart_snapshotter/...
File mojo/dart/dart_snapshotter/logging.h (right):

https://codereview.chromium.org/1273743005/diff/1/mojo/dart/dart_snapshotter/...
mojo/dart/dart_snapshotter/logging.h:12: bool LogIfError(Dart_Handle handle);
On 2015/08/10 20:45:02, Cutch wrote:
> //tonic/dart_error.h
> //tonic/dart_converter.h

Done.

https://codereview.chromium.org/1273743005/diff/1/mojo/dart/dart_snapshotter/...
File mojo/dart/dart_snapshotter/scope.h (right):

https://codereview.chromium.org/1273743005/diff/1/mojo/dart/dart_snapshotter/...
mojo/dart/dart_snapshotter/scope.h:18: ~DartIsolateScope() { Dart_ExitIsolate();
}
On 2015/08/10 20:45:02, Cutch wrote:
> Use //tonic/dart_api_scope.h and //tonic/dart_isolate_scope.h

Done.

[Cutch, 2015-08-11 14:18:06]

https://codereview.chromium.org/1273743005/diff/20002/examples/dart/hello_wor...
File examples/dart/hello_world/hello/BUILD.gn (right):

https://codereview.chromium.org/1273743005/diff/20002/examples/dart/hello_wor...
examples/dart/hello_world/hello/BUILD.gn:18: entrypoints = [ "main.dart" ]
I don't see how this is used?

https://codereview.chromium.org/1273743005/diff/20002/mojo/dart/dart_snapshot...
File mojo/dart/dart_snapshotter/switches.h (right):

https://codereview.chromium.org/1273743005/diff/20002/mojo/dart/dart_snapshot...
mojo/dart/dart_snapshotter/switches.h:10: extern const char kHelp[];
Why a separate file and namespace for these constant definitions? Can't these
just live at the top of main.cc?

https://codereview.chromium.org/1273743005/diff/20002/mojo/dart/dart_snapshot...
File mojo/dart/dart_snapshotter/test/dart_snapshotter_test.py (right):

https://codereview.chromium.org/1273743005/diff/20002/mojo/dart/dart_snapshot...
mojo/dart/dart_snapshotter/test/dart_snapshotter_test.py:36: if not
os.path.isfile(snapshot):
Is there at least a magic number at the front of the snapshot file you could
verify?

https://codereview.chromium.org/1273743005/diff/20002/mojo/dart/dart_snapshot...
File mojo/dart/dart_snapshotter/vm.cc (right):

https://codereview.chromium.org/1273743005/diff/20002/mojo/dart/dart_snapshot...
mojo/dart/dart_snapshotter/vm.cc:33: Dart_Isolate isolate =
Dart_CreateIsolate("dart:snapshot", "main",
Does this "dart:" url make it into the program that loads the snapshot? Should
we use a different url just in case?

[zra, 2015-08-11 17:03:22]

https://codereview.chromium.org/1273743005/diff/20002/examples/dart/hello_wor...
File examples/dart/hello_world/hello/BUILD.gn (right):

https://codereview.chromium.org/1273743005/diff/20002/examples/dart/hello_wor...
examples/dart/hello_world/hello/BUILD.gn:18: entrypoints = [ "main.dart" ]
On 2015/08/11 14:18:06, Cutch wrote:
> I don't see how this is used?

Used by dart_pkg rule for invoking the analyzer, but this package is created
only to test the snapshotter for now.

https://codereview.chromium.org/1273743005/diff/20002/mojo/dart/dart_snapshot...
File mojo/dart/dart_snapshotter/switches.h (right):

https://codereview.chromium.org/1273743005/diff/20002/mojo/dart/dart_snapshot...
mojo/dart/dart_snapshotter/switches.h:10: extern const char kHelp[];
On 2015/08/11 14:18:06, Cutch wrote:
> Why a separate file and namespace for these constant definitions? Can't these
> just live at the top of main.cc?

Done.

https://codereview.chromium.org/1273743005/diff/20002/mojo/dart/dart_snapshot...
File mojo/dart/dart_snapshotter/test/dart_snapshotter_test.py (right):

https://codereview.chromium.org/1273743005/diff/20002/mojo/dart/dart_snapshot...
mojo/dart/dart_snapshotter/test/dart_snapshotter_test.py:36: if not
os.path.isfile(snapshot):
On 2015/08/11 14:18:06, Cutch wrote:
> Is there at least a magic number at the front of the snapshot file you could
> verify?

Done. Not sure how robust it is, but this test will only be relevant until the
content handler is loading snapshots.

https://codereview.chromium.org/1273743005/diff/20002/mojo/dart/dart_snapshot...
File mojo/dart/dart_snapshotter/vm.cc (right):

https://codereview.chromium.org/1273743005/diff/20002/mojo/dart/dart_snapshot...
mojo/dart/dart_snapshotter/vm.cc:33: Dart_Isolate isolate =
Dart_CreateIsolate("dart:snapshot", "main",
On 2015/08/11 14:18:06, Cutch wrote:
> Does this "dart:" url make it into the program that loads the snapshot? Should
> we use a different url just in case?

This string is only used for debug messages, and doesn't make it into the
snapshot. I believe someone would only see this when trying to snapshot a script
with a syntax error, but the dart_pkg rule will fail in the analysis step before
attempting to make a snapshot, so this will never be visible.

[Cutch, 2015-08-11 17:06:36]

lgtm

[zra, 2015-08-11 17:27:00]

Committed patchset #5 (id:50017) manually as
3da94c7529a35c612adc1695c728be7a5b6f260e (presubmit successful).

[jamesr, 2015-08-17 20:44:51]

jamesr@chromium.org changed reviewers:
+ jamesr@chromium.org

[jamesr, 2015-08-17 20:44:52]

ToT GN is unhappy about the dependencies on dart_snapshotter:

Running gn 'gen' '--check' 'out/android_Debug' '--args=is_clang=false
use_goma=true target_cpu="arm" dcheck_always_on=false
go_build_tool="/ssd/mojo/src/third_party/go/tool/android_arm/bin/go"
is_asan=false is_debug=true goma_dir="/usr/local/google/home/jamesr/goma"
is_official_build=false mojo_use_go=true target_os="android"' ...
ERROR Input to targets not generated by a dependency.
The file:
  //out/android_Debug/clang_x64/dart_snapshotter
is listed as an input or source for the targets:
 
//examples/dart/hello_world/world:gen_mojo_dart_world_pkg_main_dartx_snapshot(//build/toolchain/android:arm)
 
//examples/dart/hello_world/hello:gen_mojo_dart_hello_pkg_main_dartx_snapshot(//build/toolchain/android:arm)
......

but this file was not generated by any dependencies of the targets. The target
that generates the file is:
 
//mojo/dart/dart_snapshotter:dart_snapshotter(//build/toolchain/linux:clang_x64)

If you have generated inputs, there needs to be a dependency path between the
two targets in addition to just listing the files. For indirect dependencies,
the intermediate ones must be public_deps. data_deps don't count since they're
only runtime dependencies. If you think a dependency chain exists, it might be
because the chain is private. Try "gn path" to analyze.

Not sure how to resolve this in an SDK-clean fashion