Add option to allow mojob to specify extra gn args.

As self_built_network_service_location introduced in
https://codereview.chromium.org/983143003/, this CL is adding the
support to allow mojob to specify this arg via command.

How to use (take x86 for eg):
============================
$ mojo/tools/mojob.py gn --android --target-cpu=x86 \
  --args="self_built_network_service_location=<x86_network_service_mojo_file>"
$ mojo/tools/mojob.py build --android

TEST=mojo/tools/android_mojo_shell.py mojo:sample_app
BUG=458409
R=qsr@chromium.org

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

[haltonhuo, 2015-02-13 04:39:45]

halton.huo@intel.com changed reviewers:
+ qsr@chromium.org

[haltonhuo, 2015-02-13 04:39:45]

PTAL

[haltonhuo, 2015-02-13 04:48:56]

On 2015/02/13 04:39:45, haltonhuo wrote:
> PTAL

https://codereview.chromium.org/917423004/ is prediction of this CL. Before
that, mojo:sample_app is verified on android x86 emulator.
src/mojo/services/network/prebuilt/android-x86/network_service.mojo is copied
from my local of chromium android x86 target.

[haltonhuo, 2015-02-13 08:05:16]

On 2015/02/13 04:48:56, haltonhuo wrote:
> On 2015/02/13 04:39:45, haltonhuo wrote:
> > PTAL
> 
> https://codereview.chromium.org/917423004/ is prediction of this CL. Before
> that, mojo:sample_app is verified on android x86 emulator.
> src/mojo/services/network/prebuilt/android-x86/network_service.mojo is copied
> from my local of chromium android x86 target.

And x64 is verified on emulator as well. By the time so far, the build and
mojo:sample_app for arm/x86/x64 are all working on my local. Hopefully, all the
CLs can be well landed so that I can focus some core part inside mojo.

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

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

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

ppi@ is actually the best reviewer here. I don't think these fixes are enough
fwiw, because we're not actually uploading the network service on the targets
that you're interested in.

[haltonhuo, 2015-02-13 09:09:02]

On 2015/02/13 08:32:57, blundell wrote:
> ppi@ is actually the best reviewer here. I don't think these fixes are enough
> fwiw, because we're not actually uploading the network service on the targets
> that you're interested in.

blundell, thanks for attention. Yes, I know you there is only android_arm
version on gn. That is why I submit https://codereview.chromium.org/917423004/
in chromium so that someone can upload android_x86 and android_x64
network_service mojo files together.

This is important for Intel guys that focus on IA android mojo works.

[haltonhuo, 2015-02-17 05:58:42]

ppi, psr, the patch set3 is updated according the discussion. Please review.

[qsr, 2015-02-17 09:11:45]

On 2015/02/17 05:58:42, haltonhuo wrote:
> ppi, psr, the patch set3 is updated according the discussion. Please review.

 This doesn't seem right. This should work whatever version of the os you are
using (I should be able to use this even for arm, for example to test a new
version of the network service)

 You need to work with gn variables to make this work. You could have a optional
argument that is network_service_location and have your copy use this as source
if it is defined.

[haltonhuo, 2015-03-06 11:11:04]

On 2015/02/17 09:11:45, qsr wrote:
> On 2015/02/17 05:58:42, haltonhuo wrote:
> > ppi, psr, the patch set3 is updated according the discussion. Please review.
> 
>  This doesn't seem right. This should work whatever version of the os you are
> using (I should be able to use this even for arm, for example to test a new
> version of the network service)
> 
>  You need to work with gn variables to make this work. You could have a
optional
> argument that is network_service_location and have your copy use this as
source
> if it is defined.

If I understand correctly, you're suggesting to add gn var and allow mojob to
set that with arg. The rough changes:
1. In mojo/public/tools/BUILD.gn, declare var
self_built_network_service_location with default "". In "copy_network_service"
use that source if that var is not "".
2. Add parameter for mojob to allow override the above gn arg when build.

