Move nacl_cmd_line to components/nacl/common.

Move nacl_cmd_line to components/nacl/common.

Move the files and create a target for them, which is included in chrome_common.gypi.

This is part of componentizing NaCl code.
BUG=244791
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=212886

[yael.aharon1, 2013-07-12 15:26:25]

I broke this off from https://codereview.chromium.org/16881004/ in hope this can
help make the review more manageable.
Can you please review?

[yael.aharon1, 2013-07-19 13:46:07]

oing?

[yael.aharon1, 2013-07-19 14:08:08]

On 2013/07/19 13:46:07, yael.aharon1 wrote:
ping ?

[Mark Seaborn, 2013-07-19 15:14:27]

LGTM

https://codereview.chromium.org/18098009/diff/6001/chrome/nacl.gypi
File chrome/nacl.gypi (right):

https://codereview.chromium.org/18098009/diff/6001/chrome/nacl.gypi#newcode121
chrome/nacl.gypi:121: '../components/nacl/common/nacl_cmd_line.cc',
This doesn't seem very clean.  Can you make this target depend on the
nacl_cmd_line library instead?  You'll probably need to add a
nacl_cmd_line_win64 library to build an x86-64 version.

https://codereview.chromium.org/18098009/diff/6001/components/nacl_common.gyp
File components/nacl_common.gyp (right):

https://codereview.chromium.org/18098009/diff/6001/components/nacl_common.gyp...
components/nacl_common.gyp:19: 'target_name': 'nacl_cmd_line',
Is there a particular reason that this file is in its own library?  Can you
rename this library to "nacl_common", please?

[yael.aharon1, 2013-07-19 18:10:21]

@joi, can you please review the changes to components/nacl_common.gup ?
thanks

[Mark Seaborn, 2013-07-19 18:14:05]

On 2013/07/19 18:10:21, yael.aharon1 wrote:
> @joi, can you please review the changes to components/nacl_common.gup ?
> thanks

Hmm, I hadn't noticed before that the Gyp file is components/nacl_common.gyp,
directly under components/.  Is there any reason this is not
components/nacl/nacl_common.gyp?

If I can approve changes to components/nacl/* I should really be able to approve
changes to its Gyp file.  Maybe some or all of the people in
components/nacl/OWNERS should be added as per-file OWNERS of
components/nacl_common.gyp?

[yael.aharon1, 2013-07-19 18:19:34]

On 2013/07/19 18:14:05, Mark Seaborn wrote:
> On 2013/07/19 18:10:21, yael.aharon1 wrote:
> > @joi, can you please review the changes to components/nacl_common.gup ?
> > thanks
> 
> Hmm, I hadn't noticed before that the Gyp file is components/nacl_common.gyp,
> directly under components/.  Is there any reason this is not
> components/nacl/nacl_common.gyp?
> 
I did not find any other gyp file which is more than one level deep under
components, but of course, I could be wrong.

> If I can approve changes to components/nacl/* I should really be able to
approve
> changes to its Gyp file.  Maybe some or all of the people in
> components/nacl/OWNERS should be added as per-file OWNERS of
> components/nacl_common.gyp?

I created https://codereview.chromium.org/19831003/ for the OWNERS file changes.

[Jói, 2013-07-19 19:42:47]

//components/* LGTM.

[joi, 2013-07-19 19:43:01]

> Hmm, I hadn't noticed before that the Gyp file is
> components/nacl_common.gyp,
> directly under components/.  Is there any reason this is not
> components/nacl/nacl_common.gyp?

This is by convention and avoids some problems you get if you place
your .gypi file at a different depth than the .gyp file that includes
the .gypi files.  If you do that it results in inconsistencies in how
relative paths are handled.

Cheers,
Jói


On Fri, Jul 19, 2013 at 6:14 PM,  <mseaborn@chromium.org> wrote:
> On 2013/07/19 18:10:21, yael.aharon1 wrote:
>>
>> @joi, can you please review the changes to components/nacl_common.gup ?
>> thanks
>
>
> Hmm, I hadn't noticed before that the Gyp file is
> components/nacl_common.gyp,
> directly under components/.  Is there any reason this is not
> components/nacl/nacl_common.gyp?
>
> If I can approve changes to components/nacl/* I should really be able to
> approve
> changes to its Gyp file.  Maybe some or all of the people in
> components/nacl/OWNERS should be added as per-file OWNERS of
> components/nacl_common.gyp?
>
> https://codereview.chromium.org/18098009/

[joi, 2013-07-19 19:43:01]

> Hmm, I hadn't noticed before that the Gyp file is
> components/nacl_common.gyp,
> directly under components/.  Is there any reason this is not
> components/nacl/nacl_common.gyp?

This is by convention and avoids some problems you get if you place
your .gypi file at a different depth than the .gyp file that includes
the .gypi files.  If you do that it results in inconsistencies in how
relative paths are handled.

Cheers,
Jói


On Fri, Jul 19, 2013 at 6:14 PM,  <mseaborn@chromium.org> wrote:
> On 2013/07/19 18:10:21, yael.aharon1 wrote:
>>
>> @joi, can you please review the changes to components/nacl_common.gup ?
>> thanks
>
>
> Hmm, I hadn't noticed before that the Gyp file is
> components/nacl_common.gyp,
> directly under components/.  Is there any reason this is not
> components/nacl/nacl_common.gyp?
>
> If I can approve changes to components/nacl/* I should really be able to
> approve
> changes to its Gyp file.  Maybe some or all of the people in
> components/nacl/OWNERS should be added as per-file OWNERS of
> components/nacl_common.gyp?
>
> https://codereview.chromium.org/18098009/

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to native-client-reviews+unsubscribe@googlegroups.com.
To post to this group, send email to native-client-reviews@googlegroups.com.
Visit this group at http://groups.google.com/group/native-client-reviews.
For more options, visit https://groups.google.com/groups/opt_out.

[commit-bot: I haz the power, 2013-07-19 19:50:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yael.aharon@intel.com/18098009/17001

[commit-bot: I haz the power, 2013-07-19 20:22:57]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[yael.aharon1, 2013-07-19 20:46:03]

Jam@ can you please review the deleted files?
Thanks!

[jam, 2013-07-22 06:34:49]

lgtm

[commit-bot: I haz the power, 2013-07-22 12:29:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yael.aharon@intel.com/18098009/17001

[commit-bot: I haz the power, 2013-07-22 14:54:59]

Change committed as 212886

[tfarina, 2013-07-22 16:12:16]

https://chromiumcodereview.appspot.com/18098009/diff/17001/components/nacl/co...
File components/nacl/common/nacl_cmd_line.h (right):

https://chromiumcodereview.appspot.com/18098009/diff/17001/components/nacl/co...
components/nacl/common/nacl_cmd_line.h:11: // Copy all the relevant arguments
from the command line of the current
minor-style-nit: add a blank line above.

https://chromiumcodereview.appspot.com/18098009/diff/17001/components/nacl/co...
components/nacl/common/nacl_cmd_line.h:13: void
CopyNaClCommandLineArguments(CommandLine* cmd_line);
minor-style-nit: don't indent inside namespace.

https://chromiumcodereview.appspot.com/18098009/diff/17001/components/nacl/co...
components/nacl/common/nacl_cmd_line.h:14: }
minor-style-nit: add blank line above and }  // namespace nacl