code review 4816053: godoc: app engine configuration and updated documentation

godoc: app engine configuration and updated documentation

Also: Fixed an error message in zip.go.

[gri, 2011-07-26 21:24:32]

Hello r@golang.org (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://go.googlecode.com/hg/

[r, 2011-07-26 21:42:19]

http://codereview.appspot.com/4816053/diff/8001/src/cmd/godoc/appconfig.go
File src/cmd/godoc/appconfig.go (right):

http://codereview.appspot.com/4816053/diff/8001/src/cmd/godoc/appconfig.go#ne...
src/cmd/godoc/appconfig.go:9: package main
if you were starting from scratch i'd say don't call this main but godoc, but
given the history it's probably ok

think about changing it later, though. could help you understand tracebacks, for
instance.

another approach would be to move godoc into package godoc now, and just have a
non-appengine instance be

package main

import "godoc"

func main() {
   godoc.Main()
}

perhaps even do flag processing beforehand, and pass in the file system instance
to godoc.Main

http://codereview.appspot.com/4816053/diff/8001/src/cmd/godoc/appinit.go
File src/cmd/godoc/appinit.go (right):

http://codereview.appspot.com/4816053/diff/8001/src/cmd/godoc/appinit.go#newc...
src/cmd/godoc/appinit.go:8: // parameters in appconfig.go accordingly.
as explained elsewhere, this can be done more smoothly

http://codereview.appspot.com/4816053/diff/8001/src/cmd/godoc/appinit.go#newc...
src/cmd/godoc/appinit.go:11: // library version. To correct for version skew,
copy never
s/never/newer/ ?

http://codereview.appspot.com/4816053/diff/8001/src/cmd/godoc/doc.go
File src/cmd/godoc/doc.go (right):

http://codereview.appspot.com/4816053/diff/8001/src/cmd/godoc/doc.go#newcode113
src/cmd/godoc/doc.go:113: can be set with the -maxresults flag; if is set to 0,
no full text results are
s/ is//
or
s/ is/it is/

http://codereview.appspot.com/4816053/diff/8001/src/cmd/godoc/doc.go#newcode121
src/cmd/godoc/doc.go:121: without special options). $GOROOT (or -goroot) must be
set to the .zip file
this sentence is messy and hard to parse. rewrite

http://codereview.appspot.com/4816053/diff/8001/src/cmd/godoc/doc.go#newcode123
src/cmd/godoc/doc.go:123: For instance:
bad grammar.
For instance, if you create the zip file by the command
...
then run godoc with
...

(reversing the examples)

[dsymonds, 2011-07-26 22:59:50]

http://codereview.appspot.com/4816053/diff/8001/src/cmd/godoc/appconfig.go
File src/cmd/godoc/appconfig.go (right):

http://codereview.appspot.com/4816053/diff/8001/src/cmd/godoc/appconfig.go#ne...
src/cmd/godoc/appconfig.go:16: // zipGoroot is the absolute path of the goroot
I am confused why the zipped goroot needs to have absolute paths at all. Why
can't it just be rooted at the GOROOT?

http://codereview.appspot.com/4816053/diff/8001/src/cmd/godoc/appinit.go
File src/cmd/godoc/appinit.go (right):

http://codereview.appspot.com/4816053/diff/8001/src/cmd/godoc/appinit.go#newc...
src/cmd/godoc/appinit.go:11: // library version. To correct for version skew,
copy never
also maybe s/library version/Go release/, because it's the whole thing (language
+ libraries).

http://codereview.appspot.com/4816053/diff/8001/src/cmd/godoc/appinit.go#newc...
src/cmd/godoc/appinit.go:34: //	dev_appserver.py -a <hostname> godoc
you could s/<hostname>/0/ to get the same effect, and it'll be available at both
hostname:8080 and localhost:8080.

[gri, 2011-07-26 23:24:26]

PTAL

http://codereview.appspot.com/4816053/diff/8001/src/cmd/godoc/appconfig.go
File src/cmd/godoc/appconfig.go (right):

