Simplify cli_main

Simplify cli_main
 - Have cli_main register main, move everything else to nacl_spawn.
 - Add nacl_spawn to NACL_CLI_MAIN_LIB
 - Use NACL_CLI_MAIN_LIB in build files
 - Fix a typo in the README.rst

BUG=NONE
R=bradnelson@google.com, sbc@chromium.org, sbc@google.com

Committed: https://chromium.googlesource.com/external/naclports/+/e5548672278a4b49f52238f942d62568c604e9b9

[bradn, 2015-02-13 02:12:32]

LGTM with issues.
Do wait for Sam before landing though.

https://codereview.chromium.org/907563002/diff/140001/ports/nacl-spawn/nacl_s...
File ports/nacl-spawn/nacl_spawn.cc (right):

https://codereview.chromium.org/907563002/diff/140001/ports/nacl-spawn/nacl_s...
ports/nacl-spawn/nacl_spawn.cc:92: void setup_env() {
How about something like setup_nacl_spawn_env, as this is in the global
namespace ?

https://codereview.chromium.org/907563002/diff/140001/ports/python-static/bui...
File ports/python-static/build.sh (right):

https://codereview.chromium.org/907563002/diff/140001/ports/python-static/bui...
ports/python-static/build.sh:60: PY_LINK_LINE+=" ${PY_MOD_LIBS}
-Wl,-uPSUserMainGet -lcli_main -lnacl_spawn"
Don't you want to drop this -lcli_main ?
I thought this one didn't actually use cli_main?

https://codereview.chromium.org/907563002/diff/140001/ports/sdl/nacl.patch
File ports/sdl/nacl.patch (right):

https://codereview.chromium.org/907563002/diff/140001/ports/sdl/nacl.patch#ne...
ports/sdl/nacl.patch:499: os=-elf
So this part isn't really meant to sneak into the patch.
Either manually delete it, or when you run updatepatch, be sure to git stash
transitory things and checkout the prior rev.

[Sam Clegg, 2015-02-13 18:35:17]

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

[Sam Clegg, 2015-02-13 18:35:18]

https://codereview.chromium.org/907563002/diff/140001/ports/nacl-spawn/cli_ma...
File ports/nacl-spawn/cli_main.cc (right):

https://codereview.chromium.org/907563002/diff/140001/ports/nacl-spawn/cli_ma...
ports/nacl-spawn/cli_main.cc:3: * Use of this source code is governed by a
BSD-style license that can be
How hard would it be to turn this back into a .c file?

https://codereview.chromium.org/907563002/diff/140001/ports/nacl-spawn/nacl_s...
File ports/nacl-spawn/nacl_spawn.cc (right):

https://codereview.chromium.org/907563002/diff/140001/ports/nacl-spawn/nacl_s...
ports/nacl-spawn/nacl_spawn.cc:92: void setup_env() {
On 2015/02/13 02:12:32, bradn wrote:
> How about something like setup_nacl_spawn_env, as this is in the global
> namespace ?

I agree.   Perhaps prefix with _nacl?

So this function was moved to nacl-spawn so it that nacl-spawn could use the
static helper functions above?  Perhaps it would be better to leave the body of
this function in cli_main, but expose the above helpers via a header?  (with
some kind of prefix to avoid too much global namespace pollution?)

https://codereview.chromium.org/907563002/diff/140001/ports/python-static/nac...
File ports/python-static/nacl.patch (left):

https://codereview.chromium.org/907563002/diff/140001/ports/python-static/nac...
ports/python-static/nacl.patch:444: +
So these are not longer needed because libnacl-spawn is always linked now, even
for the sel_ldr binary?

If that is true then is nacl_io also always linked?  If so we can remove the
uname below too maybe?

https://codereview.chromium.org/907563002/diff/140001/ports/tcl/build.sh
File ports/tcl/build.sh (right):

https://codereview.chromium.org/907563002/diff/140001/ports/tcl/build.sh#newc...
ports/tcl/build.sh:21: export ac_cv_func_memmove=yes
So this was trying to build its own memmove without this?  Did you just happen
to notice this or was the build failing.

[gdeepti, 2015-02-13 20:59:02]

New patchsets have been uploaded after l-g-t-m from bradnelson@google.com

[gdeepti, 2015-02-13 21:43:56]

https://codereview.chromium.org/907563002/diff/140001/ports/nacl-spawn/cli_ma...
File ports/nacl-spawn/cli_main.cc (right):

https://codereview.chromium.org/907563002/diff/140001/ports/nacl-spawn/cli_ma...
ports/nacl-spawn/cli_main.cc:3: * Use of this source code is governed by a
BSD-style license that can be
On 2015/02/13 18:35:18, Sam Clegg wrote:
> How hard would it be to turn this back into a .c file?

It shouldn't be hard, will check it in with the next patch.

https://codereview.chromium.org/907563002/diff/140001/ports/nacl-spawn/nacl_s...
File ports/nacl-spawn/nacl_spawn.cc (right):

https://codereview.chromium.org/907563002/diff/140001/ports/nacl-spawn/nacl_s...
ports/nacl-spawn/nacl_spawn.cc:92: void setup_env() {
On 2015/02/13 02:12:32, bradn wrote:
> How about something like setup_nacl_spawn_env, as this is in the global
> namespace ?

Done.

https://codereview.chromium.org/907563002/diff/140001/ports/nacl-spawn/nacl_s...
ports/nacl-spawn/nacl_spawn.cc:92: void setup_env() {
On 2015/02/13 18:35:18, Sam Clegg wrote:
> On 2015/02/13 02:12:32, bradn wrote:
> > How about something like setup_nacl_spawn_env, as this is in the global
> > namespace ?
> 
> I agree.   Perhaps prefix with _nacl?
> 
> So this function was moved to nacl-spawn so it that nacl-spawn could use the
> static helper functions above?  Perhaps it would be better to leave the body
of
> this function in cli_main, but expose the above helpers via a header?  (with
> some kind of prefix to avoid too much global namespace pollution?)

Apart from having access to the static helper functions above it was also moved
so that the mount functions can use SendRequest(pp::VarDictionary* req) to send
requests to Javascript. I can move the helper functions and the body back to
cli_main and just keep the mount specific functionality here. Would that be
better?

https://codereview.chromium.org/907563002/diff/140001/ports/python-static/bui...
File ports/python-static/build.sh (right):

https://codereview.chromium.org/907563002/diff/140001/ports/python-static/bui...
ports/python-static/build.sh:60: PY_LINK_LINE+=" ${PY_MOD_LIBS}
-Wl,-uPSUserMainGet -lcli_main -lnacl_spawn"
On 2015/02/13 02:12:32, bradn wrote:
> Don't you want to drop this -lcli_main ?
> I thought this one didn't actually use cli_main?

I think this one still uses cli_main as the entry point.

https://codereview.chromium.org/907563002/diff/140001/ports/python-static/nac...
File ports/python-static/nacl.patch (left):

https://codereview.chromium.org/907563002/diff/140001/ports/python-static/nac...
ports/python-static/nacl.patch:444: +
On 2015/02/13 18:35:18, Sam Clegg wrote:
> So these are not longer needed because libnacl-spawn is always linked now,
even
> for the sel_ldr binary?
> 
> If that is true then is nacl_io also always linked?  If so we can remove the
> uname below too maybe?

Good point, removed uname as well.

https://codereview.chromium.org/907563002/diff/140001/ports/python-static/nac...
ports/python-static/nacl.patch:444: +
On 2015/02/13 18:35:18, Sam Clegg wrote:
> So these are not longer needed because libnacl-spawn is always linked now,
even
> for the sel_ldr binary?
> 
> If that is true then is nacl_io also always linked?  If so we can remove the
> uname below too maybe?

Done.

https://codereview.chromium.org/907563002/diff/140001/ports/sdl/nacl.patch
File ports/sdl/nacl.patch (right):

https://codereview.chromium.org/907563002/diff/140001/ports/sdl/nacl.patch#ne...
ports/sdl/nacl.patch:499: os=-elf
On 2015/02/13 02:12:32, bradn wrote:
> So this part isn't really meant to sneak into the patch.
> Either manually delete it, or when you run updatepatch, be sure to git stash
> transitory things and checkout the prior rev.

Done.

https://codereview.chromium.org/907563002/diff/140001/ports/tcl/build.sh
File ports/tcl/build.sh (right):

https://codereview.chromium.org/907563002/diff/140001/ports/tcl/build.sh#newc...
ports/tcl/build.sh:21: export ac_cv_func_memmove=yes
On 2015/02/13 18:35:18, Sam Clegg wrote:
> So this was trying to build its own memmove without this?  Did you just happen
> to notice this or was the build failing.

The build was failing without this.

[Sam Clegg, 2015-02-17 18:22:45]

lgtm with comments.

https://codereview.chromium.org/907563002/diff/180001/ports/nacl-spawn/cli_ma...
File ports/nacl-spawn/cli_main.c (right):

https://codereview.chromium.org/907563002/diff/180001/ports/nacl-spawn/cli_ma...
ports/nacl-spawn/cli_main.c:13: extern int nacl_main(int argc, char *argv[]);
This is declared in nacl_main.h already.

https://codereview.chromium.org/907563002/diff/180001/ports/nacl-spawn/cli_ma...
ports/nacl-spawn/cli_main.c:14: void setup_nacl_spawn_env();
Declare this in nacl_main.h too.

Also, can can change the name of nacl_spawn comes first?

nacl_spawn_setup_env()?   That way any other functions we
add to the global namespace can share the same prefix.

https://codereview.chromium.org/907563002/diff/180001/ports/nacl-spawn/nacl_s...
File ports/nacl-spawn/nacl_spawn.cc (right):

https://codereview.chromium.org/907563002/diff/180001/ports/nacl-spawn/nacl_s...
ports/nacl-spawn/nacl_spawn.cc:92: extern "C" void setup_nacl_spawn_env() {
Don't need the "extern C" if you declare this in the header.

[gdeepti, 2015-02-17 21:02:04]

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

[gdeepti, 2015-02-17 21:06:49]

https://codereview.chromium.org/907563002/diff/180001/ports/nacl-spawn/cli_ma...
File ports/nacl-spawn/cli_main.c (right):

https://codereview.chromium.org/907563002/diff/180001/ports/nacl-spawn/cli_ma...
ports/nacl-spawn/cli_main.c:13: extern int nacl_main(int argc, char *argv[]);
On 2015/02/17 18:22:45, Sam Clegg wrote:
> This is declared in nacl_main.h already.

Acknowledged.

https://codereview.chromium.org/907563002/diff/180001/ports/nacl-spawn/cli_ma...
ports/nacl-spawn/cli_main.c:14: void setup_nacl_spawn_env();
On 2015/02/17 18:22:45, Sam Clegg wrote:
> Declare this in nacl_main.h too.
> 
> Also, can can change the name of nacl_spawn comes first?
> 
> nacl_spawn_setup_env()?   That way any other functions we
> add to the global namespace can share the same prefix.

Done.

https://codereview.chromium.org/907563002/diff/180001/ports/nacl-spawn/nacl_s...
File ports/nacl-spawn/nacl_spawn.cc (right):

https://codereview.chromium.org/907563002/diff/180001/ports/nacl-spawn/nacl_s...
ports/nacl-spawn/nacl_spawn.cc:92: extern "C" void setup_nacl_spawn_env() {
On 2015/02/17 18:22:45, Sam Clegg wrote:
> Don't need the "extern C" if you declare this in the header.

Done.

[Sam Clegg, 2015-02-17 21:27:52]

https://codereview.chromium.org/907563002/diff/220001/ports/nacl-spawn/includ...
File ports/nacl-spawn/include/nacl_main.h (right):

https://codereview.chromium.org/907563002/diff/220001/ports/nacl-spawn/includ...
ports/nacl-spawn/include/nacl_main.h:45: 
Perhaps just nacl_setup_env() to be consistent with the function above?    Or
rename above to be consistent with this one.

[gdeepti, 2015-02-17 21:59:43]

https://codereview.chromium.org/907563002/diff/220001/ports/nacl-spawn/includ...
File ports/nacl-spawn/include/nacl_main.h (right):

https://codereview.chromium.org/907563002/diff/220001/ports/nacl-spawn/includ...
ports/nacl-spawn/include/nacl_main.h:45: 
On 2015/02/17 21:27:52, Sam Clegg wrote:
> Perhaps just nacl_setup_env() to be consistent with the function above?    Or
> rename above to be consistent with this one.

Done.

[Sam Clegg, 2015-02-17 22:11:26]

thanks, lgtm

(no need to re-run trybots for spelling fixes)

https://codereview.chromium.org/907563002/diff/240001/ports/nacl-spawn/includ...
File ports/nacl-spawn/include/nacl_main.h (right):

https://codereview.chromium.org/907563002/diff/240001/ports/nacl-spawn/includ...
ports/nacl-spawn/include/nacl_main.h:41: * Setup common environt variables and
mounts.
sp: environment

[gdeepti, 2015-02-18 01:48:00]

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

[gdeepti, 2015-02-18 01:49:14]

https://codereview.chromium.org/907563002/diff/240001/ports/nacl-spawn/includ...
File ports/nacl-spawn/include/nacl_main.h (right):

https://codereview.chromium.org/907563002/diff/240001/ports/nacl-spawn/includ...
ports/nacl-spawn/include/nacl_main.h:41: * Setup common environt variables and
mounts.
On 2015/02/17 22:11:26, Sam Clegg wrote:
> sp: environment

Done.

[gdeepti, 2015-02-18 18:04:15]

Committed patchset #14 (id:260001) manually as
e5548672278a4b49f52238f942d62568c604e9b9 (presubmit successful).