Question: as we discussed in email, ppi suggest to "mojob build
--network-service PATH". This means we want to "--network-service" as sub-option
of build. While args are set during gen phase. I can come up two solutions:
1. "--network-service" as sub-option of gen instead of build.
2. Keep "--network-service" as sub-option of build. But run "gen gen --args" if
network-service is valid.

Which option you want to go?

[qsr, 2015-03-06 12:02:28]

On 2015/03/06 11:11:04, haltonhuo wrote:
> On 2015/02/17 09:11:45, qsr wrote:
> > On 2015/02/17 05:58:42, haltonhuo wrote:
> > > ppi, psr, the patch set3 is updated according the discussion. Please
review.
> > 
> >  This doesn't seem right. This should work whatever version of the os you
are
> > using (I should be able to use this even for arm, for example to test a new
> > version of the network service)
> > 
> >  You need to work with gn variables to make this work. You could have a
> optional
> > argument that is network_service_location and have your copy use this as
> source
> > if it is defined.
> 
> If I understand correctly, you're suggesting to add gn var and allow mojob to
> set that with arg. The rough changes:
> 1. In mojo/public/tools/BUILD.gn, declare var
> self_built_network_service_location with default "". In "copy_network_service"
> use that source if that var is not "".
> 2. Add parameter for mojob to allow override the above gn arg when build.
> 
> Question: as we discussed in email, ppi suggest to "mojob build
> --network-service PATH". This means we want to "--network-service" as
sub-option
> of build. While args are set during gen phase. I can come up two solutions:
> 1. "--network-service" as sub-option of gen instead of build.
> 2. Keep "--network-service" as sub-option of build. But run "gen gen --args"
if
> network-service is valid.
> 
> Which option you want to go?

 Option 1 seems better to me

[blundell, 2015-03-06 12:06:49]

On 2015/03/06 12:02:28, qsr wrote:
> On 2015/03/06 11:11:04, haltonhuo wrote:
> > On 2015/02/17 09:11:45, qsr wrote:
> > > On 2015/02/17 05:58:42, haltonhuo wrote:
> > > > ppi, psr, the patch set3 is updated according the discussion. Please
> review.
> > > 
> > >  This doesn't seem right. This should work whatever version of the os you
> are
> > > using (I should be able to use this even for arm, for example to test a
new
> > > version of the network service)
> > > 
> > >  You need to work with gn variables to make this work. You could have a
> > optional
> > > argument that is network_service_location and have your copy use this as
> > source
> > > if it is defined.
> > 
> > If I understand correctly, you're suggesting to add gn var and allow mojob
to
> > set that with arg. The rough changes:
> > 1. In mojo/public/tools/BUILD.gn, declare var
> > self_built_network_service_location with default "". In
"copy_network_service"
> > use that source if that var is not "".
> > 2. Add parameter for mojob to allow override the above gn arg when build.
> > 
> > Question: as we discussed in email, ppi suggest to "mojob build
> > --network-service PATH". This means we want to "--network-service" as
> sub-option
> > of build. While args are set during gen phase. I can come up two solutions:
> > 1. "--network-service" as sub-option of gen instead of build.
> > 2. Keep "--network-service" as sub-option of build. But run "gen gen --args"
> if
> > network-service is valid.
> > 
> > Which option you want to go?
> 
>  Option 1 seems better to me

I think that this variable should be in //build/module_args/mojo.gni. Note that
instead of having a default value, it should just not be defined by default: you
can check whether a variable is defined in GN.

I don't think there's a real need to have mojob.py understand this. You can just
change the value of the variable in //build/module_args/mojo.gni. If you have
known values where the prebuilt network service will live for all developers in
your repo, you could add that logic to //build/module_args/mojo.gni and commit
it in your repo:

if (is_android and is_x86) {
  prebuilt_network_service_location = "//foo/bar"
}

etc.

[haltonhuo, 2015-03-06 12:22:23]

