Issue 6976045: code review 6976045: go/misc: Adding go-tour to the generated packages for e... (Closed)
|
Created:
12 years, 2 months ago by francesc Modified:
12 years, 1 month ago Reviewers:
CC:
adg, minux1, golang-dev Visibility:
Public. |
Descriptiongo/misc: Adding go-tour to the generated packages for every distribution.
Patch Set 1 #Patch Set 2 : diff -r b9bee6184b4e https://code.google.com/p/go #Patch Set 3 : diff -r b9bee6184b4e https://code.google.com/p/go #
Total comments: 12
Patch Set 4 : diff -r b9bee6184b4e https://code.google.com/p/go #Patch Set 5 : diff -r b9bee6184b4e https://code.google.com/p/go #Patch Set 6 : diff -r b9bee6184b4e https://code.google.com/p/go #Patch Set 7 : diff -r b9bee6184b4e https://code.google.com/p/go #Patch Set 8 : diff -r b9bee6184b4e https://code.google.com/p/go #
Total comments: 2
Patch Set 9 : diff -r 2b24ee0f7b5e https://code.google.com/p/go #Patch Set 10 : diff -r 2b24ee0f7b5e https://code.google.com/p/go #Patch Set 11 : diff -r 2b24ee0f7b5e https://code.google.com/p/go #Patch Set 12 : diff -r 2b24ee0f7b5e https://code.google.com/p/go #Patch Set 13 : diff -r 2b24ee0f7b5e https://code.google.com/p/go #
Total comments: 18
Patch Set 14 : diff -r 9a3e56fe880c https://code.google.com/p/go #Patch Set 15 : diff -r 9a3e56fe880c https://code.google.com/p/go #Patch Set 16 : diff -r 9a3e56fe880c https://code.google.com/p/go #Patch Set 17 : diff -r 9a3e56fe880c https://code.google.com/p/go #Patch Set 18 : diff -r 9a3e56fe880c https://code.google.com/p/go #
Total comments: 11
Patch Set 19 : diff -r 9a3e56fe880c https://code.google.com/p/go #
Total comments: 2
Patch Set 20 : diff -r 9a3e56fe880c https://code.google.com/p/go #Patch Set 21 : diff -r 9a3e56fe880c https://code.google.com/p/go #Patch Set 22 : diff -r 9a3e56fe880c https://code.google.com/p/go #
Total comments: 2
Patch Set 23 : diff -r 9a3e56fe880c https://code.google.com/p/go #
Total comments: 14
Patch Set 24 : diff -r 9a3e56fe880c https://code.google.com/p/go #Patch Set 25 : diff -r fd51d6123dfa https://code.google.com/p/go #
Total comments: 2
Patch Set 26 : diff -r fd51d6123dfa https://code.google.com/p/go #Patch Set 27 : diff -r 20f79f2809b7 https://code.google.com/p/go #MessagesTotal messages: 33
Hello adg@golang.org (cc: golang-dev@googlegroups.com), I'd like you to review this change to https://code.google.com/p/go
Sign in to reply to this message.
description: use 'misc/dist' is better than 'go/misc'. the changes look good, i wonder if we need to provide some docs. pointer to the newly embedded go-tour?
Sign in to reply to this message.
https://codereview.appspot.com/6976045/diff/4001/misc/dist/bindist.go File misc/dist/bindist.go (right): https://codereview.appspot.com/6976045/diff/4001/misc/dist/bindist.go#newcode44 misc/dist/bindist.go:44: tourRepoURL = "http://code.google.com/p/go-tour" make it a flag ("-tour") just like -repo https://codereview.appspot.com/6976045/diff/4001/misc/dist/bindist.go#newcode352 misc/dist/bindist.go:352: func (b *Build) BuildTour() error { s/BuildTour/tour/g https://codereview.appspot.com/6976045/diff/4001/misc/dist/bindist.go#newcode361 misc/dist/bindist.go:361: toolPah = filepath.Join(b.root, "pkg", "tool", b.OS+"_"+b.Arch, "tour") s/toolPah/toolPath/ https://codereview.appspot.com/6976045/diff/4001/misc/dist/bindist.go#newcode374 misc/dist/bindist.go:374: {tourRoot, "mv", "tree", "pic", "wc", pkgDir}, These packages should be inside $GOROOT/src/pkg/tour/ so that they are imported as "tour/pic" etc
Sign in to reply to this message.
https://codereview.appspot.com/6976045/diff/4001/misc/dist/bindist.go File misc/dist/bindist.go (right): https://codereview.appspot.com/6976045/diff/4001/misc/dist/bindist.go#newcode372 misc/dist/bindist.go:372: {tourPkg, "go", "build"}, make it "go build -o tour" we want to be able to run "go tool tour" https://codereview.appspot.com/6976045/diff/4001/misc/dist/bindist.go#newcode373 misc/dist/bindist.go:373: {tourPkg, "mv", "gotour", toolPah}, s/gotour/tour/
Sign in to reply to this message.
On Wed, Jan 9, 2013 at 1:25 PM, <adg@golang.org> wrote: > https://codereview.appspot.**com/6976045/diff/4001/misc/** > dist/bindist.go#newcode374<https://codereview.appspot.com/6976045/diff/4001/misc/dist/bindist.go#newcode374> > misc/dist/bindist.go:374: {tourRoot, "mv", "tree", "pic", "wc", pkgDir}, > These packages should be inside $GOROOT/src/pkg/tour/ > so that they are imported as "tour/pic" etc > I wonder if we should put those packages in misc/tour/src/tour, as I doubt we want to pollute the namespace of standard library (if we put that in GOROOT, people won't be able to have a package named tour, for example). We can let go-tour detect this (perhaps by build tags) and set GOPATH before running user programs.
Sign in to reply to this message.
On 9 January 2013 20:48, minux <minux.ma@gmail.com> wrote: > I wonder if we should put those packages in misc/tour/src/tour, as I doubt > we want to pollute the namespace of standard library (if we put that in > GOROOT, > people won't be able to have a package named tour, for example). > > We can let go-tour detect this (perhaps by build tags) and set GOPATH > before > running user programs. > This came up in the discussion, and I saw no real resolution. Putting the tour packages inside $GOROOT/src/pkg pros: - they're in godoco when/if the user looks for them - therefore we can document those packages - tour binary doesn't do anything magic when building cons: - they pollute the standard library not sure if pro or con: - they can be used by non-tour programs Putting the packages inside $GOROOT/misc/tour pros: - tour content is all in one place - standard library remains pristine cons: - magic GOPATH in tour's build step - no easy way to find the docs, tricky to make godoc see them In conclusion, I don't know. Help! Andrew
Sign in to reply to this message.
On Thu, Jan 10, 2013 at 8:51 AM, Andrew Gerrand <adg@golang.org> wrote: > Putting the tour packages inside $GOROOT/src/pkg > pros: > - they're in godoco when/if the user looks for them > - therefore we can document those packages > - tour binary doesn't do anything magic when building > cons: > - they pollute the standard library > one more: the user can't define a package named tour in GOPATH, and there is no workaround for this problem (unless he deletes those packages and breaks gotour). > not sure if pro or con: > - they can be used by non-tour programs > > Putting the packages inside $GOROOT/misc/tour > pros: > - tour content is all in one place > - standard library remains pristine > cons: > - magic GOPATH in tour's build step > - no easy way to find the docs, tricky to make godoc see them > the user only needs to set GOPATH=$GOROOT/misc/tour to be able to see the docs, so I think this isn't a serious issue. (we can even start a godoc process with appropriate GOPATH setting from the gotour process and give docs link for tour/* packages; I think this solution is good enough for this case because I imagine the user would only want to see godoc for tour/* packages when using the tour) As both of these have workarounds, I prefer this one.
Sign in to reply to this message.
I agree with minux. Polluting the standard library seems a pretty bad side effect. https://codereview.appspot.com/6976045/diff/4001/misc/dist/bindist.go File misc/dist/bindist.go (right): https://codereview.appspot.com/6976045/diff/4001/misc/dist/bindist.go#newcode44 misc/dist/bindist.go:44: tourRepoURL = "http://code.google.com/p/go-tour" On 2013/01/09 05:25:31, adg wrote: > make it a flag ("-tour") just like -repo Done. https://codereview.appspot.com/6976045/diff/4001/misc/dist/bindist.go#newcode352 misc/dist/bindist.go:352: func (b *Build) BuildTour() error { On 2013/01/09 05:25:31, adg wrote: > s/BuildTour/tour/g Done. https://codereview.appspot.com/6976045/diff/4001/misc/dist/bindist.go#newcode361 misc/dist/bindist.go:361: toolPah = filepath.Join(b.root, "pkg", "tool", b.OS+"_"+b.Arch, "tour") On 2013/01/09 05:25:31, adg wrote: > s/toolPah/toolPath/ Done. https://codereview.appspot.com/6976045/diff/4001/misc/dist/bindist.go#newcode372 misc/dist/bindist.go:372: {tourPkg, "go", "build"}, On 2013/01/09 05:26:25, adg wrote: > make it "go build -o tour" > > we want to be able to run "go tool tour" Done. I was changin its name in the next command, but I prefer your way. https://codereview.appspot.com/6976045/diff/4001/misc/dist/bindist.go#newcode373 misc/dist/bindist.go:373: {tourPkg, "mv", "gotour", toolPah}, On 2013/01/09 05:26:25, adg wrote: > s/gotour/tour/ Done. https://codereview.appspot.com/6976045/diff/4001/misc/dist/bindist.go#newcode374 misc/dist/bindist.go:374: {tourRoot, "mv", "tree", "pic", "wc", pkgDir}, On 2013/01/09 05:25:31, adg wrote: > These packages should be inside $GOROOT/src/pkg/tour/ > so that they are imported as "tour/pic" etc Waiting for your confirmation on this, should I put it in the std library? or inside of misc?
Sign in to reply to this message.
On 10 Jan 2013 21:03, <campoy@golang.org> wrote: > > I agree with minux. I agree also. We can launch a godoc process when the tour starts, and change the links in the tour slides to point there. Nice. > > Polluting the standard library seems a pretty bad side effect. > > > > https://codereview.appspot.com/6976045/diff/4001/misc/dist/bindist.go > File misc/dist/bindist.go (right): > > https://codereview.appspot.com/6976045/diff/4001/misc/dist/bindist.go#newcode44 > misc/dist/bindist.go:44: tourRepoURL = > "http://code.google.com/p/go-tour" > On 2013/01/09 05:25:31, adg wrote: >> >> make it a flag ("-tour") just like -repo > > > Done. > > > https://codereview.appspot.com/6976045/diff/4001/misc/dist/bindist.go#newcode352 > misc/dist/bindist.go:352: func (b *Build) BuildTour() error { > On 2013/01/09 05:25:31, adg wrote: >> >> s/BuildTour/tour/g > > > Done. > > > https://codereview.appspot.com/6976045/diff/4001/misc/dist/bindist.go#newcode361 > misc/dist/bindist.go:361: toolPah = filepath.Join(b.root, "pkg", > "tool", b.OS+"_"+b.Arch, "tour") > On 2013/01/09 05:25:31, adg wrote: >> >> s/toolPah/toolPath/ > > > Done. > > > https://codereview.appspot.com/6976045/diff/4001/misc/dist/bindist.go#newcode372 > misc/dist/bindist.go:372: {tourPkg, "go", "build"}, > On 2013/01/09 05:26:25, adg wrote: >> >> make it "go build -o tour" > > >> we want to be able to run "go tool tour" > > > Done. I was changin its name in the next command, but I prefer your way. > > > https://codereview.appspot.com/6976045/diff/4001/misc/dist/bindist.go#newcode373 > misc/dist/bindist.go:373: {tourPkg, "mv", "gotour", toolPah}, > On 2013/01/09 05:26:25, adg wrote: >> >> s/gotour/tour/ > > > Done. > > > https://codereview.appspot.com/6976045/diff/4001/misc/dist/bindist.go#newcode374 > misc/dist/bindist.go:374: {tourRoot, "mv", "tree", "pic", "wc", pkgDir}, > On 2013/01/09 05:25:31, adg wrote: >> >> These packages should be inside $GOROOT/src/pkg/tour/ >> so that they are imported as "tour/pic" etc > > > Waiting for your confirmation on this, should I put it in the std > library? or inside of misc? > > https://codereview.appspot.com/6976045/
Sign in to reply to this message.
Implemented in combination with https://codereview.appspot.com/6920055/ Tested and it seems it works nicely, so PTAL :-) On Thu, Jan 10, 2013 at 11:25 AM, Andrew Gerrand <adg@golang.org> wrote: > > On 10 Jan 2013 21:03, <campoy@golang.org> wrote: > > > > I agree with minux. > > I agree also. We can launch a godoc process when the tour starts, and > change the links in the tour slides to point there. Nice. > > > > > Polluting the standard library seems a pretty bad side effect. > > > > > > > > https://codereview.appspot.com/6976045/diff/4001/misc/dist/bindist.go > > File misc/dist/bindist.go (right): > > > > > https://codereview.appspot.com/6976045/diff/4001/misc/dist/bindist.go#newcode44 > > misc/dist/bindist.go:44: tourRepoURL = > > "http://code.google.com/p/go-tour" > > On 2013/01/09 05:25:31, adg wrote: > >> > >> make it a flag ("-tour") just like -repo > > > > > > Done. > > > > > > > https://codereview.appspot.com/6976045/diff/4001/misc/dist/bindist.go#newcode352 > > misc/dist/bindist.go:352: func (b *Build) BuildTour() error { > > On 2013/01/09 05:25:31, adg wrote: > >> > >> s/BuildTour/tour/g > > > > > > Done. > > > > > > > https://codereview.appspot.com/6976045/diff/4001/misc/dist/bindist.go#newcode361 > > misc/dist/bindist.go:361: toolPah = filepath.Join(b.root, "pkg", > > "tool", b.OS+"_"+b.Arch, "tour") > > On 2013/01/09 05:25:31, adg wrote: > >> > >> s/toolPah/toolPath/ > > > > > > Done. > > > > > > > https://codereview.appspot.com/6976045/diff/4001/misc/dist/bindist.go#newcode372 > > misc/dist/bindist.go:372: {tourPkg, "go", "build"}, > > On 2013/01/09 05:26:25, adg wrote: > >> > >> make it "go build -o tour" > > > > > >> we want to be able to run "go tool tour" > > > > > > Done. I was changin its name in the next command, but I prefer your way. > > > > > > > https://codereview.appspot.com/6976045/diff/4001/misc/dist/bindist.go#newcode373 > > misc/dist/bindist.go:373: {tourPkg, "mv", "gotour", toolPah}, > > On 2013/01/09 05:26:25, adg wrote: > >> > >> s/gotour/tour/ > > > > > > Done. > > > > > > > https://codereview.appspot.com/6976045/diff/4001/misc/dist/bindist.go#newcode374 > > misc/dist/bindist.go:374: {tourRoot, "mv", "tree", "pic", "wc", pkgDir}, > > On 2013/01/09 05:25:31, adg wrote: > >> > >> These packages should be inside $GOROOT/src/pkg/tour/ > >> so that they are imported as "tour/pic" etc > > > > > > Waiting for your confirmation on this, should I put it in the std > > library? or inside of misc? > > > > https://codereview.appspot.com/6976045/ > -- -- Francesc
Sign in to reply to this message.
As discussed, the steps should be:
create a temporary GOPATH
call "go get code.google.com/p/go-tour/gotour" with the GOPATH environment
variable set to that temporary path
mv $GOPATH/src/code.google.com/p/go-tour/{solutions,static} $GOROOT/misc/tour
mv $GOPATH/bin/gotour $GOROOT/pkg/tool/$GOOS_$GOARCH/tour
mv $GOPATH/src/code.google.com/p/go-tour/{pic,tree,wc}
$GOROOT/misc/src/tour/code.google.com/p/go-tour/
mv $GOPATH/pkg/code.google.com/p/go-tour/{pic,tree,wc}
$GOROOT/misc/pkg/tour/code.google.com/p/go-tour/
https://codereview.appspot.com/6976045/diff/5003/misc/dist/bindist.go
File misc/dist/bindist.go (right):
https://codereview.appspot.com/6976045/diff/5003/misc/dist/bindist.go#newcode38
misc/dist/bindist.go:38: tourRepo = flag.String("tour",
"http://code.google.com/p/go-tour", "Go tour repo URL")
put this beside repo
Sign in to reply to this message.
PTAL
On 2013/01/10 23:25:33, adg wrote:
> As discussed, the steps should be:
>
> create a temporary GOPATH
>
> call "go get code.google.com/p/go-tour/gotour" with the GOPATH environment
> variable set to that temporary path
>
> mv $GOPATH/src/code.google.com/p/go-tour/{solutions,static} $GOROOT/misc/tour
>
> mv $GOPATH/bin/gotour $GOROOT/pkg/tool/$GOOS_$GOARCH/tour
>
> mv $GOPATH/src/code.google.com/p/go-tour/{pic,tree,wc}
> $GOROOT/misc/src/tour/code.google.com/p/go-tour/
>
> mv $GOPATH/pkg/code.google.com/p/go-tour/{pic,tree,wc}
> $GOROOT/misc/pkg/tour/code.google.com/p/go-tour/
>
> https://codereview.appspot.com/6976045/diff/5003/misc/dist/bindist.go
> File misc/dist/bindist.go (right):
>
>
https://codereview.appspot.com/6976045/diff/5003/misc/dist/bindist.go#newcode38
> misc/dist/bindist.go:38: tourRepo = flag.String("tour",
> "http://code.google.com/p/go-tour", "Go tour repo URL")
> put this beside repo
Sign in to reply to this message.
https://codereview.appspot.com/6976045/diff/5003/misc/dist/bindist.go File misc/dist/bindist.go (right): https://codereview.appspot.com/6976045/diff/5003/misc/dist/bindist.go#newcode38 misc/dist/bindist.go:38: tourRepo = flag.String("tour", "http://code.google.com/p/go-tour", "Go tour repo URL") On 2013/01/10 23:25:33, adg wrote: > put this beside repo Done.
Sign in to reply to this message.
Please re-read the previous mail I sent on this CL. https://codereview.appspot.com/6976045/diff/26002/misc/dist/bindist.go File misc/dist/bindist.go (right): https://codereview.appspot.com/6976045/diff/26002/misc/dist/bindist.go#newcode34 misc/dist/bindist.go:34: tourRepo = flag.String("tour", "http://code.google.com/p/go-tour", "Go tour repo import path") s/http/https/ s/import path/URL/ - there's a crucial difference https://codereview.appspot.com/6976045/diff/26002/misc/dist/bindist.go#newcod... misc/dist/bindist.go:139: fmt.Println("Work: ", work) delete https://codereview.appspot.com/6976045/diff/26002/misc/dist/bindist.go#newcod... misc/dist/bindist.go:140: // defer os.RemoveAll(work) revert https://codereview.appspot.com/6976045/diff/26002/misc/dist/bindist.go#newcod... misc/dist/bindist.go:141: b.root = filepath.Join(work, "go") also set b.gopath = work https://codereview.appspot.com/6976045/diff/26002/misc/dist/bindist.go#newcod... misc/dist/bindist.go:144: if _, err = b.run(work, "hg", "clone", "-q", *repo, b.root); err != nil { revert https://codereview.appspot.com/6976045/diff/26002/misc/dist/bindist.go#newcod... misc/dist/bindist.go:147: if _, err = b.run(b.root, "hg", "update", *tag); err != nil { revert https://codereview.appspot.com/6976045/diff/26002/misc/dist/bindist.go#newcod... misc/dist/bindist.go:372: oldpath := b.gopath delete these three lines https://codereview.appspot.com/6976045/diff/26002/misc/dist/bindist.go#newcod... misc/dist/bindist.go:377: {tmp, "hg", "clone", *tourRepo}, use go get, as the tour will soon depend on go.talks, and who knows what other dependencies we might add you should set gopath to somewhere temporary (the workdir is ideal for this), and move the specific pieces inside. the way you have it now, the tour repo is left intact except that you move the static directory out. misc/tour/code.google.com/p/go-tour should only contain pic, tree, and wc. https://codereview.appspot.com/6976045/diff/26002/misc/dist/bindist.go#newcod... misc/dist/bindist.go:379: {tourRoot, "mv", "static", miscDir}, what about solutions? https://codereview.appspot.com/6976045/diff/26002/misc/dist/bindist.go#newcod... misc/dist/bindist.go:380: {tourPkg, goBin, "build", "-o", "tour"}, go get will do this for you, and put the result at b.gopath + "/bin/gotour" https://codereview.appspot.com/6976045/diff/26002/misc/dist/bindist.go#newcod... misc/dist/bindist.go:381: {tourPkg, "mv", "tour", toolPath}, windows doesn't have a mv command, so you need to use the cp() function (defined in this file), and for copying directories write a function like func cpDir(dst, src string) error that walks the paths, makes directories, and so on. (see filepath.Walk)
Sign in to reply to this message.
While I agree with moving GOPATH to the tmp directory, we should still fix the the go-tour package so we can actually use "go get" on it. https://codereview.appspot.com/6976045/diff/26002/misc/dist/bindist.go File misc/dist/bindist.go (right): https://codereview.appspot.com/6976045/diff/26002/misc/dist/bindist.go#newcode34 misc/dist/bindist.go:34: tourRepo = flag.String("tour", "http://code.google.com/p/go-tour", "Go tour repo import path") On 2013/01/14 22:27:06, adg wrote: > s/http/https/ > s/import path/URL/ - there's a crucial difference I changed tourRepo to be the import path, but go get of code.google.com/p/go-tour won't work, since there's not go file there. Should we fix that first? https://codereview.appspot.com/6976045/diff/26002/misc/dist/bindist.go#newcod... misc/dist/bindist.go:139: fmt.Println("Work: ", work) On 2013/01/14 22:27:06, adg wrote: > delete Done. https://codereview.appspot.com/6976045/diff/26002/misc/dist/bindist.go#newcod... misc/dist/bindist.go:140: // defer os.RemoveAll(work) On 2013/01/14 22:27:06, adg wrote: > revert Done. https://codereview.appspot.com/6976045/diff/26002/misc/dist/bindist.go#newcod... misc/dist/bindist.go:144: if _, err = b.run(work, "hg", "clone", "-q", *repo, b.root); err != nil { On 2013/01/14 22:27:06, adg wrote: > revert Any good reason? The code is shorter and as readable as before, IMHO https://codereview.appspot.com/6976045/diff/26002/misc/dist/bindist.go#newcod... misc/dist/bindist.go:147: if _, err = b.run(b.root, "hg", "update", *tag); err != nil { On 2013/01/14 22:27:06, adg wrote: > revert Idem https://codereview.appspot.com/6976045/diff/26002/misc/dist/bindist.go#newcod... misc/dist/bindist.go:377: {tmp, "hg", "clone", *tourRepo}, I can do it most of that, but "go get code.google.com/p/go-tour" will fail. Also "go get code.google.com/p/go-tour/gotour" fails On 2013/01/14 22:27:06, adg wrote: > use go get, as the tour will soon depend on go.talks, and who knows what other > dependencies we might add > > you should set gopath to somewhere temporary (the workdir is ideal for this), > and move the specific pieces inside. the way you have it now, the tour repo is > left intact except that you move the static directory out. > > misc/tour/code.google.com/p/go-tour should only contain pic, tree, and wc. https://codereview.appspot.com/6976045/diff/26002/misc/dist/bindist.go#newcod... misc/dist/bindist.go:380: {tourPkg, goBin, "build", "-o", "tour"}, On 2013/01/14 22:27:06, adg wrote: > go get will do this for you, and put the result at b.gopath + "/bin/gotour" Read above.
Sign in to reply to this message.
On 15 January 2013 10:11, <campoy@golang.org> wrote: > Also "go get code.google.com/p/go-tour/**gotour<http://code.google.com/p/go-tour/gotour>" > fails > Why?
Sign in to reply to this message.
On 15 January 2013 10:13, Andrew Gerrand <adg@golang.org> wrote: > > On 15 January 2013 10:11, <campoy@golang.org> wrote: > >> Also "go get code.google.com/p/go-tour/**gotour<http://code.google.com/p/go-tour/gotour>" >> fails >> > > Why? > It works for me.
Sign in to reply to this message.
Maybe this is caused by something else? package code.google.com/p/go-tour/gotour: /var/folders/00/10xjh000h01000cxqpysvccm0043p6/T/bindist579464539/go/misc/tour/src/ code.google.com/p/go-tour exists but /var/folders/00/10xjh000h01000cxqpysvccm0043p6/T/bindist579464539/go/misc/tour/src/ code.google.com/p/go-tour/.hg does not - stale checkout? 2013/01/14 12:51:09 darwin-amd64: /var/folders/00/10xjh000h01000cxqpysvccm0043p6/T/bindist579464539/go/bin/go get code.google.com/p/go-tour/gotour: exit status 1 On Mon, Jan 14, 2013 at 11:14 PM, Andrew Gerrand <adg@golang.org> wrote: > go get code.google.com/p/go-tour/**gotour<http://code.google.com/p/go-tour/gotour> > -- -- Francesc
Sign in to reply to this message.
Well something is wrong on your end. Check that go get works outside of dist first, then check all your other assumptions. On 15 Jan 2013 09:22, "Francesc Campoy Flores" <campoy@golang.org> wrote: > Maybe this is caused by something else? > > package code.google.com/p/go-tour/gotour: > /var/folders/00/10xjh000h01000cxqpysvccm0043p6/T/bindist579464539/go/misc/tour/src/ > code.google.com/p/go-tour exists but > /var/folders/00/10xjh000h01000cxqpysvccm0043p6/T/bindist579464539/go/misc/tour/src/ > code.google.com/p/go-tour/.hg does not - stale checkout? > > 2013/01/14 12:51:09 darwin-amd64: > /var/folders/00/10xjh000h01000cxqpysvccm0043p6/T/bindist579464539/go/bin/go > get code.google.com/p/go-tour/gotour: exit status 1 > > > On Mon, Jan 14, 2013 at 11:14 PM, Andrew Gerrand <adg@golang.org> wrote: > >> go get code.google.com/p/go-tour/**gotour<http://code.google.com/p/go-tour/gotour> >> > > > > > -- > -- > Francesc >
Sign in to reply to this message.
On Tuesday, January 15, 2013 10:22:22 AM UTC+11, Francesc wrote: > > Maybe this is caused by something else? > > package code.google.com/p/go-tour/gotour: > /var/folders/00/10xjh000h01000cxqpysvccm0043p6/T/bindist579464539/go/misc/tour/src/ > code.google.com/p/go-tour exists but > /var/folders/00/10xjh000h01000cxqpysvccm0043p6/T/bindist579464539/go/misc/tour/src/ > code.google.com/p/go-tour/.hg does not - stale checkout? > > 2013/01/14 12:51:09 darwin-amd64: > /var/folders/00/10xjh000h01000cxqpysvccm0043p6/T/bindist579464539/go/bin/go > get code.google.com/p/go-tour/gotour: exit status 1 > Do you still have os.RemoveAll(work) commented out in Do? Maybe there's a stale environment hanging around.
Sign in to reply to this message.
PTAL, I tried to assess all of your comments and modified substantially the whole tour function. On 2013/01/15 10:50:27, adg wrote: > > On Tuesday, January 15, 2013 10:22:22 AM UTC+11, Francesc wrote: > > > > Maybe this is caused by something else? > > > > package code.google.com/p/go-tour/gotour: > > > /var/folders/00/10xjh000h01000cxqpysvccm0043p6/T/bindist579464539/go/misc/tour/src/ > > code.google.com/p/go-tour exists but > > > /var/folders/00/10xjh000h01000cxqpysvccm0043p6/T/bindist579464539/go/misc/tour/src/ > > code.google.com/p/go-tour/.hg does not - stale checkout? > > > > 2013/01/14 12:51:09 darwin-amd64: > > /var/folders/00/10xjh000h01000cxqpysvccm0043p6/T/bindist579464539/go/bin/go > > get code.google.com/p/go-tour/gotour: exit status 1 > > > > Do you still have os.RemoveAll(work) commented out in Do? > > Maybe there's a stale environment hanging around. >
Sign in to reply to this message.
Lookin' good. https://codereview.appspot.com/6976045/diff/32002/misc/dist/bindist.go File misc/dist/bindist.go (right): https://codereview.appspot.com/6976045/diff/32002/misc/dist/bindist.go#newcod... misc/dist/bindist.go:362: tourRel = filepath.Join(strings.Split(*tourPath, "/")...) tourRel = filepath.FromSlash(*tourPath) maybe tourPkg ? https://codereview.appspot.com/6976045/diff/32002/misc/dist/bindist.go#newcod... misc/dist/bindist.go:367: for _, dir := range []string{miscDir, tourDst} { do this after running go get https://codereview.appspot.com/6976045/diff/32002/misc/dist/bindist.go#newcod... misc/dist/bindist.go:373: _, err := b.run(b.gopath, goBin, "get", filepath.Join(*tourPath, "gotour")) this is an import path, not a filesystem path; under windows this will use the wrong slashes instead of filepath.Join(...), just do *tourPath + "/gotour" https://codereview.appspot.com/6976045/diff/32002/misc/dist/bindist.go#newcod... misc/dist/bindist.go:618: func(path string, info os.FileInfo, err error) error { avoid the indent by first defining fn := func(path ...) return filepath.Walk(src, fn) https://codereview.appspot.com/6976045/diff/32002/misc/dist/bindist.go#newcod... misc/dist/bindist.go:619: if !strings.HasPrefix(path, src) { This shouldn't be possible. Did you actually see this happen? https://codereview.appspot.com/6976045/diff/32002/misc/dist/bindist.go#newcod... misc/dist/bindist.go:630: func cpAllDir(dst, src string, dirs ...string) error { s/src/basePath/
Sign in to reply to this message.
Glad we're getting closer to an agreement here :-) PTAL https://codereview.appspot.com/6976045/diff/32002/misc/dist/bindist.go File misc/dist/bindist.go (right): https://codereview.appspot.com/6976045/diff/32002/misc/dist/bindist.go#newcod... misc/dist/bindist.go:362: tourRel = filepath.Join(strings.Split(*tourPath, "/")...) On 2013/01/15 21:12:05, adg wrote: > tourRel = filepath.FromSlash(*tourPath) > maybe tourPkg ? Done. https://codereview.appspot.com/6976045/diff/32002/misc/dist/bindist.go#newcod... misc/dist/bindist.go:367: for _, dir := range []string{miscDir, tourDst} { On 2013/01/15 21:12:05, adg wrote: > do this after running go get Done. https://codereview.appspot.com/6976045/diff/32002/misc/dist/bindist.go#newcod... misc/dist/bindist.go:373: _, err := b.run(b.gopath, goBin, "get", filepath.Join(*tourPath, "gotour")) On 2013/01/15 21:12:05, adg wrote: > this is an import path, not a filesystem path; under windows this will use the > wrong slashes > instead of filepath.Join(...), just do *tourPath + "/gotour" Done. https://codereview.appspot.com/6976045/diff/32002/misc/dist/bindist.go#newcod... misc/dist/bindist.go:619: if !strings.HasPrefix(path, src) { On 2013/01/15 21:12:05, adg wrote: > This shouldn't be possible. Did you actually see this happen? Not really, I just got "inspiration" from another filepath.Walk in this file. See line 649 https://codereview.appspot.com/6976045/diff/32002/misc/dist/bindist.go#newcod... misc/dist/bindist.go:630: func cpAllDir(dst, src string, dirs ...string) error { On 2013/01/15 21:12:05, adg wrote: > s/src/basePath/ Done.
Sign in to reply to this message.
LGTM Make sure you test it fully on at least OS X and Linux before submitting. Thanks. https://codereview.appspot.com/6976045/diff/39001/misc/dist/bindist.go File misc/dist/bindist.go (right): https://codereview.appspot.com/6976045/diff/39001/misc/dist/bindist.go#newcod... misc/dist/bindist.go:617: walk := func(path string, info os.FileInfo, err error) error { s/path/srcPath/ in this func
Sign in to reply to this message.
On 16 January 2013 10:47, <adg@golang.org> wrote: > Make sure you test it fully on at least OS X and Linux before > submitting. Thanks. > And you should also wait for the corresponding change to go-tour to be submitted.
Sign in to reply to this message.
Will test on linux tomorrow. https://codereview.appspot.com/6976045/diff/39001/misc/dist/bindist.go File misc/dist/bindist.go (right): https://codereview.appspot.com/6976045/diff/39001/misc/dist/bindist.go#newcod... misc/dist/bindist.go:617: walk := func(path string, info os.FileInfo, err error) error { On 2013/01/15 23:47:12, adg wrote: > s/path/srcPath/ in this func Done.
Sign in to reply to this message.
Works on Linux too. Did you create a CL for the change on the go-tour side? I can't find it. On 2013/01/16 00:09:33, gocampoy wrote: > Will test on linux tomorrow. > > https://codereview.appspot.com/6976045/diff/39001/misc/dist/bindist.go > File misc/dist/bindist.go (right): > > https://codereview.appspot.com/6976045/diff/39001/misc/dist/bindist.go#newcod... > misc/dist/bindist.go:617: walk := func(path string, info os.FileInfo, err error) > error { > On 2013/01/15 23:47:12, adg wrote: > > s/path/srcPath/ in this func > > Done.
Sign in to reply to this message.
https://codereview.appspot.com/6920055/ It's associated with the user "gocampoy" rather than "campoy", which might be why it was hard to find. On 17 January 2013 10:03, <campoy@golang.org> wrote: > Works on Linux too. Did you create a CL for the change on the go-tour > side? I can't find it. > > > On 2013/01/16 00:09:33, gocampoy wrote: > >> Will test on linux tomorrow. >> > > https://codereview.appspot.**com/6976045/diff/39001/misc/** >> dist/bindist.go<https://codereview.appspot.com/6976045/diff/39001/misc/dist/bindist.go> >> File misc/dist/bindist.go (right): >> > > > https://codereview.appspot.**com/6976045/diff/39001/misc/** > dist/bindist.go#newcode617<https://codereview.appspot.com/6976045/diff/39001/misc/dist/bindist.go#newcode617> > >> misc/dist/bindist.go:617: walk := func(path string, info os.FileInfo, >> > err error) > >> error { >> On 2013/01/15 23:47:12, adg wrote: >> > s/path/srcPath/ in this func >> > > Done. >> > > > > https://codereview.appspot.**com/6976045/<https://codereview.appspot.com/6976... >
Sign in to reply to this message.
PTAL https://codereview.appspot.com/6976045/diff/48002/misc/dist/bindist.go File misc/dist/bindist.go (right): https://codereview.appspot.com/6976045/diff/48002/misc/dist/bindist.go#newcod... misc/dist/bindist.go:614: stat, err := sf.Stat() Added this to make the binary "tour" runnable. https://codereview.appspot.com/6976045/diff/48002/misc/dist/bindist.go#newcod... misc/dist/bindist.go:625: if err != nil { This happened, when the src file didn't exist. In that case info was nil and the code would panic.
Sign in to reply to this message.
Just nitpicky stuff https://codereview.appspot.com/6976045/diff/52001/misc/dist/bindist.go File misc/dist/bindist.go (right): https://codereview.appspot.com/6976045/diff/52001/misc/dist/bindist.go#newcode67 misc/dist/bindist.go:67: create top level variables: var tourPackages = []string{ "pic", "tree", "wc", } var tourContent = []string{ "prog", "solutions", "static", "template", "tour.article", } as these will need to be updated from time to time. https://codereview.appspot.com/6976045/diff/52001/misc/dist/bindist.go#newcod... misc/dist/bindist.go:356: binDst = filepath.Join(b.root, "pkg", "tool", b.OS+"_"+b.Arch, "tour") I'd prefer to declare each of these variables just before the sites they are used. In the case of binDst and binSrc they're only used once and it might just be clearer not to declare the temporary variable. They could just be inlined in the cp call: // Copy gotour binary to tool directory as "tour"; invoked as "go tool tour". return cp( filepath.Join(b.root, "pkg", "tool", b.OS+"_"+b.Arch, "tour"), filepath.Join(b.gopath, "bin", "gotour"), ) https://codereview.appspot.com/6976045/diff/52001/misc/dist/bindist.go#newcod... misc/dist/bindist.go:358: miscDir = filepath.Join(b.root, "misc", "tour") s/miscDir/contentDir/ https://codereview.appspot.com/6976045/diff/52001/misc/dist/bindist.go#newcod... misc/dist/bindist.go:361: // Relative path to the tour from GOPATH it's the import path, actually https://codereview.appspot.com/6976045/diff/52001/misc/dist/bindist.go#newcod... misc/dist/bindist.go:364: tourDst = filepath.Join(miscDir, "src", tourPkg) s/tourDst/packageDir/ https://codereview.appspot.com/6976045/diff/52001/misc/dist/bindist.go#newcod... misc/dist/bindist.go:372: for _, dir := range []string{miscDir, tourDst} { Perhaps just do the MkdirAll inside cpAllDir. https://codereview.appspot.com/6976045/diff/52001/misc/dist/bindist.go#newcod... misc/dist/bindist.go:381: res := []string{"prog", "solutions", "static", "template", "tour.article"} s/res/content/
Sign in to reply to this message.
PTAL https://codereview.appspot.com/6976045/diff/52001/misc/dist/bindist.go File misc/dist/bindist.go (right): https://codereview.appspot.com/6976045/diff/52001/misc/dist/bindist.go#newcode67 misc/dist/bindist.go:67: On 2013/01/24 02:12:46, adg wrote: > create top level variables: > > var tourPackages = []string{ > "pic", > "tree", > "wc", > } > > var tourContent = []string{ > "prog", > "solutions", > "static", > "template", > "tour.article", > } > > as these will need to be updated from time to time. Done. https://codereview.appspot.com/6976045/diff/52001/misc/dist/bindist.go#newcod... misc/dist/bindist.go:356: binDst = filepath.Join(b.root, "pkg", "tool", b.OS+"_"+b.Arch, "tour") On 2013/01/24 02:12:46, adg wrote: > I'd prefer to declare each of these variables just before the sites they are > used. > In the case of binDst and binSrc they're only used once and it might just be > clearer not to declare the temporary variable. They could just be inlined in > the cp call: > > // Copy gotour binary to tool directory as "tour"; invoked as "go tool tour". > return cp( > filepath.Join(b.root, "pkg", "tool", b.OS+"_"+b.Arch, "tour"), > filepath.Join(b.gopath, "bin", "gotour"), > ) Done. https://codereview.appspot.com/6976045/diff/52001/misc/dist/bindist.go#newcod... misc/dist/bindist.go:358: miscDir = filepath.Join(b.root, "misc", "tour") On 2013/01/24 02:12:46, adg wrote: > s/miscDir/contentDir/ Done. https://codereview.appspot.com/6976045/diff/52001/misc/dist/bindist.go#newcod... misc/dist/bindist.go:361: // Relative path to the tour from GOPATH On 2013/01/24 02:12:46, adg wrote: > it's the import path, actually Done. https://codereview.appspot.com/6976045/diff/52001/misc/dist/bindist.go#newcod... misc/dist/bindist.go:364: tourDst = filepath.Join(miscDir, "src", tourPkg) On 2013/01/24 02:12:46, adg wrote: > s/tourDst/packageDir/ Done. https://codereview.appspot.com/6976045/diff/52001/misc/dist/bindist.go#newcod... misc/dist/bindist.go:372: for _, dir := range []string{miscDir, tourDst} { On 2013/01/24 02:12:46, adg wrote: > Perhaps just do the MkdirAll inside cpAllDir. Actually it was already doing it :-) Done. https://codereview.appspot.com/6976045/diff/52001/misc/dist/bindist.go#newcod... misc/dist/bindist.go:381: res := []string{"prog", "solutions", "static", "template", "tour.article"} On 2013/01/24 02:12:46, adg wrote: > s/res/content/ Done.
Sign in to reply to this message.
LGTM https://codereview.appspot.com/6976045/diff/59001/misc/dist/bindist.go File misc/dist/bindist.go (right): https://codereview.appspot.com/6976045/diff/59001/misc/dist/bindist.go#newcod... misc/dist/bindist.go:619: stat, err := sf.Stat() looks like a fix for a separate issue. please put it in a followup cl
Sign in to reply to this message.
*** Submitted as https://code.google.com/p/go/source/detail?r=9bcb8f09b9dc *** go/misc: Adding go-tour to the generated packages for every distribution. R=adg, minux.ma CC=golang-dev https://codereview.appspot.com/6976045 https://codereview.appspot.com/6976045/diff/59001/misc/dist/bindist.go File misc/dist/bindist.go (right): https://codereview.appspot.com/6976045/diff/59001/misc/dist/bindist.go#newcod... misc/dist/bindist.go:619: stat, err := sf.Stat() On 2013/01/29 04:06:00, adg wrote: > looks like a fix for a separate issue. please put it in a followup cl Done.
Sign in to reply to this message.
|