http://codereview.appspot.com/4816053/diff/8001/src/cmd/godoc/appconfig.go#ne...
src/cmd/godoc/appconfig.go:9: package main
On 2011/07/26 21:42:23, r wrote:
> if you were starting from scratch i'd say don't call this main but godoc, but
> given the history it's probably ok
> 
> think about changing it later, though. could help you understand tracebacks,
for
> instance.
> 
> another approach would be to move godoc into package godoc now, and just have
a
> non-appengine instance be
> 
> package main
> 
> import "godoc"
> 
> func main() {
>    godoc.Main()
> }
> 
> perhaps even do flag processing beforehand, and pass in the file system
instance
> to godoc.Main

I think the latter sounds like a good plan but requires a bit of shuffling. I
would prefer doing this in a 2nd and separate CL.

As it is, there is no changes needed to the standard godoc, and to run on app
engine one simply copies all files except for the main.go file.

http://codereview.appspot.com/4816053/diff/8001/src/cmd/godoc/appconfig.go#ne...
src/cmd/godoc/appconfig.go:16: // zipGoroot is the absolute path of the goroot
On 2011/07/26 22:59:50, dsymonds wrote:
> I am confused why the zipped goroot needs to have absolute paths at all. Why
> can't it just be rooted at the GOROOT?

It can, but the zip file may contain other file systems as well (for instance
company internal ones). Then we have several directory trees which conceptually
are all under / in the zip file. zipGoroot is the path to the directory tree
containing the Go distribution. The path is very specific to how the .zip file
was created.

http://codereview.appspot.com/4816053/diff/8001/src/cmd/godoc/appinit.go
File src/cmd/godoc/appinit.go (right):

http://codereview.appspot.com/4816053/diff/8001/src/cmd/godoc/appinit.go#newc...
src/cmd/godoc/appinit.go:8: // parameters in appconfig.go accordingly.
On 2011/07/26 21:42:23, r wrote:
> as explained elsewhere, this can be done more smoothly

Either way, a file with the .zip file specifics must be updated. And either way,
one file (main.go) must not be copied. I think the difference is only
organizational. Happy to do it, but in a separate CL.

http://codereview.appspot.com/4816053/diff/8001/src/cmd/godoc/appinit.go#newc...
src/cmd/godoc/appinit.go:11: // library version. To correct for version skew,
copy never
On 2011/07/26 21:42:23, r wrote:
> s/never/newer/ ?

Done.

http://codereview.appspot.com/4816053/diff/8001/src/cmd/godoc/appinit.go#newc...
src/cmd/godoc/appinit.go:11: // library version. To correct for version skew,
copy never
On 2011/07/26 22:59:50, dsymonds wrote:
> also maybe s/library version/Go release/, because it's the whole thing
(language
> + libraries).

Done.

http://codereview.appspot.com/4816053/diff/8001/src/cmd/godoc/appinit.go#newc...
src/cmd/godoc/appinit.go:34: //	dev_appserver.py -a <hostname> godoc
On 2011/07/26 22:59:50, dsymonds wrote:
> you could s/<hostname>/0/ to get the same effect, and it'll be available at
both
> hostname:8080 and localhost:8080.

Done.

http://codereview.appspot.com/4816053/diff/8001/src/cmd/godoc/doc.go
File src/cmd/godoc/doc.go (right):

http://codereview.appspot.com/4816053/diff/8001/src/cmd/godoc/doc.go#newcode113
src/cmd/godoc/doc.go:113: can be set with the -maxresults flag; if is set to 0,
no full text results are
On 2011/07/26 21:42:23, r wrote:
> s/ is//
> or
> s/ is/it is/

Done.

http://codereview.appspot.com/4816053/diff/8001/src/cmd/godoc/doc.go#newcode121
src/cmd/godoc/doc.go:121: without special options). $GOROOT (or -goroot) must be
set to the .zip file
On 2011/07/26 21:42:23, r wrote:
> this sentence is messy and hard to parse. rewrite

Done.

http://codereview.appspot.com/4816053/diff/8001/src/cmd/godoc/doc.go#newcode123
src/cmd/godoc/doc.go:123: For instance:
On 2011/07/26 21:42:23, r wrote:
> bad grammar.
> For instance, if you create the zip file by the command
> ...
> then run godoc with
> ...
> 
> (reversing the examples)

Done.

[dsymonds, 2011-07-26 23:32:53]

FYI

