Expose ability to download network service impl in SDK.

Expose ability to download network service impl in SDK.

Consumers of the Mojo SDK need to be able obtain the impl of the network
service that is associated with the version of the SDK. This CL moves infra for
downloading the network service under the SDK. It gates the usage of the
prebuilt network service via a variable in //build/module_args/mojo.gni,
similarly to how the usage of the prebuilt shell is gated.

Concretely, this CL does the following:
- Moves download_network_service.py to live in the SDK
- Changes the structure of //mojo/public/tools/prebuilt to have a subdir
  for each downloaded artifact, with platform-specific subdirs underneath each
  artifact's subdir
- Moves the definition of the copy_network_service target into
  //mojo/public/tools/BUILD.gn and changes the structure of dependencies on
  that target to be analogous to the structure of dependencies on the
  copy_mojo_shell target

R=jamesr@chromium.org, ppi@chromium.org

Committed: https://chromium.googlesource.com/external/mojo/+/6966625cbb4c3b9392f07527c6177486de3ae7d7

[blundell, 2015-02-10 14:37:22]

blundell@chromium.org changed reviewers:
+ ppi@chromium.org

[blundell, 2015-02-10 14:38:40]

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

[ppi, 2015-02-10 16:49:50]

Overall looks right to me, please find some comments below.

https://codereview.chromium.org/910763002/diff/80001/build/module_args/mojo.gni
File build/module_args/mojo.gni (right):

https://codereview.chromium.org/910763002/diff/80001/build/module_args/mojo.g...
build/module_args/mojo.gni:10: # To use the prebuilt network service, set this
variable to true or simply omit
Should both comments go the other way, ie. "By default a prebuilt network
service is used. To use a locally built network service set this variable to
false." ?

https://codereview.chromium.org/910763002/diff/80001/mojo/public/mojo.gni
File mojo/public/mojo.gni (right):

https://codereview.chromium.org/910763002/diff/80001/mojo/public/mojo.gni#new...
mojo/public/mojo.gni:16: # The prebuilt Mojo shell packages the network service
on Android.
nit: I think s/packages/contains/ would stress that the network binary is
actually within the shell. wdyt?

https://codereview.chromium.org/910763002/diff/80001/mojo/public/mojo.gni#new...
mojo/public/mojo.gni:18: is_linux || (is_android && !use_prebuilt_mojo_shell)
Is this part needed? On Android the choice to pull a prebuilt network service or
to build locally doesn't matter, and it doesn't matter symetrically for both
cases. Not sure why would we override the behavior of pulling down binaries
versus the local build one way or another.

Wdyt?

https://codereview.chromium.org/910763002/diff/80001/mojo/public/mojo_applica...
File mojo/public/mojo_application.gni (right):

https://codereview.chromium.org/910763002/diff/80001/mojo/public/mojo_applica...
mojo/public/mojo_application.gni:94: # Copy the prebuilt network service if
using it.
nit: tautology, maybe s/using it/needed/?