On 2015/03/06 12:06:49, blundell wrote:
> I think that this variable should be in //build/module_args/mojo.gni. Note
that
> instead of having a default value, it should just not be defined by default:
you
> can check whether a variable is defined in GN.
> 
> I don't think there's a real need to have mojob.py understand this. You can
just
> change the value of the variable in //build/module_args/mojo.gni. If you have
> known values where the prebuilt network service will live for all developers
in
> your repo, you could add that logic to //build/module_args/mojo.gni and commit
> it in your repo:
> 
> if (is_android and is_x86) {
>   prebuilt_network_service_location = "//foo/bar"
> }
> 
> etc.

blundell, thanks for attention. This is not only my personal need to specify
this network_service mojo file. It will affect all developers working for
non-arm devices. And as qsr said, this feature can be used for arm as well(to
use self built instead of download from GN).

Thanks for suggestion of adding this var in //build/module_args/mojo.gni, I can
see it does have same problem for chromium if "use_prebuilt_network_service ==
true". I'll work that on chromium repo.

And for this mojob.py change, I do think it is still helpful to allow developer
easily change that var. "mojob.py" is the entrance to gen and build of the mojo
standalone repo. In contrast, chromium seems does not have that entrance, so "gn
args" is okay to use.

Should I proceed?

[blundell, 2015-03-06 12:23:11]

On 2015/03/06 12:06:49, blundell wrote:
> On 2015/03/06 12:02:28, qsr wrote:
> > On 2015/03/06 11:11:04, haltonhuo wrote:
> > > On 2015/02/17 09:11:45, qsr wrote:
> > > > On 2015/02/17 05:58:42, haltonhuo wrote:
> > > > > ppi, psr, the patch set3 is updated according the discussion. Please
> > review.
> > > > 
> > > >  This doesn't seem right. This should work whatever version of the os
you
> > are
> > > > using (I should be able to use this even for arm, for example to test a
> new
> > > > version of the network service)
> > > > 
> > > >  You need to work with gn variables to make this work. You could have a
> > > optional
> > > > argument that is network_service_location and have your copy use this as
> > > source
> > > > if it is defined.
> > > 
> > > If I understand correctly, you're suggesting to add gn var and allow mojob
> to
> > > set that with arg. The rough changes:
> > > 1. In mojo/public/tools/BUILD.gn, declare var
> > > self_built_network_service_location with default "". In
> "copy_network_service"
> > > use that source if that var is not "".
> > > 2. Add parameter for mojob to allow override the above gn arg when build.
> > > 
> > > Question: as we discussed in email, ppi suggest to "mojob build
> > > --network-service PATH". This means we want to "--network-service" as
> > sub-option
> > > of build. While args are set during gen phase. I can come up two
solutions:
> > > 1. "--network-service" as sub-option of gen instead of build.
> > > 2. Keep "--network-service" as sub-option of build. But run "gen gen
--args"
> > if
> > > network-service is valid.
> > > 
> > > Which option you want to go?
> > 
> >  Option 1 seems better to me
> 
> I think that this variable should be in //build/module_args/mojo.gni. Note
that
> instead of having a default value, it should just not be defined by default:
you
> can check whether a variable is defined in GN.
> 
> I don't think there's a real need to have mojob.py understand this. You can
just
> change the value of the variable in //build/module_args/mojo.gni. If you have
> known values where the prebuilt network service will live for all developers
in
> your repo, you could add that logic to //build/module_args/mojo.gni and commit
> it in your repo:
> 
> if (is_android and is_x86) {
>   prebuilt_network_service_location = "//foo/bar"
> }
> 
> etc.

qsr@ and I discussed a bit more offline. I think the best solution here is to
have this variable be a buildarg whose value defaults to "". Then when you want
to change it you can simply set the value of the arg via "gn args <out_dir>"
after having run mojob.py gn.

[blundell, 2015-03-06 12:25:01]