http://codereview.appspot.com/4816053/diff/8001/src/cmd/godoc/appconfig.go
File src/cmd/godoc/appconfig.go (right):

http://codereview.appspot.com/4816053/diff/8001/src/cmd/godoc/appconfig.go#ne...
src/cmd/godoc/appconfig.go:16: // zipGoroot is the absolute path of the goroot
On 2011/07/26 23:24:26, gri wrote:
> On 2011/07/26 22:59:50, dsymonds wrote:
> > I am confused why the zipped goroot needs to have absolute paths at all. Why
> > can't it just be rooted at the GOROOT?
> 
> It can, but the zip file may contain other file systems as well (for instance
> company internal ones). Then we have several directory trees which
conceptually
> are all under / in the zip file. zipGoroot is the path to the directory tree
> containing the Go distribution. The path is very specific to how the .zip file
> was created.

Okay. Can this not be "absolute", then? Can it just be the path to the goroot in
the zip file, so it could be empty for a simple case, or a path like "goroot"
for your described case?

http://codereview.appspot.com/4816053/diff/12001/src/cmd/godoc/doc.go
File src/cmd/godoc/doc.go (right):

http://codereview.appspot.com/4816053/diff/12001/src/cmd/godoc/doc.go#newcode123
src/cmd/godoc/doc.go:123: zip go.zip $HOME/go
it would be good if this worked:
  cd $HOME && zip -r go.zip go
  godoc -http=:6060 -zip=go.zip -goroot=go

so that there's no mention of $HOME in the paths.

[gri, 2011-07-27 20:39:32]

PTAL

http://codereview.appspot.com/4816053/diff/8001/src/cmd/godoc/appconfig.go
File src/cmd/godoc/appconfig.go (right):

http://codereview.appspot.com/4816053/diff/8001/src/cmd/godoc/appconfig.go#ne...
src/cmd/godoc/appconfig.go:16: // zipGoroot is the absolute path of the goroot
On 2011/07/26 23:32:53, dsymonds wrote:
> On 2011/07/26 23:24:26, gri wrote:
> > On 2011/07/26 22:59:50, dsymonds wrote:
> > > I am confused why the zipped goroot needs to have absolute paths at all.
Why
> > > can't it just be rooted at the GOROOT?
> > 
> > It can, but the zip file may contain other file systems as well (for
instance
> > company internal ones). Then we have several directory trees which
> conceptually
> > are all under / in the zip file. zipGoroot is the path to the directory tree
> > containing the Go distribution. The path is very specific to how the .zip
file
> > was created.
> 
> Okay. Can this not be "absolute", then? Can it just be the path to the goroot
in
> the zip file, so it could be empty for a simple case, or a path like "goroot"
> for your described case?

Done.

[r, 2011-07-27 21:22:31]

this is fine as far as it goes, but is your plan to turn godoc into a package as
suggested?

http://codereview.appspot.com/4816053/diff/11002/src/cmd/godoc/doc.go
File src/cmd/godoc/doc.go (right):

http://codereview.appspot.com/4816053/diff/11002/src/cmd/godoc/doc.go#newcode117
src/cmd/godoc/doc.go:117: Alternatively, a .zip file may be provided via the
-zip flag, which contains
s/Alternatively/Instead/

[gri, 2011-07-27 21:56:17]

PTAL
- gri

PS: Yes, I am planing to package godoc in a next step.

http://codereview.appspot.com/4816053/diff/11002/src/cmd/godoc/doc.go
File src/cmd/godoc/doc.go (right):

http://codereview.appspot.com/4816053/diff/11002/src/cmd/godoc/doc.go#newcode117
src/cmd/godoc/doc.go:117: Alternatively, a .zip file may be provided via the
-zip flag, which contains
On 2011/07/27 21:22:32, r wrote:
> s/Alternatively/Instead/

Done.

[r, 2011-07-27 22:03:30]

LGTM

[gri, 2011-07-27 22:14:16]

*** Submitted as http://code.google.com/p/go/source/detail?r=00a82b7b3a2d ***

godoc: app engine configuration and updated documentation

Also: Fixed an error message in zip.go.

R=r, dsymonds
CC=golang-dev
http://codereview.appspot.com/4816053