Issue 75080043: code review 75080043: misc/nacl: add Native Client support scripts and docume... (Closed)
Descriptionmisc/nacl: add Native Client support scripts and documentation
Patch Set 1 #Patch Set 2 : diff -r 4d38723d03f0 https://code.google.com/p/go #Patch Set 3 : diff -r 4d38723d03f0 https://code.google.com/p/go #Patch Set 4 : diff -r 67ba17a89376 https://code.google.com/p/go #
Total comments: 34
Patch Set 5 : diff -r 321d42ff40d3 https://code.google.com/p/go #
Total comments: 4
Patch Set 6 : diff -r 3ca54dd30864 https://code.google.com/p/go #
Total comments: 5
Patch Set 7 : diff -r 3ca54dd30864 https://code.google.com/p/go #Patch Set 8 : diff -r 3ca54dd30864 https://code.google.com/p/go #Patch Set 9 : diff -r 01358eb1cea5 https://code.google.com/p/go #
MessagesTotal messages: 20
Hello golang-codereviews@googlegroups.com, I'd like you to review this change to https://code.google.com/p/go
Sign in to reply to this message.
This isn't perfect, mailing for feedback.
Sign in to reply to this message.
Thanks for writing this up! I followed the instructions pretty mindlessly and
now have a working NaCl setup locally. (Well, kinda working -- lots of tests are
failing, but I can compile and run simple prorgams.)
Find lots of nitpicks below. The one significant change that would make this
much nicer would be to relocate go_nacl_{amd64p32,386}_exec to bin.
https://codereview.appspot.com/75080043/diff/60001/misc/nacl/README
File misc/nacl/README (right):
https://codereview.appspot.com/75080043/diff/60001/misc/nacl/README#newcode11
misc/nacl/README:11: For background it is recommended that you read this
introductory document http://golang.org/s/go13nacl.
s/this introductory document//
For users of NaCl (not folks hacking on the toolchain), that document might be
overkill.
https://codereview.appspot.com/75080043/diff/60001/misc/nacl/README#newcode18
misc/nacl/README:18: The NaCl distribution comes with an installer which ensures
you have access to the latest versions of the runtime. These versions track the
Chome numbering scheme. At the time of writing, version 33 is the stable
version.
I'd cut the last sentence (about version 33); that's covered in the next
section.
https://codereview.appspot.com/75080043/diff/60001/misc/nacl/README#newcode22
misc/nacl/README:22: Download the nacl_sdk.zip file from
https://developers.google.com/native-client/dev/sdk/download, and unpack it. I
chose /opt/nacl_sdk
s/the//
https://codereview.appspot.com/75080043/diff/60001/misc/nacl/README#newcode28
misc/nacl/README:28: % cd /opt/nack_sdk
s/nack_sdk/nacl_sdk/
https://codereview.appspot.com/75080043/diff/60001/misc/nacl/README#newcode29
misc/nacl/README:29: % ./nacksdk update
s/nacksdk/naclsdk/
https://codereview.appspot.com/75080043/diff/60001/misc/nacl/README#newcode31
misc/nacl/README:31: At this time pepper_33 is the stable version, if nacksdk
downloads a later version, please adjust accordingly.
s/, if/. If/
s/nacksdk/naclsdk/
https://codereview.appspot.com/75080043/diff/60001/misc/nacl/README#newcode33
misc/nacl/README:33: The cmd/go helper scripts expect that the runtime loader,
sel_ldr_x86_{32,64} are in your path. I find it easiest to make a symlink from
the NaCl distribution to my $GOPATH/bin directory.
s/loader/loaders/
https://codereview.appspot.com/75080043/diff/60001/misc/nacl/README#newcode43
misc/nacl/README:43: % ln -nfs $HOME/go/misc/nacl/go_nacl_amd64p32_exec
$GOPATH/bin/go_nacl_amd64p32_exec
The reader has to guess that $HOME/go is the path to the toolchain. Might be
nice to make that explicit.
Better yet, could go_nacl_{386,amd64p32}_exec live in $GOROOT/bin? Then the
usual make.bash banner "Installed commands in $GOROOT/bin" covers these helper
scripts.
https://codereview.appspot.com/75080043/diff/60001/misc/nacl/README#newcode46
misc/nacl/README:46: Pay close attention to the name, this is drawn from the
GOOS and GOARCH you build NaCl with.
This could be clearer. Maybe something more like "The exec script's name has a
special format (go_{GOOS}_{GOARCH}_exec) so that the cmd/go tool can find it."
And perhaps move this detail down near where it is used (executing a NaCl
binary).
https://codereview.appspot.com/75080043/diff/60001/misc/nacl/README#newcode53
misc/nacl/README:53: Using the support scripts you linked in above, the cmd/go
tool knows if the GOOS is set to `nacl` it should not try to execute any
binaries itself, but passes their execution to support scripts which set up an
Native Client environment and invoke the NaCl sandbox.
s/knows if/knows that if/
s/, but/. Instead, it/
https://codereview.appspot.com/75080043/diff/60001/misc/nacl/README#newcode57
misc/nacl/README:57: # building Go
s/building/build/ here and below
Perhaps s/Go/Go toolchain/ here and below
https://codereview.appspot.com/75080043/diff/60001/misc/nacl/README#newcode69
misc/nacl/README:69: % env GOOS=nacl GOARCH=amd64p32 go build $COMMAND
Maybe add another line here:
% go_nacl_amd64p32_exec $COMMAND
and use `go build $COMMAND.go` above instead? Also, perhaps change the title to
include "running Go NaCl binaries"?
Hmm, probably not a good idea, but reading the above makes me kinda want `go
build $COMMAND.go` to spit out two files: $COMMAND.nacl (binary) and $COMMAND
(platform-appropriate shell script that invokes $COMMAND.nacl using the correct
wrapper). And `go install` would copy both into $GOPATH/bin.
https://codereview.appspot.com/75080043/diff/60001/misc/nacl/README#newcode71
misc/nacl/README:71: # running GO programs
s/GO/Go/
https://codereview.appspot.com/75080043/diff/60001/misc/nacl/go_nacl_386_exec
File misc/nacl/go_nacl_386_exec (right):
https://codereview.appspot.com/75080043/diff/60001/misc/nacl/go_nacl_386_exec...
misc/nacl/go_nacl_386_exec:6: export NACLENV_GOOS=$GOOS
The amd64 version has:
export NACLENV_GOROOT=/go
export NACLENV_NACLPWD=$(pwd | sed "s;$GOROOT;/go;")
Are these not needed for 386?
https://codereview.appspot.com/75080043/diff/60001/misc/nacl/go_nacl_386_exec...
misc/nacl/go_nacl_386_exec:8: exec sel_ldr_x86_32 -l /dev/null -e "$@"
The amd64 version uses -S. Is this not needed for 386?
Sign in to reply to this message.
https://codereview.appspot.com/75080043/diff/60001/misc/nacl/go_nacl_amd64p32... File misc/nacl/go_nacl_amd64p32_exec (right): https://codereview.appspot.com/75080043/diff/60001/misc/nacl/go_nacl_amd64p32... misc/nacl/go_nacl_amd64p32_exec:10: exec sel_ldr_x86_64 -l /dev/null -S -e "$@" What is the '-e' here? I cannot find that in the help from sel_ldr_x86_64. Should it be '-f'?
Sign in to reply to this message.
Hello golang-codereviews@googlegroups.com, josharian@gmail.com, dan.kortschak@adelaide.edu.au (cc: golang-codereviews@googlegroups.com), Please take another look.
Sign in to reply to this message.
Josh, Dan, thanks for your detailed review. PTAL https://codereview.appspot.com/75080043/diff/60001/misc/nacl/README File misc/nacl/README (right): https://codereview.appspot.com/75080043/diff/60001/misc/nacl/README#newcode11 misc/nacl/README:11: For background it is recommended that you read this introductory document http://golang.org/s/go13nacl. On 2014/03/17 22:28:26, josharian wrote: > s/this introductory document// > > For users of NaCl (not folks hacking on the toolchain), that document might be > overkill. Done. https://codereview.appspot.com/75080043/diff/60001/misc/nacl/README#newcode18 misc/nacl/README:18: The NaCl distribution comes with an installer which ensures you have access to the latest versions of the runtime. These versions track the Chome numbering scheme. At the time of writing, version 33 is the stable version. On 2014/03/17 22:28:26, josharian wrote: > I'd cut the last sentence (about version 33); that's covered in the next > section. Done. https://codereview.appspot.com/75080043/diff/60001/misc/nacl/README#newcode22 misc/nacl/README:22: Download the nacl_sdk.zip file from https://developers.google.com/native-client/dev/sdk/download, and unpack it. I chose /opt/nacl_sdk On 2014/03/17 22:28:26, josharian wrote: > s/the// Done. https://codereview.appspot.com/75080043/diff/60001/misc/nacl/README#newcode28 misc/nacl/README:28: % cd /opt/nack_sdk On 2014/03/17 22:28:26, josharian wrote: > s/nack_sdk/nacl_sdk/ wow, such typo https://codereview.appspot.com/75080043/diff/60001/misc/nacl/README#newcode28 misc/nacl/README:28: % cd /opt/nack_sdk On 2014/03/17 22:28:26, josharian wrote: > s/nack_sdk/nacl_sdk/ Done. https://codereview.appspot.com/75080043/diff/60001/misc/nacl/README#newcode29 misc/nacl/README:29: % ./nacksdk update On 2014/03/17 22:28:26, josharian wrote: > s/nacksdk/naclsdk/ Done. https://codereview.appspot.com/75080043/diff/60001/misc/nacl/README#newcode31 misc/nacl/README:31: At this time pepper_33 is the stable version, if nacksdk downloads a later version, please adjust accordingly. On 2014/03/17 22:28:26, josharian wrote: > s/, if/. If/ > s/nacksdk/naclsdk/ Done. https://codereview.appspot.com/75080043/diff/60001/misc/nacl/README#newcode33 misc/nacl/README:33: The cmd/go helper scripts expect that the runtime loader, sel_ldr_x86_{32,64} are in your path. I find it easiest to make a symlink from the NaCl distribution to my $GOPATH/bin directory. On 2014/03/17 22:28:26, josharian wrote: > s/loader/loaders/ Done. https://codereview.appspot.com/75080043/diff/60001/misc/nacl/README#newcode43 misc/nacl/README:43: % ln -nfs $HOME/go/misc/nacl/go_nacl_amd64p32_exec $GOPATH/bin/go_nacl_amd64p32_exec On 2014/03/17 22:28:26, josharian wrote: > The reader has to guess that $HOME/go is the path to the toolchain. Might be > nice to make that explicit. I tried to avoid using the word $GOROOT, but it didn't make things clearer. I've tried a different want now > > Better yet, could go_nacl_{386,amd64p32}_exec live in $GOROOT/bin? Then the > usual make.bash banner "Installed commands in $GOROOT/bin" covers these helper > scripts. Possibly. I'd like to get them into the tree first, so everyone can use them, then we can move them from there. https://codereview.appspot.com/75080043/diff/60001/misc/nacl/README#newcode46 misc/nacl/README:46: Pay close attention to the name, this is drawn from the GOOS and GOARCH you build NaCl with. On 2014/03/17 22:28:26, josharian wrote: > This could be clearer. Maybe something more like "The exec script's name has a > special format (go_{GOOS}_{GOARCH}_exec) so that the cmd/go tool can find it." > And perhaps move this detail down near where it is used (executing a NaCl > binary). Excellent suggestion. https://codereview.appspot.com/75080043/diff/60001/misc/nacl/README#newcode53 misc/nacl/README:53: Using the support scripts you linked in above, the cmd/go tool knows if the GOOS is set to `nacl` it should not try to execute any binaries itself, but passes their execution to support scripts which set up an Native Client environment and invoke the NaCl sandbox. On 2014/03/17 22:28:26, josharian wrote: > s/knows if/knows that if/ > s/, but/. Instead, it/ Done. https://codereview.appspot.com/75080043/diff/60001/misc/nacl/README#newcode53 misc/nacl/README:53: Using the support scripts you linked in above, the cmd/go tool knows if the GOOS is set to `nacl` it should not try to execute any binaries itself, but passes their execution to support scripts which set up an Native Client environment and invoke the NaCl sandbox. On 2014/03/17 22:28:26, josharian wrote: > s/knows if/knows that if/ > s/, but/. Instead, it/ Done. https://codereview.appspot.com/75080043/diff/60001/misc/nacl/README#newcode57 misc/nacl/README:57: # building Go On 2014/03/17 22:28:26, josharian wrote: > s/building/build/ here and below > > Perhaps s/Go/Go toolchain/ here and below Done. https://codereview.appspot.com/75080043/diff/60001/misc/nacl/README#newcode69 misc/nacl/README:69: % env GOOS=nacl GOARCH=amd64p32 go build $COMMAND On 2014/03/17 22:28:26, josharian wrote: > Maybe add another line here: > > % go_nacl_amd64p32_exec $COMMAND > > and use `go build $COMMAND.go` above instead? Also, perhaps change the title to > include "running Go NaCl binaries"? Done > > > Hmm, probably not a good idea, but reading the above makes me kinda want `go > build $COMMAND.go` to spit out two files: $COMMAND.nacl (binary) and $COMMAND > (platform-appropriate shell script that invokes $COMMAND.nacl using the correct > wrapper). And `go install` would copy both into $GOPATH/bin. hmm, yeah, I don't want to write cheques like that https://codereview.appspot.com/75080043/diff/60001/misc/nacl/README#newcode71 misc/nacl/README:71: # running GO programs On 2014/03/17 22:28:26, josharian wrote: > s/GO/Go/ Done. https://codereview.appspot.com/75080043/diff/60001/misc/nacl/go_nacl_386_exec File misc/nacl/go_nacl_386_exec (right): https://codereview.appspot.com/75080043/diff/60001/misc/nacl/go_nacl_386_exec... misc/nacl/go_nacl_386_exec:6: export NACLENV_GOOS=$GOOS On 2014/03/17 22:28:26, josharian wrote: > The amd64 version has: > > export NACLENV_GOROOT=/go > export NACLENV_NACLPWD=$(pwd | sed "s;$GOROOT;/go;") > > Are these not needed for 386? They probably are, i've added them for completeness. https://codereview.appspot.com/75080043/diff/60001/misc/nacl/go_nacl_386_exec... misc/nacl/go_nacl_386_exec:8: exec sel_ldr_x86_32 -l /dev/null -e "$@" On 2014/03/17 22:28:26, josharian wrote: > The amd64 version uses -S. Is this not needed for 386? -S enables signal handling, i've added that missing flag. https://codereview.appspot.com/75080043/diff/60001/misc/nacl/go_nacl_amd64p32... File misc/nacl/go_nacl_amd64p32_exec (right): https://codereview.appspot.com/75080043/diff/60001/misc/nacl/go_nacl_amd64p32... misc/nacl/go_nacl_amd64p32_exec:10: exec sel_ldr_x86_64 -l /dev/null -S -e "$@" On 2014/03/19 00:30:02, kortschak wrote: > What is the '-e' here? I cannot find that in the help from sel_ldr_x86_64. > Should it be '-f'? I think these instructions were taken from pepper_27 last year. Possibly the need to be updated, but for the moment -e appears to work ** update ** I checked pepper_27 and there is no sign of -e either. Let's see what rsc says
Sign in to reply to this message.
On 21/03/2014, at 8:50 PM, "dave@cheney.net" <dave@cheney.net> wrote: > https://codereview.appspot.com/75080043/diff/60001/misc/nacl/go_nacl_386_exec... > misc/nacl/go_nacl_386_exec:8: exec sel_ldr_x86_32 -l /dev/null -e "$@" > On 2014/03/17 22:28:26, josharian wrote: >> The amd64 version uses -S. Is this not needed for 386? > > -S enables signal handling, i've added that missing flag. In the NaCl docs it says this does work for Windows. Presumably it's just ignored on that platform. What are the implications of this? > https://codereview.appspot.com/75080043/diff/60001/misc/nacl/go_nacl_amd64p32... > File misc/nacl/go_nacl_amd64p32_exec (right): > > https://codereview.appspot.com/75080043/diff/60001/misc/nacl/go_nacl_amd64p32... > misc/nacl/go_nacl_amd64p32_exec:10: exec sel_ldr_x86_64 -l /dev/null -S > -e "$@" > On 2014/03/19 00:30:02, kortschak wrote: >> What is the '-e' here? I cannot find that in the help from > sel_ldr_x86_64. >> Should it be '-f'? > > I think these instructions were taken from pepper_27 last year. Possibly > the need to be updated, but for the moment -e appears to work > > ** update ** I checked pepper_27 and there is no sign of -e either. > Let's see what rsc says Agreed. I tried using -f instead of -e and that doesn't work. Maybe it's documentation lag in NaCl.
Sign in to reply to this message.
s/does/doesn't/ On 22/03/2014, at 7:18 AM, "Dan Kortschak" <dan.kortschak@adelaide.edu.au> wrote: > In the NaCl docs it says this does work for Windows.
Sign in to reply to this message.
LGTM for words, with nits It'd be good to get confirmation that this works on Windows. Thanks again for writing this up. https://codereview.appspot.com/75080043/diff/80001/misc/nacl/README File misc/nacl/README (right): https://codereview.appspot.com/75080043/diff/80001/misc/nacl/README#newcode49 misc/nacl/README:49: Building for NaCl is similar to cross compiling for other platforms, however as it is not possible to ever build in a `native` NaCl environment the cmd/go tool has been enhanced to allow the full build, all.bash, to be executed, rather than the compile stage, make.bash. s/, however/. However,/ s/environment the/environment, the/ s/rather than the/rather than just the/ https://codereview.appspot.com/75080043/diff/80001/misc/nacl/README#newcode62 misc/nacl/README:62: # Testing the Go toolchain. s/Testing/Test/ and the same with "Building", "Running", and "Running" below
Sign in to reply to this message.
Hello golang-codereviews@googlegroups.com, josharian@gmail.com, dan.kortschak@adelaide.edu.au (cc: golang-codereviews@googlegroups.com), Please take another look.
Sign in to reply to this message.
https://codereview.appspot.com/75080043/diff/100001/misc/nacl/README File misc/nacl/README (right): https://codereview.appspot.com/75080043/diff/100001/misc/nacl/README#newcode67 misc/nacl/README:67: # Build Go programs This section was what got me originally confused when I was working on the go.tools/playground/socket CL. It reads as though the object created by go build is a runnable binary. Two possible alternatives: note that objects must be run using the relevant go_nacl_*_exec script as below, or just merge these two sections in some sensible way. https://codereview.appspot.com/75080043/diff/100001/misc/nacl/README#newcode75 misc/nacl/README:75: # Run Go NaCl binaries (alternate) s/alternate/alternative/ but see comment above.
Sign in to reply to this message.
Sorry, missed this in previous. https://codereview.appspot.com/75080043/diff/100001/misc/nacl/README File misc/nacl/README (right): https://codereview.appspot.com/75080043/diff/100001/misc/nacl/README#newcode49 misc/nacl/README:49: Building for NaCl is similar to cross compiling for other platforms. However, as it is not possible to ever build in a `native` NaCl environment, the cmd/go tool has been enhanced to allow the full build, all.bash, to be executed, rather than just Gthe compile stage, make.bash. s/Gthe/the/
Sign in to reply to this message.
Hello golang-codereviews@googlegroups.com, josharian@gmail.com, dan.kortschak@adelaide.edu.au (cc: golang-codereviews@googlegroups.com), Please take another look.
Sign in to reply to this message.
Thank you for your comments. I'll aim to submit this on Monday if there is no further discussion. This CL is just a starting point, I fully expect everything in it to be superseded by the 1.3 release. https://codereview.appspot.com/75080043/diff/80001/misc/nacl/README File misc/nacl/README (right): https://codereview.appspot.com/75080043/diff/80001/misc/nacl/README#newcode49 misc/nacl/README:49: Building for NaCl is similar to cross compiling for other platforms, however as it is not possible to ever build in a `native` NaCl environment the cmd/go tool has been enhanced to allow the full build, all.bash, to be executed, rather than the compile stage, make.bash. On 2014/03/22 00:18:04, josharian wrote: > s/, however/. However,/ > s/environment the/environment, the/ > s/rather than the/rather than just the/ Done. https://codereview.appspot.com/75080043/diff/80001/misc/nacl/README#newcode62 misc/nacl/README:62: # Testing the Go toolchain. On 2014/03/22 00:18:04, josharian wrote: > s/Testing/Test/ and the same with "Building", "Running", and "Running" below Done. https://codereview.appspot.com/75080043/diff/100001/misc/nacl/README File misc/nacl/README (right): https://codereview.appspot.com/75080043/diff/100001/misc/nacl/README#newcode49 misc/nacl/README:49: Building for NaCl is similar to cross compiling for other platforms. However, as it is not possible to ever build in a `native` NaCl environment, the cmd/go tool has been enhanced to allow the full build, all.bash, to be executed, rather than just Gthe compile stage, make.bash. On 2014/03/22 22:56:39, kortschak wrote: > s/Gthe/the/ Done. https://codereview.appspot.com/75080043/diff/100001/misc/nacl/README#newcode67 misc/nacl/README:67: # Build Go programs At this point I'm inclined to delete everything past the first two points.
Sign in to reply to this message.
Hello golang-codereviews@googlegroups.com, josharian@gmail.com, dan.kortschak@adelaide.edu.au (cc: golang-codereviews@googlegroups.com), Please take another look.
Sign in to reply to this message.
LGTM with hopes for extension in a later CL. On 2014/03/23 00:03:50, dfc wrote: > Hello mailto:golang-codereviews@googlegroups.com, mailto:josharian@gmail.com, > mailto:dan.kortschak@adelaide.edu.au (cc: mailto:golang-codereviews@googlegroups.com), > > Please take another look.
Sign in to reply to this message.
> > LGTM with hopes for extension in a later CL. > Same, we gotta start somewhere, but I don't know what the destination looks like yet.
Sign in to reply to this message.
*** Submitted as https://code.google.com/p/go/source/detail?r=61133dcaba23 *** misc/nacl: add Native Client support scripts and documentation LGTM=josharian, dan.kortschak R=golang-codereviews, josharian, dan.kortschak CC=golang-codereviews https://codereview.appspot.com/75080043
Sign in to reply to this message.
Message was sent while issue was closed.
This CL appears to have broken the darwin-386 builder. See http://build.golang.org/log/ebce514ff66a695341184876dade4eda59c0f212
Sign in to reply to this message.
Lies! OSX 10.6 just can't handle its file descriptors. > On 24 Mar 2014, at 12:48, gobot@golang.org wrote: > > This CL appears to have broken the darwin-386 builder. > See http://build.golang.org/log/ebce514ff66a695341184876dade4eda59c0f212 > > https://codereview.appspot.com/75080043/
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