On 2015/03/06 12:22:23, haltonhuo wrote:
> On 2015/03/06 12:06:49, blundell wrote:
> > I think that this variable should be in //build/module_args/mojo.gni. Note
> that
> > instead of having a default value, it should just not be defined by default:
> you
> > can check whether a variable is defined in GN.
> > 
> > I don't think there's a real need to have mojob.py understand this. You can
> just
> > change the value of the variable in //build/module_args/mojo.gni. If you
have
> > known values where the prebuilt network service will live for all developers
> in
> > your repo, you could add that logic to //build/module_args/mojo.gni and
commit
> > it in your repo:
> > 
> > if (is_android and is_x86) {
> >   prebuilt_network_service_location = "//foo/bar"
> > }
> > 
> > etc.
> 
> blundell, thanks for attention. This is not only my personal need to specify
> this network_service mojo file. It will affect all developers working for
> non-arm devices. And as qsr said, this feature can be used for arm as well(to
> use self built instead of download from GN).
> 
> Thanks for suggestion of adding this var in //build/module_args/mojo.gni, I
can
> see it does have same problem for chromium if "use_prebuilt_network_service ==
> true". I'll work that on chromium repo.
> 
> And for this mojob.py change, I do think it is still helpful to allow
developer
> easily change that var. "mojob.py" is the entrance to gen and build of the
mojo
> standalone repo. In contrast, chromium seems does not have that entrance, so
"gn
> args" is okay to use.
> 
> Should I proceed?

Our emails crossed. If you want to add the ability to change this via
"mojob.py", what I would do is have "mojob.py gn" be able to take in a --args
option, which it simply passes on to gn via its --args option. That way you're
coming up with a reusable solution instead of something totally custom.

[haltonhuo, 2015-03-06 14:29:51]

blundell, I do not found the var in //build/module_args/mojo.gni will be listed
in "gn args --list out/android_Debug". Is that right way to use?

Error if I have self_built_network_service_location = "" in
//build/module_args/mojo.gni
==================
$ gn args out/android_Debug/
Waiting for editor on
"/home/halton/work/projects/github/mojo/android/src/out/android_Debug/args.gn"...
Generating files...
ERROR at //mojo/BUILD.gn:6:1: Value collision.
import("//build/module_args/mojo.gni")
^------------------------------------
This import contains "self_built_network_service_location"
See //build/module_args/mojo.gni:18:39: defined here.
self_built_network_service_location = ""
                                      ^-
Which would clobber the one in your current scope
See build arg file (use "gn args <out_dir>" to edit):9:39: defined here.
self_built_network_service_location =
"/home/halton/work/projects/github/mojo/android/src/mojo/public/tools/prebuilt/network_service/android-arm/network_service.mojo"
                                     
^-------------------------------------------------------------------------------------------------------------------------------
Executing import should not conflict with anything in the current
scope unless the values are identical.


If I remove that var in mojo.gni, error below:
===========
The variable "self_built_network_service_location" was set as a build argument
but never appeared in a declare_args() block in any buildfile.

[blundell, 2015-03-06 14:34:31]

On 2015/03/06 14:29:51, haltonhuo wrote:
> blundell, I do not found the var in //build/module_args/mojo.gni will be
listed
> in "gn args --list out/android_Debug". Is that right way to use?
> 
> Error if I have self_built_network_service_location = "" in
> //build/module_args/mojo.gni
> ==================
> $ gn args out/android_Debug/
> Waiting for editor on
>
"/home/halton/work/projects/github/mojo/android/src/out/android_Debug/args.gn"...
> Generating files...
> ERROR at //mojo/BUILD.gn:6:1: Value collision.
> import("//build/module_args/mojo.gni")
> ^------------------------------------
> This import contains "self_built_network_service_location"
> See //build/module_args/mojo.gni:18:39: defined here.
> self_built_network_service_location = ""
>                                       ^-
> Which would clobber the one in your current scope
> See build arg file (use "gn args <out_dir>" to edit):9:39: defined here.
> self_built_network_service_location =
>
"/home/halton/work/projects/github/mojo/android/src/mojo/public/tools/prebuilt/network_service/android-arm/network_service.mojo"
>                                      
>
^-------------------------------------------------------------------------------------------------------------------------------
> Executing import should not conflict with anything in the current
> scope unless the values are identical.
> 
> 
> If I remove that var in mojo.gni, error below:
> ===========
> The variable "self_built_network_service_location" was set as a build argument
> but never appeared in a declare_args() block in any buildfile.