https://codereview.chromium.org/910763002/diff/80001/mojo/public/mojo_applica...
mojo/public/mojo_application.gni:96: if (defined(data_deps)) {
nit: We already have a similar block above, maybe declare data_deps = [] and
then append unconditionally here and above?

https://codereview.chromium.org/910763002/diff/80001/shell/BUILD.gn
File shell/BUILD.gn (right):

https://codereview.chromium.org/910763002/diff/80001/shell/BUILD.gn#newcode310
shell/BUILD.gn:310: if (use_prebuilt_network_service) {
Is this needed? I'd think that putting "$root_out_dir/network_service.mojo" in
sources would add the dependency. Or is it better to declare it explicitly?

[jamesr, 2015-02-10 22:45:21]

https://codereview.chromium.org/910763002/diff/80001/build/module_args/mojo.gni
File build/module_args/mojo.gni (right):

https://codereview.chromium.org/910763002/diff/80001/build/module_args/mojo.g...
build/module_args/mojo.gni:10: # To use the prebuilt network service, set this
variable to true or simply omit
On 2015/02/10 16:49:50, ppi wrote:
> Should both comments go the other way, ie. "By default a prebuilt network
> service is used. To use a locally built network service set this variable to
> false." ?

Yeah, I think the default should be prebuilt (here the mojo repo is the odd one
out)

https://codereview.chromium.org/910763002/diff/80001/shell/BUILD.gn
File shell/BUILD.gn (right):

https://codereview.chromium.org/910763002/diff/80001/shell/BUILD.gn#newcode310
shell/BUILD.gn:310: if (use_prebuilt_network_service) {
On 2015/02/10 16:49:50, ppi wrote:
> Is this needed? I'd think that putting "$root_out_dir/network_service.mojo" in
> sources would add the dependency. Or is it better to declare it explicitly?

This shouldn't be conditional other than as a transition state.  Building the
shell implies being in the mojo which implies that the network service always
must be prebuilt.  I also agree that this target should just declare a
dependency on $r_o_t/n_s.mojo and let the build system figure out how that file
gets there

[blundell, 2015-02-11 20:57:51]

Patchset #6 (id:100001) has been deleted

[blundell, 2015-02-11 21:06:17]

https://codereview.chromium.org/910763002/diff/80001/build/module_args/mojo.gni
File build/module_args/mojo.gni (right):

https://codereview.chromium.org/910763002/diff/80001/build/module_args/mojo.g...
build/module_args/mojo.gni:10: # To use the prebuilt network service, set this
variable to true or simply omit
On 2015/02/10 22:45:21, jamesr wrote:
> On 2015/02/10 16:49:50, ppi wrote:
> > Should both comments go the other way, ie. "By default a prebuilt network
> > service is used. To use a locally built network service set this variable to
> > false." ?
> 
> Yeah, I think the default should be prebuilt (here the mojo repo is the odd
one
> out)

Changed the comments. The code defaults to prebuilt if the variable isn't
defined. ppi@ offline commented that it's a little bizarre to have undefined
mean "true" instead of "false", and that it would be more logical to have a
variable "build_network_service_from_source" that is false if undefined. I'm
happy either way; if changing this var, I think it would make sense to make a
corresponding change to use_prebuilt_mojo_shell.

https://codereview.chromium.org/910763002/diff/80001/mojo/public/mojo.gni
File mojo/public/mojo.gni (right):

https://codereview.chromium.org/910763002/diff/80001/mojo/public/mojo.gni#new...
mojo/public/mojo.gni:16: # The prebuilt Mojo shell packages the network service
on Android.
On 2015/02/10 16:49:50, ppi wrote:
> nit: I think s/packages/contains/ would stress that the network binary is
> actually within the shell. wdyt?

Done.

https://codereview.chromium.org/910763002/diff/80001/mojo/public/mojo.gni#new...
mojo/public/mojo.gni:18: is_linux || (is_android && !use_prebuilt_mojo_shell)
On 2015/02/10 16:49:50, ppi wrote:
> Is this part needed? On Android the choice to pull a prebuilt network service
or
> to build locally doesn't matter, and it doesn't matter symetrically for both
> cases. Not sure why would we override the behavior of pulling down binaries
> versus the local build one way or another.
> 
> Wdyt?

My thought is that it's weird and a little confusing to pull a
network_service.mojo artifact into the build dir that's never used on Android. I
agree that it wouldn't actually cause any problems.

https://codereview.chromium.org/910763002/diff/80001/mojo/public/mojo_applica...
File mojo/public/mojo_application.gni (right):

https://codereview.chromium.org/910763002/diff/80001/mojo/public/mojo_applica...
mojo/public/mojo_application.gni:94: # Copy the prebuilt network service if
using it.
On 2015/02/10 16:49:50, ppi wrote:
> nit: tautology, maybe s/using it/needed/?

Done.

https://codereview.chromium.org/910763002/diff/80001/mojo/public/mojo_applica...
mojo/public/mojo_application.gni:96: if (defined(data_deps)) {
On 2015/02/10 16:49:50, ppi wrote:
> nit: We already have a similar block above, maybe declare data_deps = [] and
> then append unconditionally here and above?

Done.

https://codereview.chromium.org/910763002/diff/80001/shell/BUILD.gn
File shell/BUILD.gn (right):

https://codereview.chromium.org/910763002/diff/80001/shell/BUILD.gn#newcode310
shell/BUILD.gn:310: if (use_prebuilt_network_service) {
On 2015/02/10 22:45:21, jamesr wrote:
> On 2015/02/10 16:49:50, ppi wrote:
> > Is this needed? I'd think that putting "$root_out_dir/network_service.mojo"
in
> > sources would add the dependency. Or is it better to declare it explicitly?
> 
> This shouldn't be conditional other than as a transition state.  Building the
> shell implies being in the mojo which implies that the network service always
> must be prebuilt.  I also agree that this target should just declare a
> dependency on $r_o_t/n_s.mojo and let the build system figure out how that
file
> gets there

Done.

[jamesr, 2015-02-11 21:09:46]

lgtm

In the fullness of time we should get rid of the distinction between the SDK and
network service 'version' since the fact that the two are from different places
shouldn't matter to anyone outside of the mojo and chromium repos.  We'll also
want to have a more reasonable version naming scheme too.  But that's far down
the road.

[ppi, 2015-02-12 13:36:49]

https://codereview.chromium.org/910763002/diff/80001/build/module_args/mojo.gni
File build/module_args/mojo.gni (right):

https://codereview.chromium.org/910763002/diff/80001/build/module_args/mojo.g...
build/module_args/mojo.gni:10: # To use the prebuilt network service, set this
variable to true or simply omit
On 2015/02/11 21:06:17, blundell wrote:
> On 2015/02/10 22:45:21, jamesr wrote:
> > On 2015/02/10 16:49:50, ppi wrote:
> > > Should both comments go the other way, ie. "By default a prebuilt network
> > > service is used. To use a locally built network service set this variable
to
> > > false." ?
> > 
> > Yeah, I think the default should be prebuilt (here the mojo repo is the odd
> one
> > out)
> 
> Changed the comments. The code defaults to prebuilt if the variable isn't
> defined. ppi@ offline commented that it's a little bizarre to have undefined
> mean "true" instead of "false", and that it would be more logical to have a
> variable "build_network_service_from_source" that is false if undefined. I'm
> happy either way; if changing this var, I think it would make sense to make a
> corresponding change to use_prebuilt_mojo_shell.

Yeah, flipping both flags around sounds good to me, so does doing this in a
follow-up.

https://codereview.chromium.org/910763002/diff/80001/mojo/public/mojo.gni
File mojo/public/mojo.gni (right):

https://codereview.chromium.org/910763002/diff/80001/mojo/public/mojo.gni#new...
mojo/public/mojo.gni:18: is_linux || (is_android && !use_prebuilt_mojo_shell)
On 2015/02/11 21:06:17, blundell wrote:
> On 2015/02/10 16:49:50, ppi wrote:
> > Is this part needed? On Android the choice to pull a prebuilt network
service
> or
> > to build locally doesn't matter, and it doesn't matter symetrically for both
> > cases. Not sure why would we override the behavior of pulling down binaries
> > versus the local build one way or another.
> > 
> > Wdyt?
> 
> My thought is that it's weird and a little confusing to pull a
> network_service.mojo artifact into the build dir that's never used on Android.
I
> agree that it wouldn't actually cause any problems.

Right, but isn't building it locally when it's never used less weird? I would
err on the side of simplicity and drop the android condition here. Wdyt?

[blundell, 2015-02-12 13:55:01]

New patchsets have been uploaded after l-g-t-m from jamesr@chromium.org

[blundell, 2015-02-12 13:57:41]

https://codereview.chromium.org/910763002/diff/80001/mojo/public/mojo.gni
File mojo/public/mojo.gni (right):

https://codereview.chromium.org/910763002/diff/80001/mojo/public/mojo.gni#new...
mojo/public/mojo.gni:18: is_linux || (is_android && !use_prebuilt_mojo_shell)
On 2015/02/12 13:36:49, ppi wrote:
> On 2015/02/11 21:06:17, blundell wrote:
> > On 2015/02/10 16:49:50, ppi wrote:
> > > Is this part needed? On Android the choice to pull a prebuilt network
> service
> > or
> > > to build locally doesn't matter, and it doesn't matter symetrically for
both
> > > cases. Not sure why would we override the behavior of pulling down
binaries
> > > versus the local build one way or another.
> > > 
> > > Wdyt?
> > 
> > My thought is that it's weird and a little confusing to pull a
> > network_service.mojo artifact into the build dir that's never used on
Android.
> I
> > agree that it wouldn't actually cause any problems.
> 
> Right, but isn't building it locally when it's never used less weird? I would
> err on the side of simplicity and drop the android condition here. Wdyt?

Yes, you're right (I think you meant *more* weird). I hadn't thought about the
Chromium case, where are you actually *are* building the network service from
source if you're not using the prebuilt network service. In that case, it would
be totally unexpected for someone to set use_prebuilt_network_service to true
and then have the network service be built from source on Android anyway

[ppi, 2015-02-12 13:59:04]

lgtm, thanks!

[blundell, 2015-02-12 14:01:10]

New patchsets have been uploaded after l-g-t-m from ppi@chromium.org

[blundell, 2015-02-13 08:38:32]

Committed patchset #8 (id:160001) manually as
6966625cbb4c3b9392f07527c6177486de3ae7d7 (presubmit successful).