code review 5608056: misc/goplay: use go tool "run"

misc/goplay: use go tool "run"

Fixes issue 2872

[andybalholm, 2012-02-04 00:00:55]

It seems to work.

http://codereview.appspot.com/5608056/diff/2001/misc/goplay/goplay.go
File misc/goplay/goplay.go (right):

http://codereview.appspot.com/5608056/diff/2001/misc/goplay/goplay.go#newcode61
misc/goplay/goplay.go:61: x := strconv.Itoa(<-uniq)
removing the os.TempDir call will clutter up the current working directory with
temporary files.

They will be deleted eventually, but putting them in /tmp seems like a better
practice.

[dupoxy, 2012-02-04 00:23:27]

http://codereview.appspot.com/5608056/diff/2001/misc/goplay/goplay.go
File misc/goplay/goplay.go (right):

http://codereview.appspot.com/5608056/diff/2001/misc/goplay/goplay.go#newcode61
misc/goplay/goplay.go:61: x := strconv.Itoa(<-uniq)
On 2012/02/04 00:00:55, andybalholm wrote:
> removing the os.TempDir call will clutter up the current working directory
with
> temporary files.
> 
> They will be deleted eventually, but putting them in /tmp seems like a better
> practice.
if I do: x := os.TempDir() + strconv.Itoa(<-uniq)
I get: open /tmp0.go: permission denied
and if I do: x := os.TempDir() + "/" + strconv.Itoa(<-uniq)
I get: named files must be in current directory and .go files
this is why, I did it like this. Any idea welcome

[dupoxy, 2012-02-04 00:26:05]

Hello andybalholm@gmail.com (cc: golang-dev@googlegroups.com),

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

[andybalholm, 2012-02-04 00:53:12]

On 2012/02/04 00:23:27, dupoxy wrote:
> if I do: x := os.TempDir() + strconv.Itoa(<-uniq)
> I get: open /tmp0.go: permission denied
> and if I do: x := os.TempDir() + "/" + strconv.Itoa(<-uniq)
> I get: named files must be in current directory and .go files
> this is why, I did it like this. Any idea welcome

Maybe you could use os.Chdir to change the current working directory to
something in /tmp.

[dupoxy, 2012-02-04 01:53:45]

Hello andybalholm@gmail.com (cc: golang-dev@googlegroups.com),

Please take another look.

[dupoxy, 2012-02-04 10:41:16]

Hello andybalholm@gmail.com (cc: golang-dev@googlegroups.com),

Please take another look.

[dupoxy, 2012-02-04 11:46:26]

almost good
I don't get how to remove the temp dir 
please see the comment
sorry this look's simple, but I have much to learn & understand.

http://codereview.appspot.com/5608056/diff/6003/misc/goplay/goplay.go
File misc/goplay/goplay.go (right):

http://codereview.appspot.com/5608056/diff/6003/misc/goplay/goplay.go#newcode44
misc/goplay/goplay.go:44: defer os.RemoveAll(tpdir)
this does not seem to remove the dir how should I do it ?

[andybalholm, 2012-02-04 16:31:25]

http://codereview.appspot.com/5608056/diff/6003/misc/goplay/goplay.go
File misc/goplay/goplay.go (right):

http://codereview.appspot.com/5608056/diff/6003/misc/goplay/goplay.go#newcode44
misc/goplay/goplay.go:44: defer os.RemoveAll(tpdir)
On 2012/02/04 11:46:26, dupoxy wrote:
> this does not seem to remove the dir how should I do it ?
I think the problem is that it is deferred to the end of main(), and main never
exits normally. It runs until the program is killed, and the defer apparently
doesn't get executed then.

The original version doesn't actually use a subdirectory of /tmp. The files have
the form /tmp/compile1.go. You could do the same thing: Chdir to /tmp and create
files with names like goplay1.go

[dupoxy, 2012-02-04 18:30:39]

Hello andybalholm@gmail.com (cc: golang-dev@googlegroups.com),

Please take another look.

[dupoxy, 2012-02-04 18:38:22]

http://codereview.appspot.com/5608056/diff/6003/misc/goplay/goplay.go
File misc/goplay/goplay.go (right):

http://codereview.appspot.com/5608056/diff/6003/misc/goplay/goplay.go#newcode44
misc/goplay/goplay.go:44: defer os.RemoveAll(tpdir)
On 2012/02/04 16:31:26, andybalholm wrote:
> On 2012/02/04 11:46:26, dupoxy wrote:
> > this does not seem to remove the dir how should I do it ?
> I think the problem is that it is deferred to the end of main(), and main
never
> exits normally. It runs until the program is killed, and the defer apparently
> doesn't get executed then.
thanks alot for the explanation on defer in main
> The original version doesn't actually use a subdirectory of /tmp. The files
have
> the form /tmp/compile1.go. You could do the same thing: Chdir to /tmp and
create
> files with names like goplay1.go
Done

[andybalholm, 2012-02-05 00:54:59]

We're getting down to nitpicking stuff now.

http://codereview.appspot.com/5608056/diff/2007/misc/goplay/goplay.go
File misc/goplay/goplay.go (right):

http://codereview.appspot.com/5608056/diff/2007/misc/goplay/goplay.go#newcode40
misc/goplay/goplay.go:40: dir := os.Chdir(os.TempDir())
Shouldn't this variable be called err? It isn't holding a directory.

http://codereview.appspot.com/5608056/diff/2007/misc/goplay/goplay.go#newcode66
misc/goplay/goplay.go:66: // x is the base name for .go, .6, executable files
This comment needs to be updated. The documentation comment for this function
should probably be updated a little too.

[dupoxy, 2012-02-05 01:21:11]

nice, nitpicking
thanks

http://codereview.appspot.com/5608056/diff/2007/misc/goplay/goplay.go
File misc/goplay/goplay.go (right):

http://codereview.appspot.com/5608056/diff/2007/misc/goplay/goplay.go#newcode40
misc/goplay/goplay.go:40: dir := os.Chdir(os.TempDir())
On 2012/02/05 00:54:59, andybalholm wrote:
> Shouldn't this variable be called err? It isn't holding a directory.

Done.

http://codereview.appspot.com/5608056/diff/2007/misc/goplay/goplay.go#newcode66
misc/goplay/goplay.go:66: // x is the base name for .go, .6, executable files
On 2012/02/05 00:54:59, andybalholm wrote:
> This comment needs to be updated. The documentation comment for this function
> should probably be updated a little too.

Done.

[dupoxy, 2012-02-05 01:21:47]

Hello andybalholm@gmail.com (cc: golang-dev@googlegroups.com),

Please take another look.

[andybalholm, 2012-02-06 17:08:40]

LGTM.

[rsc, 2012-02-06 17:10:38]

LGTM

[rsc, 2012-02-06 17:10:55]

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

misc/goplay: use go tool "run"

Fixes issue 2872

R=andybalholm, rsc
CC=golang-dev
http://codereview.appspot.com/5608056

Committer: Russ Cox <rsc@golang.org>