It needs to be specified as a build_arg in //build/module_args/mojo.gni, not a
vanilla variable. "gn help declare_args"

[haltonhuo, 2015-03-06 15:39:27]

blundell, patch set4 is updated as we discussed. Now it depends on the chromium
part CL https://codereview.chromium.org/983143003/ as dependency.

After give a try to add "gn --args" support to mojob.py, it will be affect
several modules. I think "gn args" is okay to used by now.

[haltonhuo, 2015-03-06 15:45:53]

On 2015/03/06 15:39:27, haltonhuo wrote:
> blundell, patch set4 is updated as we discussed. Now it depends on the
chromium
> part CL https://codereview.chromium.org/983143003/ as dependency.
> 
> After give a try to add "gn --args" support to mojob.py, it will be affect
> several modules. I think "gn args" is okay to used by now.

One question is: There also only arm version of MojoShell.apk and
network_service_apptests.mojo on gn://, should we do the same thing? (I just
leave them untouched in the patch now)

[haltonhuo, 2015-03-09 11:53:53]

In patch set 5, I added --args option to allow specify
self_built_network_service_location qs we discussed previously. Otherwise gn
will fail on the checking of cpu_arch == "arm" when target-cpu is x86/x64.

Open: There also only arm version of MojoShell.apk and
network_service_apptests.mojo on gn://, should we do the same thing?

[qsr, 2015-03-09 11:58:42]

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

