Upload/Download dart_snapshotter

Upload/Download dart_snapshotter

BUG=
R=jamesr@chromium.org

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

[zra, 2015-08-14 22:47:21]

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

[jamesr, 2015-08-14 22:55:01]

jamesr@chromium.org changed reviewers:
+ jamesr@chromium.org
- jamesr@google.com

[jamesr, 2015-08-14 22:55:02]

https://codereview.chromium.org/1300463002/diff/40001/mojo/public/tools/BUILD.gn
File mojo/public/tools/BUILD.gn (right):

https://codereview.chromium.org/1300463002/diff/40001/mojo/public/tools/BUILD...
mojo/public/tools/BUILD.gn:34:
"prebuilt/dart_snapshotter/linux-x64/dart_snapshotter",
this doesn't look like the right binary to use when developing on a mac host

https://codereview.chromium.org/1300463002/diff/40001/mojo/public/tools/BUILD...
mojo/public/tools/BUILD.gn:36: if (is_android) {
do you want to test for "is_android" here or "am i cross compiling"? do you need
the snapshotter for the host or target build?

https://codereview.chromium.org/1300463002/diff/40001/mojo/public/tools/BUILD...
mojo/public/tools/BUILD.gn:39: "$root_out_dir/$toolchain_name/dart_snapshotter",
hm, doesn't root_out_dir already contain the current toolchain?

[zra, 2015-08-17 15:06:16]

Thought about this a bit more over the weekend. I think there might be a place I
can put a $host_toolchain annotation that will help clean this up a bit.

https://codereview.chromium.org/1300463002/diff/40001/mojo/public/tools/BUILD.gn
File mojo/public/tools/BUILD.gn (right):

https://codereview.chromium.org/1300463002/diff/40001/mojo/public/tools/BUILD...
mojo/public/tools/BUILD.gn:34:
"prebuilt/dart_snapshotter/linux-x64/dart_snapshotter",
On 2015/08/14 22:55:02, jamesr wrote:
> this doesn't look like the right binary to use when developing on a mac host

I didn't see a Mac builder for Mojo. Is there one?

https://codereview.chromium.org/1300463002/diff/40001/mojo/public/tools/BUILD...
mojo/public/tools/BUILD.gn:36: if (is_android) {
On 2015/08/14 22:55:01, jamesr wrote:
> do you want to test for "is_android" here or "am i cross compiling"? do you
need
> the snapshotter for the host or target build?

Right now in this CL, the snapshotter is invoked during the target build. Since
the Dart .mojo apps are platform agnostic, it could just as well be invoked for
the host build. I'm going to give that a try and see if it will help clean this
up.

https://codereview.chromium.org/1300463002/diff/40001/mojo/public/tools/BUILD...
mojo/public/tools/BUILD.gn:39: "$root_out_dir/$toolchain_name/dart_snapshotter",
On 2015/08/14 22:55:01, jamesr wrote:
> hm, doesn't root_out_dir already contain the current toolchain?

If the current toolchain is the target toolchain, root_out_dir is e.g.
//out/android_Debug/. If the current toolchain is something else then
root_out_dir is //out/android_debug/$toolchain_name/

[jamesr, 2015-08-17 17:29:40]

https://codereview.chromium.org/1300463002/diff/40001/mojo/public/tools/BUILD.gn
File mojo/public/tools/BUILD.gn (right):

https://codereview.chromium.org/1300463002/diff/40001/mojo/public/tools/BUILD...
mojo/public/tools/BUILD.gn:34:
"prebuilt/dart_snapshotter/linux-x64/dart_snapshotter",
On 2015/08/17 at 15:06:16, zra wrote:
> On 2015/08/14 22:55:02, jamesr wrote:
> > this doesn't look like the right binary to use when developing on a mac host
> 
> I didn't see a Mac builder for Mojo. Is there one?

There is not, but developing from a Mac host is supported (on a best-effort
basis).  We provide linux and mac host binaries for other blobs.

https://codereview.chromium.org/1300463002/diff/40001/mojo/public/tools/BUILD...
mojo/public/tools/BUILD.gn:36: if (is_android) {
On 2015/08/17 at 15:06:16, zra wrote:
> On 2015/08/14 22:55:01, jamesr wrote:
> > do you want to test for "is_android" here or "am i cross compiling"? do you
need
> > the snapshotter for the host or target build?
> 
> Right now in this CL, the snapshotter is invoked during the target build.
Since the Dart .mojo apps are platform agnostic, it could just as well be
invoked for the host build. I'm going to give that a try and see if it will help
clean this up.

I think at a minimum you shouldn't need to invoke the copy step for both the
host and target toolchain.

You could have the copy step declare a data dependency on a copy that runs in
the host toolchain, depending on if that makes more sense (imo it would since in
the non-prebuilt state the snapshotter has to be built in the host toolchain)

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

https://codereview.chromium.org/1300463002/diff/100001/mojo/public/dart/rules...
File mojo/public/dart/rules.gni (right):

https://codereview.chromium.org/1300463002/diff/100001/mojo/public/dart/rules...
mojo/public/dart/rules.gni:26: rebase_path("mojo/dart/dart_snapshotter", ".",
mojo_root)
the sdk should not depend on paths outside the sdk. if you want to allow
specifying a label in the host environment (i.e. the mojo checkout) make that an
optional parameter in //build/module_args/

https://codereview.chromium.org/1300463002/diff/100001/mojo/public/dart/rules...
mojo/public/dart/rules.gni:28: data_deps = [
"$dart_snapshotter_rule($host_toolchain)" ]
data_deps is wrong since you need this to exist before you can start running the
action. use deps

[zra, 2015-08-17 21:59:18]

https://codereview.chromium.org/1300463002/diff/40001/mojo/public/tools/BUILD.gn
File mojo/public/tools/BUILD.gn (right):

https://codereview.chromium.org/1300463002/diff/40001/mojo/public/tools/BUILD...
mojo/public/tools/BUILD.gn:34:
"prebuilt/dart_snapshotter/linux-x64/dart_snapshotter",
On 2015/08/17 17:29:39, jamesr wrote:
> On 2015/08/17 at 15:06:16, zra wrote:
> > On 2015/08/14 22:55:02, jamesr wrote:
> > > this doesn't look like the right binary to use when developing on a mac
host
> > 
> > I didn't see a Mac builder for Mojo. Is there one?
> 
> There is not, but developing from a Mac host is supported (on a best-effort
> basis).  We provide linux and mac host binaries for other blobs.

I've modified here and the upload_binaries.py script to do what I think might be
the right thing, but I think I don't understand how the mac host binaries are
provided if they aren't uploaded by a mac builder somewhere.

https://codereview.chromium.org/1300463002/diff/40001/mojo/public/tools/BUILD...
mojo/public/tools/BUILD.gn:36: if (is_android) {
On 2015/08/17 17:29:39, jamesr wrote:
> On 2015/08/17 at 15:06:16, zra wrote:
> > On 2015/08/14 22:55:01, jamesr wrote:
> > > do you want to test for "is_android" here or "am i cross compiling"? do
you
> need
> > > the snapshotter for the host or target build?
> > 
> > Right now in this CL, the snapshotter is invoked during the target build.
> Since the Dart .mojo apps are platform agnostic, it could just as well be
> invoked for the host build. I'm going to give that a try and see if it will
help
> clean this up.
> 
> I think at a minimum you shouldn't need to invoke the copy step for both the
> host and target toolchain.
> 
> You could have the copy step declare a data dependency on a copy that runs in
> the host toolchain, depending on if that makes more sense (imo it would since
in
> the non-prebuilt state the snapshotter has to be built in the host toolchain)

Done.

https://codereview.chromium.org/1300463002/diff/100001/mojo/public/dart/rules...
File mojo/public/dart/rules.gni (right):

https://codereview.chromium.org/1300463002/diff/100001/mojo/public/dart/rules...
mojo/public/dart/rules.gni:26: rebase_path("mojo/dart/dart_snapshotter", ".",
mojo_root)
On 2015/08/17 20:59:51, jamesr wrote:
> the sdk should not depend on paths outside the sdk. if you want to allow
> specifying a label in the host environment (i.e. the mojo checkout) make that
an
> optional parameter in //build/module_args/

Used the variable you added.

https://codereview.chromium.org/1300463002/diff/100001/mojo/public/dart/rules...
mojo/public/dart/rules.gni:28: data_deps = [
"$dart_snapshotter_rule($host_toolchain)" ]
On 2015/08/17 20:59:51, jamesr wrote:
> data_deps is wrong since you need this to exist before you can start running
the
> action. use deps

Fixed after your change.

[jamesr, 2015-08-17 22:03:02]

On 2015/08/17 at 21:59:18, zra wrote:
>
https://codereview.chromium.org/1300463002/diff/40001/mojo/public/tools/BUILD.gn
> File mojo/public/tools/BUILD.gn (right):
> 
>
https://codereview.chromium.org/1300463002/diff/40001/mojo/public/tools/BUILD...
> mojo/public/tools/BUILD.gn:34:
"prebuilt/dart_snapshotter/linux-x64/dart_snapshotter",
> On 2015/08/17 17:29:39, jamesr wrote:
> > On 2015/08/17 at 15:06:16, zra wrote:
> > > On 2015/08/14 22:55:02, jamesr wrote:
> > > > this doesn't look like the right binary to use when developing on a mac
host
> > > 
> > > I didn't see a Mac builder for Mojo. Is there one?
> > 
> > There is not, but developing from a Mac host is supported (on a best-effort
> > basis).  We provide linux and mac host binaries for other blobs.
> 
> I've modified here and the upload_binaries.py script to do what I think might
be the right thing, but I think I don't understand how the mac host binaries are
provided if they aren't uploaded by a mac builder somewhere.

They are produced by a developer on a mac and uploaded to google storage.

[jamesr, 2015-08-17 22:10:16]

https://codereview.chromium.org/1300463002/diff/140001/mojo/dart/BUILD.gn
File mojo/dart/BUILD.gn (right):

https://codereview.chromium.org/1300463002/diff/140001/mojo/dart/BUILD.gn#new...
mojo/dart/BUILD.gn:5: import("//mojo/public/mojo.gni")
what's this needed for?

https://codereview.chromium.org/1300463002/diff/140001/mojo/dart/BUILD.gn#new...
mojo/dart/BUILD.gn:15: if (!mojo_use_prebuilt_dart_snapshotter) {
this will never be true in //mojo/dart, since this is our own repo. i don't
think you want this to be conditional

https://codereview.chromium.org/1300463002/diff/140001/mojo/dart/dart_snapsho...
File mojo/dart/dart_snapshotter/BUILD.gn (right):

https://codereview.chromium.org/1300463002/diff/140001/mojo/dart/dart_snapsho...
mojo/dart/dart_snapshotter/BUILD.gn:7: if (!mojo_use_prebuilt_dart_snapshotter)
{
this doesn't make sense. //mojo/dart/BUILD.gn is local to the mojo repository
and thus it'll never be used with a different //build/module_args/...

https://codereview.chromium.org/1300463002/diff/140001/mojo/public/mojo.gni
File mojo/public/mojo.gni (right):

https://codereview.chromium.org/1300463002/diff/140001/mojo/public/mojo.gni#n...
mojo/public/mojo.gni:20: mojo_use_prebuilt_dart_snapshotter = host_os == "linux"
|| host_os == "mac"
what is a hypothetical developer on a different host_os supposed to do?  it
doesn't seem like they have any real choices - even if our prebuilts don't work
they certainly won't be able to build one themselves (i.e. the label won't
exist). i think what would make more sense here is to assert that the
module_args either set use_prebuilt_dart_snapshotter or specify a label for the
snapshot binary and not have branches on host_os

https://codereview.chromium.org/1300463002/diff/140001/mojo/tools/mopy/paths.py
File mojo/tools/mopy/paths.py (right):

https://codereview.chromium.org/1300463002/diff/140001/mojo/tools/mopy/paths....
mojo/tools/mopy/paths.py:45: self.target_dart_snapshotter_path = None
this doesn't seem quite right - if building for android the snapshotter path
should be the one for the host os, right?

https://codereview.chromium.org/1300463002/diff/140001/mojo/tools/upload_bina...
File mojo/tools/upload_binaries.py (right):

https://codereview.chromium.org/1300463002/diff/140001/mojo/tools/upload_bina...
mojo/tools/upload_binaries.py:167: # TODO(blundell): Change this to be in the
same structure as the LATEST files,
is colin going to do something about this TODO? what's the action item here?

[zra, 2015-08-17 22:51:28]

https://codereview.chromium.org/1300463002/diff/140001/mojo/dart/BUILD.gn
File mojo/dart/BUILD.gn (right):

https://codereview.chromium.org/1300463002/diff/140001/mojo/dart/BUILD.gn#new...
mojo/dart/BUILD.gn:5: import("//mojo/public/mojo.gni")
On 2015/08/17 22:10:15, jamesr wrote:
> what's this needed for?

It was for mojo_use_prebuilt_dart_snapshotter. Removed.

https://codereview.chromium.org/1300463002/diff/140001/mojo/dart/BUILD.gn#new...
mojo/dart/BUILD.gn:15: if (!mojo_use_prebuilt_dart_snapshotter) {
On 2015/08/17 22:10:16, jamesr wrote:
> this will never be true in //mojo/dart, since this is our own repo. i don't
> think you want this to be conditional

Ah, yah. I was just locally testing the rules for copying and using the prebuilt
binary. I can remove these now.

https://codereview.chromium.org/1300463002/diff/140001/mojo/dart/dart_snapsho...
File mojo/dart/dart_snapshotter/BUILD.gn (right):

https://codereview.chromium.org/1300463002/diff/140001/mojo/dart/dart_snapsho...
mojo/dart/dart_snapshotter/BUILD.gn:7: if (!mojo_use_prebuilt_dart_snapshotter)
{
On 2015/08/17 22:10:16, jamesr wrote:
> this doesn't make sense. //mojo/dart/BUILD.gn is local to the mojo repository
> and thus it'll never be used with a different //build/module_args/...

Removed.

https://codereview.chromium.org/1300463002/diff/140001/mojo/public/mojo.gni
File mojo/public/mojo.gni (right):

https://codereview.chromium.org/1300463002/diff/140001/mojo/public/mojo.gni#n...
mojo/public/mojo.gni:20: mojo_use_prebuilt_dart_snapshotter = host_os == "linux"
|| host_os == "mac"
On 2015/08/17 22:10:16, jamesr wrote:
> what is a hypothetical developer on a different host_os supposed to do?  it
> doesn't seem like they have any real choices - even if our prebuilts don't
work
> they certainly won't be able to build one themselves (i.e. the label won't
> exist). i think what would make more sense here is to assert that the
> module_args either set use_prebuilt_dart_snapshotter or specify a label for
the
> snapshot binary and not have branches on host_os

Added assert that dart_snapshotter_bin is defined when we decide that
mojo_use_prebuilt_dart_snapshotter is false, which is when
mojo_build_dart_snapshotter_from_source is undefined or true.

https://codereview.chromium.org/1300463002/diff/140001/mojo/tools/mopy/paths.py
File mojo/tools/mopy/paths.py (right):

https://codereview.chromium.org/1300463002/diff/140001/mojo/tools/mopy/paths....
mojo/tools/mopy/paths.py:45: self.target_dart_snapshotter_path = None
On 2015/08/17 22:10:16, jamesr wrote:
> this doesn't seem quite right - if building for android the snapshotter path
> should be the one for the host os, right?

For an Android build, self.dart_snapshotter_path is the path to the dart
snapshotter to run on the host. Since there is no dart_snapshotter to run on the
Android target, I thought None would make sense. I've added a comment.

https://codereview.chromium.org/1300463002/diff/140001/mojo/tools/upload_bina...
File mojo/tools/upload_binaries.py (right):

https://codereview.chromium.org/1300463002/diff/140001/mojo/tools/upload_bina...
mojo/tools/upload_binaries.py:167: # TODO(blundell): Change this to be in the
same structure as the LATEST files,
On 2015/08/17 22:10:16, jamesr wrote:
> is colin going to do something about this TODO? what's the action item here?

Copied from above. I'm not sure what this is for, so I'll leave the one above,
and remove this one.

[jamesr, 2015-08-17 23:08:49]

lgtm

https://codereview.chromium.org/1300463002/diff/180001/mojo/public/tools/BUIL...
File mojo/public/tools/BUILD.gn (right):

https://codereview.chromium.org/1300463002/diff/180001/mojo/public/tools/BUIL...
mojo/public/tools/BUILD.gn:36: assert(host_os == "mac")
i would do this

} else if (host_os == "mac") {
  ..
} else {
  assert(false, "host_os not supported ...")
}

so the message can be more informative

[zra, 2015-08-18 15:49:30]

Committed patchset #11 (id:200001) manually as
3dcf5823716c35e8c2c19a471ed7927685f98a6e (presubmit successful).

[zra, 2015-08-18 15:49:50]

Thanks!

https://codereview.chromium.org/1300463002/diff/180001/mojo/public/tools/BUIL...
File mojo/public/tools/BUILD.gn (right):

https://codereview.chromium.org/1300463002/diff/180001/mojo/public/tools/BUIL...
mojo/public/tools/BUILD.gn:36: assert(host_os == "mac")
On 2015/08/17 23:08:49, jamesr wrote:
> i would do this
> 
> } else if (host_os == "mac") {
>   ..
> } else {
>   assert(false, "host_os not supported ...")
> }
> 
> so the message can be more informative

Done.