code review 6920055: go-tour/gotour: find html root through running director...

go-tour/gotour: find html root through running directory if package lookup fails.

[francesc, 2012-12-12 19:29:43]

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

I'd like you to review this change to
https://code.google.com/p/go-tour/

[francesc, 2012-12-13 02:40:44]

On 2012/12/12 19:29:43, gocampoy wrote:
> Hello mailto:adg@golang.org (cc: mailto:golang-dev@googlegroups.com),
> 
> I'd like you to review this change to
> https://code.google.com/p/go-tour/

PTAL

genzip.sh generates gotour.zip containing a directory structure that allows
running "go run run.go" in the decompressed folder and everything works.

I think that this is pretty much what we were trying to do, but let me know how
you feel about it.

[adg, 2012-12-13 06:02:18]

https://codereview.appspot.com/6920055/diff/3005/gotour/local.go
File gotour/local.go (right):

https://codereview.appspot.com/6920055/diff/3005/gotour/local.go#newcode45
gotour/local.go:45: func findServerRoot() (string, error) {
it's not really a server root, it's the tour root

findRoot() is probably fine

https://codereview.appspot.com/6920055/diff/3005/gotour/local.go#newcode46
gotour/local.go:46: isRoot := func(p string) bool {
s/isRoot/exists/

and make it a top-level function

https://codereview.appspot.com/6920055/diff/3005/gotour/local.go#newcode59
gotour/local.go:59: return "", fmt.Errorf("Couldn't find server root: %v", err)
return "", err

this fmt.Errorf adds nothing

https://codereview.appspot.com/6920055/diff/3005/run.go
File run.go (right):

https://codereview.appspot.com/6920055/diff/3005/run.go#newcode19
run.go:19: if err := os.Setenv("GOPATH", wd); err != nil {
we should only set mutate GOPATH for the user's processes, not for the gotour
process itself

and we should only do that if the user doesn't have GOPATH set or if the tour
packages are not inside that GOPATH

https://codereview.appspot.com/6920055/diff/3005/run.go#newcode35
run.go:35: run("go", "build", "code.google.com/p/go-tour/gotour")
why build the gotour here? what's the purpose of this file?

I suggested the gotour binary itself might build the dependent packages on
startup. But there's no point in providing a separate bootstrap binary to build
the gotour binary itself.

[adg, 2012-12-13 06:03:43]

https://codereview.appspot.com/6920055/diff/3005/run.go
File run.go (right):

https://codereview.appspot.com/6920055/diff/3005/run.go#newcode35
run.go:35: run("go", "build", "code.google.com/p/go-tour/gotour")
Ah, I see. You want the user to run "go run run.go"? I think we should just give
them gotour.exe or whatever.

[francesc, 2012-12-13 20:56:48]

Let's discuss this live

https://codereview.appspot.com/6920055/diff/3005/gotour/local.go
File gotour/local.go (right):

https://codereview.appspot.com/6920055/diff/3005/gotour/local.go#newcode45
gotour/local.go:45: func findServerRoot() (string, error) {
On 2012/12/13 06:02:18, adg wrote:
> it's not really a server root, it's the tour root
> 
> findRoot() is probably fine

Done.

https://codereview.appspot.com/6920055/diff/3005/gotour/local.go#newcode46
gotour/local.go:46: isRoot := func(p string) bool {
On 2012/12/13 06:02:18, adg wrote:
> s/isRoot/exists/
> 
> and make it a top-level function

Done.

https://codereview.appspot.com/6920055/diff/3005/gotour/local.go#newcode59
gotour/local.go:59: return "", fmt.Errorf("Couldn't find server root: %v", err)
On 2012/12/13 06:02:18, adg wrote:
> return "", err
> 
> this fmt.Errorf adds nothing

Done.

https://codereview.appspot.com/6920055/diff/3005/run.go
File run.go (right):

https://codereview.appspot.com/6920055/diff/3005/run.go#newcode19
run.go:19: if err := os.Setenv("GOPATH", wd); err != nil {
On 2012/12/13 06:02:18, adg wrote:
> we should only set mutate GOPATH for the user's processes, not for the gotour
> process itself
> 
> and we should only do that if the user doesn't have GOPATH set or if the tour
> packages are not inside that GOPATH

I agree with doing it only when the user doesn't have a GOPATH set, but if we
don't set the GOPATH for the gotour itself, how can it compile any code?

Let's discuss this and the rest of comments in the file live

[francesc, 2012-12-14 00:17:14]

PTAL

Now genzip.sh generates a zip containing the executable gotour binary. If GOPATH
is not set it will be set to the current directory by the gotour

On 2012/12/13 20:56:48, gocampoy wrote:
> Let's discuss this live
> 
> https://codereview.appspot.com/6920055/diff/3005/gotour/local.go
> File gotour/local.go (right):
> 
> https://codereview.appspot.com/6920055/diff/3005/gotour/local.go#newcode45
> gotour/local.go:45: func findServerRoot() (string, error) {
> On 2012/12/13 06:02:18, adg wrote:
> > it's not really a server root, it's the tour root
> > 
> > findRoot() is probably fine
> 
> Done.
> 
> https://codereview.appspot.com/6920055/diff/3005/gotour/local.go#newcode46
> gotour/local.go:46: isRoot := func(p string) bool {
> On 2012/12/13 06:02:18, adg wrote:
> > s/isRoot/exists/
> > 
> > and make it a top-level function
> 
> Done.
> 
> https://codereview.appspot.com/6920055/diff/3005/gotour/local.go#newcode59
> gotour/local.go:59: return "", fmt.Errorf("Couldn't find server root: %v",
err)
> On 2012/12/13 06:02:18, adg wrote:
> > return "", err
> > 
> > this fmt.Errorf adds nothing
> 
> Done.
> 
> https://codereview.appspot.com/6920055/diff/3005/run.go
> File run.go (right):
> 
> https://codereview.appspot.com/6920055/diff/3005/run.go#newcode19
> run.go:19: if err := os.Setenv("GOPATH", wd); err != nil {
> On 2012/12/13 06:02:18, adg wrote:
> > we should only set mutate GOPATH for the user's processes, not for the
gotour
> > process itself
> > 
> > and we should only do that if the user doesn't have GOPATH set or if the
tour
> > packages are not inside that GOPATH
> 
> I agree with doing it only when the user doesn't have a GOPATH set, but if we
> don't set the GOPATH for the gotour itself, how can it compile any code?
> 
> Let's discuss this and the rest of comments in the file live

[minux1, 2012-12-14 09:05:00]

On 2012/12/14 00:17:14, gocampoy wrote:
> Now genzip.sh generates a zip containing the executable gotour binary. If
GOPATH
> is not set it will be set to the current directory by the gotour
are we going to provide official binary distributions for go-tour?

[francesc, 2012-12-14 17:46:49]

You're guessing correctly :-)

We think that providing an easy way to tun the go-tour, without requiring
anything else than installing Go, will help the newcomers to the language.

Cheers,


On Fri, Dec 14, 2012 at 1:05 AM, <minux.ma@gmail.com> wrote:

> On 2012/12/14 00:17:14, gocampoy wrote:
>
>> Now genzip.sh generates a zip containing the executable gotour binary.
>>
> If GOPATH
>
>> is not set it will be set to the current directory by the gotour
>>
> are we going to provide official binary distributions for go-tour?
>
>
https://codereview.appspot.**com/6920055/<https://codereview.appspot.com/6920...
>



-- 
--
Francesc

[minux1, 2012-12-14 17:50:05]

On Sat, Dec 15, 2012 at 1:46 AM, Francesc Campoy Flores
<campoy@golang.org>wrote:

> We think that providing an easy way to tun the go-tour, without requiring
> anything else than installing Go, will help the newcomers to the language.
>
I think we can bundle a copy of go-tour in official Go binary
distributions, and point
to it in the documentation for the binary distribution.
Won't take much space, and will be a huge help for newcomers.

[adg, 2012-12-16 23:08:36]

That's a really great idea minux. The next step would be to modify the dist
tool to include the tour when creating the bundle.

Maybe we could just do that instead of writing this script?


On 15 December 2012 04:49, minux <minux.ma@gmail.com> wrote:

>
> On Sat, Dec 15, 2012 at 1:46 AM, Francesc Campoy Flores <campoy@golang.org
> > wrote:
>
>> We think that providing an easy way to tun the go-tour, without requiring
>> anything else than installing Go, will help the newcomers to the language.
>>
> I think we can bundle a copy of go-tour in official Go binary
> distributions, and point
> to it in the documentation for the binary distribution.
> Won't take much space, and will be a huge help for newcomers.
>

[minux1, 2012-12-17 17:10:30]

On Mon, Dec 17, 2012 at 7:08 AM, Andrew Gerrand <adg@golang.org> wrote:

> That's a really great idea minux. The next step would be to modify the
> dist tool to include the tour when creating the bundle.
>
where do you want go-tour to live in the binary distribution?

[adg, 2012-12-17 23:04:32]

On 18 December 2012 04:10, minux <minux.ma@gmail.com> wrote:

>
> On Mon, Dec 17, 2012 at 7:08 AM, Andrew Gerrand <adg@golang.org> wrote:
>
>> That's a really great idea minux. The next step would be to modify the
>> dist tool to include the tour when creating the bundle.
>>
> where do you want go-tour to live in the binary distribution?
>

Let's take this to a separate discussion thread.

[adg, 2013-01-10 23:32:13]

This is supposed to complement our packaging of the tour inside
$GOROOT/misc/tour, right? As per this CL https://codereview.appspot.com/6976045/
?

It seems that when running "go tool tour" the content won't be in the current
directory.

Instead, this CL should change the tour binary to 
1. try to find the tour in GOPATH (as it does already), 
2. if not found, look inside filepath.Join(runtime.GOROOT(), "misc", "tour") for
the static content, and set a temporary GOPATH that points to that same
directory when calling "go build".

[francesc, 2013-01-24 01:03:32]

PTAL

I modified the code to take into account your CL. I have tested it with CL
https://codereview.appspot.com/6976045 and everything works fine.

On 2013/01/10 23:32:13, adg wrote:
> This is supposed to complement our packaging of the tour inside
> $GOROOT/misc/tour, right? As per this CL
https://codereview.appspot.com/6976045/
> ?
> 
> It seems that when running "go tool tour" the content won't be in the current
> directory.
> 
> Instead, this CL should change the tour binary to 
> 1. try to find the tour in GOPATH (as it does already), 
> 2. if not found, look inside filepath.Join(runtime.GOROOT(), "misc", "tour")
for
> the static content, and set a temporary GOPATH that points to that same
> directory when calling "go build".

[adg, 2013-01-24 01:54:52]

https://codereview.appspot.com/6920055/diff/31002/gotour/local.go
File gotour/local.go (right):

https://codereview.appspot.com/6920055/diff/31002/gotour/local.go#newcode46
gotour/local.go:46: // Set GOPATH to the current directory if it wasn't set yet.
I don't think we should just override GOPATH. We should be more specific about
what's going on.

Only set the GOPATH variable in the environment when invoking the Go tool in
run(). (This means you'll need to add an env []string argument to the run
function.)

Introduce a global variable:

var gopath = os.Getenv("GOPATH")

And when running "go build", pass in the GO* environment variables to cmd.Env,
with GOPATH overridden by the gopath global.

Then findRoot should use its own version of build.Context to override GOPATH
when searching for the package:

ctx := *build.Default // copy default context
// First just look in the user's environment.
p, err := ctx.Import(basePkg, "", build.FindOnly)
if err == nil && isRoot(p.Dir) {
    return p.Dir, nil
}
// See if the tour has been packaged with the Go distribution.
tourRoot := filepath.Join(runtime.GOROOT(), "misc", "tour")
ctx.GOPATH = tourRoot
p, err := ctx.Import(basePkg, "", build.FindOnly)
if err == nil && isRoot(p.Dir) {
    gopath = tourRoot
    return p.Dir, nil
}
return nil, errors.New("could not find go-tour content; check $GOROOT and
$GOPATH")

[francesc, 2013-01-24 17:33:40]

PTAL

https://codereview.appspot.com/6920055/diff/31002/gotour/local.go
File gotour/local.go (right):

https://codereview.appspot.com/6920055/diff/31002/gotour/local.go#newcode46
gotour/local.go:46: // Set GOPATH to the current directory if it wasn't set yet.
Done with little differences:

I return tourRoot instead of p.Dir after replacing the GOPATH in ctx. That root
directory is used to obtain the static files, p.Dir won't work.

Added environ function to handle custom environments.

QUESTION: Why not using the amended environment also for running the binary?
That way I wouldn't need to pass it as a parameter, and I don't think having a
different GOPATH should matter for the program execution.

On 2013/01/24 01:54:53, adg wrote:
> I don't think we should just override GOPATH. We should be more specific about
> what's going on.
> 
> Only set the GOPATH variable in the environment when invoking the Go tool in
> run(). (This means you'll need to add an env []string argument to the run
> function.)
> 
> Introduce a global variable:
> 
> var gopath = os.Getenv("GOPATH")
> 
> And when running "go build", pass in the GO* environment variables to cmd.Env,
> with GOPATH overridden by the gopath global.
> 
> Then findRoot should use its own version of build.Context to override GOPATH
> when searching for the package:
> 
> ctx := *build.Default // copy default context
> // First just look in the user's environment.
> p, err := ctx.Import(basePkg, "", build.FindOnly)
> if err == nil && isRoot(p.Dir) {
>     return p.Dir, nil
> }
> // See if the tour has been packaged with the Go distribution.
> tourRoot := filepath.Join(runtime.GOROOT(), "misc", "tour")
> ctx.GOPATH = tourRoot
> p, err := ctx.Import(basePkg, "", build.FindOnly)
> if err == nil && isRoot(p.Dir) {
>     gopath = tourRoot
>     return p.Dir, nil
> }
> return nil, errors.New("could not find go-tour content; check $GOROOT and
> $GOPATH")

[adg, 2013-01-24 21:48:10]

https://codereview.appspot.com/6920055/diff/40001/gotour/local.go
File gotour/local.go (right):

https://codereview.appspot.com/6920055/diff/40001/gotour/local.go#newcode50
gotour/local.go:50: func environ(replacement ...string) (env []string) {
put this function below run()

https://codereview.appspot.com/6920055/diff/40001/gotour/local.go#newcode50
gotour/local.go:50: func environ(replacement ...string) (env []string) {
I think you're right, there's no reason not to pass GOPATH through to the
running executable as well, and this function is more complex than it needs to
be.

It could be simply:

func environ() (env []string) {
for _, v := range os.Environ() {
  if !strings.HasPrefix(v, "GO") {
    continue
  }
  if strings.HasPrefix(v, "GOPATH=") {
    v = "GOPATH="+gopath
  }
  env = append(env, v)
}
return
}

https://codereview.appspot.com/6920055/diff/40001/gotour/local.go#newcode77
gotour/local.go:77: ctx := build.Default
ctx := *build.Default // make a copy

[francesc, 2013-01-24 22:05:17]

PTAL

https://codereview.appspot.com/6920055/diff/40001/gotour/local.go
File gotour/local.go (right):

https://codereview.appspot.com/6920055/diff/40001/gotour/local.go#newcode50
gotour/local.go:50: func environ(replacement ...string) (env []string) {
On 2013/01/24 21:48:10, adg wrote:
> I think you're right, there's no reason not to pass GOPATH through to the
> running executable as well, and this function is more complex than it needs to
> be.
> 
> It could be simply:
> 
> func environ() (env []string) {
> for _, v := range os.Environ() {
>   if !strings.HasPrefix(v, "GO") {
>     continue
>   }
>   if strings.HasPrefix(v, "GOPATH=") {
>     v = "GOPATH="+gopath
>   }
>   env = append(env, v)
> }
> return
> }

Done.

https://codereview.appspot.com/6920055/diff/40001/gotour/local.go#newcode50
gotour/local.go:50: func environ(replacement ...string) (env []string) {
On 2013/01/24 21:48:10, adg wrote:
> put this function below run()

Done.

https://codereview.appspot.com/6920055/diff/40001/gotour/local.go#newcode77
gotour/local.go:77: ctx := build.Default
build.Default is a Context struct, not a pointer.
Should I do a deepcopy instead? I think they way I do it does a copy already.

On 2013/01/24 21:48:10, adg wrote:
> ctx := *build.Default // make a copy

[adg, 2013-01-24 22:09:11]

LGTM

https://codereview.appspot.com/6920055/diff/40001/gotour/local.go
File gotour/local.go (right):

https://codereview.appspot.com/6920055/diff/40001/gotour/local.go#newcode77
gotour/local.go:77: ctx := build.Default
On 2013/01/24 22:05:17, gocampoy wrote:
> build.Default is a Context struct, not a pointer.
> Should I do a deepcopy instead? I think they way I do it does a copy already.

Oh yeah, I thought build.Default was a *Context, not Context. You're right.

[francesc, 2013-01-24 22:16:55]

*** Submitted as https://code.google.com/p/go-tour/source/detail?r=884c45414de8
***

go-tour/gotour: find html root through running directory if package lookup
fails.

R=adg, minux.ma
CC=golang-dev
https://codereview.appspot.com/6920055