https://codereview.chromium.org/921053003/diff/80001/mojo/public/tools/BUILD....
mojo/public/tools/BUILD.gn:34: if (defined(self_built_network_service_location)
&&
Don't you need to define this variable somewhere?

Also, maybe name it prebuilt_network_service_location instead.

https://codereview.chromium.org/921053003/diff/80001/mojo/tools/mojob.py
File mojo/tools/mojob.py (right):

https://codereview.chromium.org/921053003/diff/80001/mojo/tools/mojob.py#newc...
mojo/tools/mojob.py:223: gn_parser.add_argument('--args', help='Specify extra
args for gn phase',
No need to specify that it is for the gn phase. This parser is only for the gn
phase, so won't be used for anything else.

https://codereview.chromium.org/921053003/diff/80001/mojo/tools/mopy/gn.py
File mojo/tools/mopy/gn.py (right):

https://codereview.chromium.org/921053003/diff/80001/mojo/tools/mopy/gn.py#ne...
mojo/tools/mopy/gn.py:77: val =
config.values.get("self_built_network_service_location")
I don't think you want this here. Any reason you would need it?

[haltonhuo, 2015-03-09 12:13:00]

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

https://codereview.chromium.org/921053003/diff/80001/mojo/public/tools/BUILD....
mojo/public/tools/BUILD.gn:34: if (defined(self_built_network_service_location)
&&
On 2015/03/09 11:58:41, qsr wrote:
> Don't you need to define this variable somewhere?
> 
> Also, maybe name it prebuilt_network_service_location instead.

As blundell suggested, I move the declaration in build/module_args/mojo.gni,
which is changed in CL https://codereview.chromium.org/983143003/

I'm okay with the name "prebuilt_network_service_location", will change in next
patch set.

https://codereview.chromium.org/921053003/diff/80001/mojo/tools/mojob.py
File mojo/tools/mojob.py (right):

https://codereview.chromium.org/921053003/diff/80001/mojo/tools/mojob.py#newc...
mojo/tools/mojob.py:223: gn_parser.add_argument('--args', help='Specify extra
args for gn phase',
On 2015/03/09 11:58:41, qsr wrote:
> No need to specify that it is for the gn phase. This parser is only for the gn
> phase, so won't be used for anything else.

It is added only under "gn" sub-command, did I missing something?

https://codereview.chromium.org/921053003/diff/80001/mojo/tools/mopy/gn.py
File mojo/tools/mopy/gn.py (right):

https://codereview.chromium.org/921053003/diff/80001/mojo/tools/mopy/gn.py#ne...
mojo/tools/mopy/gn.py:77: val =
config.values.get("self_built_network_service_location")
On 2015/03/09 11:58:41, qsr wrote:
> I don't think you want this here. Any reason you would need it?

Yes, it is needed, otherwise the "gn gen --args" command will missed that
argument, thus the self_built_network_service_location won't bet set in
out_dir/args.gn.

[qsr, 2015-03-09 13:13:06]

https://codereview.chromium.org/921053003/diff/80001/mojo/tools/mojob.py
File mojo/tools/mojob.py (right):

https://codereview.chromium.org/921053003/diff/80001/mojo/tools/mojob.py#newc...
mojo/tools/mojob.py:223: gn_parser.add_argument('--args', help='Specify extra
args for gn phase',
On 2015/03/09 12:13:00, haltonhuo wrote:
> On 2015/03/09 11:58:41, qsr wrote:
> > No need to specify that it is for the gn phase. This parser is only for the
gn
> > phase, so won't be used for anything else.
> 
> It is added only under "gn" sub-command, did I missing something?

 I meant that you do not need to speak about the gn phasein the help message.
Just have 'Specify extra args'

https://codereview.chromium.org/921053003/diff/80001/mojo/tools/mopy/gn.py
File mojo/tools/mopy/gn.py (right):

https://codereview.chromium.org/921053003/diff/80001/mojo/tools/mopy/gn.py#ne...
mojo/tools/mopy/gn.py:77: val =
config.values.get("self_built_network_service_location")
On 2015/03/09 12:13:00, haltonhuo wrote:
> On 2015/03/09 11:58:41, qsr wrote:
> > I don't think you want this here. Any reason you would need it?
> 
> Yes, it is needed, otherwise the "gn gen --args" command will missed that
> argument, thus the self_built_network_service_location won't bet set in
> out_dir/args.gn.

You do not want to have anything about this variable in those files, and what
you add should work for all args passed to mojob.

[haltonhuo, 2015-03-10 06:50:44]

qsr, all your comments are addressed in patch set 6, please review.

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

https://codereview.chromium.org/921053003/diff/80001/mojo/public/tools/BUILD....
mojo/public/tools/BUILD.gn:34: if (defined(self_built_network_service_location)
&&
On 2015/03/09 12:13:00, haltonhuo wrote:
> On 2015/03/09 11:58:41, qsr wrote:
> > Don't you need to define this variable somewhere?
> > 
> > Also, maybe name it prebuilt_network_service_location instead.
> 
> As blundell suggested, I move the declaration in build/module_args/mojo.gni,
> which is changed in CL https://codereview.chromium.org/983143003/
> 
> I'm okay with the name "prebuilt_network_service_location", will change in
next
> patch set.

Done.

https://codereview.chromium.org/921053003/diff/80001/mojo/tools/mojob.py
File mojo/tools/mojob.py (right):

https://codereview.chromium.org/921053003/diff/80001/mojo/tools/mojob.py#newc...
mojo/tools/mojob.py:223: gn_parser.add_argument('--args', help='Specify extra
args for gn phase',
On 2015/03/09 13:13:06, qsr wrote:
> On 2015/03/09 12:13:00, haltonhuo wrote:
> > On 2015/03/09 11:58:41, qsr wrote:
> > > No need to specify that it is for the gn phase. This parser is only for
the
> gn
> > > phase, so won't be used for anything else.
> > 
> > It is added only under "gn" sub-command, did I missing something?
> 
>  I meant that you do not need to speak about the gn phasein the help message.
> Just have 'Specify extra args'

Done in patch set 6.

https://codereview.chromium.org/921053003/diff/80001/mojo/tools/mopy/gn.py
File mojo/tools/mopy/gn.py (right):

https://codereview.chromium.org/921053003/diff/80001/mojo/tools/mopy/gn.py#ne...
mojo/tools/mopy/gn.py:77: val =
config.values.get("self_built_network_service_location")
On 2015/03/09 13:13:06, qsr wrote:
> On 2015/03/09 12:13:00, haltonhuo wrote:
> > On 2015/03/09 11:58:41, qsr wrote:
> > > I don't think you want this here. Any reason you would need it?
> > 
> > Yes, it is needed, otherwise the "gn gen --args" command will missed that
> > argument, thus the self_built_network_service_location won't bet set in
> > out_dir/args.gn.
> 
> You do not want to have anything about this variable in those files, and what
> you add should work for all args passed to mojob.
> 
>  

Done in patch set 6

[qsr, 2015-03-10 10:02:38]

https://codereview.chromium.org/921053003/diff/100001/mojo/tools/mojob.py
File mojo/tools/mojob.py (right):

https://codereview.chromium.org/921053003/diff/100001/mojo/tools/mojob.py#new...
mojo/tools/mojob.py:68: additional_args['extra_args'] = args.extra_args
Could you use gn_args instead of extra_args?

https://codereview.chromium.org/921053003/diff/100001/mojo/tools/mopy/gn.py
File mojo/tools/mopy/gn.py (right):

https://codereview.chromium.org/921053003/diff/100001/mojo/tools/mopy/gn.py#n...
mojo/tools/mopy/gn.py:78: if (extra_args):
Parenthesis are not needed.

https://codereview.chromium.org/921053003/diff/100001/mojo/tools/mopy/gn.py#n...
mojo/tools/mopy/gn.py:79: for arg in extra_args.split(' '):
split() is enough when splitting with whitespaces. This will also handles
multiple spaces.

[haltonhuo, 2015-03-11 05:58:06]

https://codereview.chromium.org/921053003/diff/100001/mojo/tools/mojob.py
File mojo/tools/mojob.py (right):

https://codereview.chromium.org/921053003/diff/100001/mojo/tools/mojob.py#new...
mojo/tools/mojob.py:68: additional_args['extra_args'] = args.extra_args
On 2015/03/10 10:02:38, qsr wrote:
> Could you use gn_args instead of extra_args?

Done.

https://codereview.chromium.org/921053003/diff/100001/mojo/tools/mopy/gn.py
File mojo/tools/mopy/gn.py (right):

https://codereview.chromium.org/921053003/diff/100001/mojo/tools/mopy/gn.py#n...
mojo/tools/mopy/gn.py:78: if (extra_args):
On 2015/03/10 10:02:38, qsr wrote:
> Parenthesis are not needed.

Done.

https://codereview.chromium.org/921053003/diff/100001/mojo/tools/mopy/gn.py#n...
mojo/tools/mopy/gn.py:79: for arg in extra_args.split(' '):
On 2015/03/10 10:02:38, qsr wrote:
> split() is enough when splitting with whitespaces. This will also handles
> multiple spaces.

Done.

[qsr, 2015-03-12 13:31:15]

lgtm

[haltonhuo, 2015-03-13 01:32:08]

On 2015/03/12 13:31:15, qsr wrote:
> lgtm

thanks, could you land it by hand as you usually did?

[qsr, 2015-03-16 09:44:20]

Committed patchset #7 (id:120001) manually as
3b020f62f0fd03493ec08e400b5f387056820609 (presubmit successful).

[qsr, 2015-03-16 09:44:38]

On 2015/03/13 01:32:08, haltonhuo wrote:
> On 2015/03/12 13:31:15, qsr wrote:
> > lgtm
> 
> thanks, could you land it by hand as you usually did?

Done