code review 1301043: Make goinstall work with main packages.

Make goinstall work with main packages.
Currently to install a command, you have to manually
goinstall each of the remote packages that it depends on.
This patch lets goinstall P work where P is
contains files in package main.
It does not actually build the package, but
it installs all of its dependencies and prints a message
to that effect.

[rog, 2010-05-27 06:55:10]

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

I'd like you to review this change.

[rsc1, 2010-06-01 21:25:41]

Looks pretty good.

http://codereview.appspot.com/1301043/diff/5/5001
File src/cmd/goinstall/main.go (right):

http://codereview.appspot.com/1301043/diff/5/5001#newcode139
src/cmd/goinstall/main.go:139: fmt.Fprintf(os.Stderr, "%s: %s needs manual
installation. Its dependencies are successfully installed\n", argv0, pkg)
if !errors {
     "%s: %s's dependencies are installed."

http://codereview.appspot.com/1301043/diff/5/5002
File src/cmd/goinstall/make.go (right):

http://codereview.appspot.com/1301043/diff/5/5002#newcode28
src/cmd/goinstall/make.go:28: if err != nil || makefile == nil {
Can this happen?
It shouldn't.

http://codereview.appspot.com/1301043/diff/5/5003
File src/cmd/goinstall/parse.go (right):

http://codereview.appspot.com/1301043/diff/5/5003#newcode59
src/cmd/goinstall/parse.go:59: if allowMain {
I'd drop this if statement just to simplify the logic.
It is epsilon slower but that doesn't matter.

[rog, 2010-06-06 10:47:08]

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

Please take another look.

[rog, 2010-06-06 10:48:22]

also found an embarassing error that meant that the dependencies weren't
actually built. i could've sworn i'd tested it!

http://codereview.appspot.com/1301043/diff/5/5001
File src/cmd/goinstall/main.go (right):

http://codereview.appspot.com/1301043/diff/5/5001#newcode139
src/cmd/goinstall/main.go:139: fmt.Fprintf(os.Stderr, "%s: %s needs manual
installation. Its dependencies are successfully installed\n", argv0, pkg)
On 2010/06/01 21:25:41, rsc1 wrote:
> if !errors {
>      "%s: %s's dependencies are installed."
> 

Done.

http://codereview.appspot.com/1301043/diff/5/5002
File src/cmd/goinstall/make.go (right):

http://codereview.appspot.com/1301043/diff/5/5002#newcode28
src/cmd/goinstall/make.go:28: if err != nil || makefile == nil {
On 2010/06/01 21:25:41, rsc1 wrote:
> Can this happen?
> It shouldn't.

yup. don't know where that came from.

http://codereview.appspot.com/1301043/diff/5/5003
File src/cmd/goinstall/parse.go (right):

http://codereview.appspot.com/1301043/diff/5/5003#newcode59
src/cmd/goinstall/parse.go:59: if allowMain {
On 2010/06/01 21:25:41, rsc1 wrote:
> I'd drop this if statement just to simplify the logic.
> It is epsilon slower but that doesn't matter.
> 

Done.

[rog, 2010-06-20 21:06:56]

is there any chance that this patch might be accepted,
or should i abandon it?

On 1 June 2010 22:25,  <rsc@google.com> wrote:
> Looks pretty good.
>
>
>
> http://codereview.appspot.com/1301043/diff/5/5001
> File src/cmd/goinstall/main.go (right):
>
> http://codereview.appspot.com/1301043/diff/5/5001#newcode139
> src/cmd/goinstall/main.go:139: fmt.Fprintf(os.Stderr, "%s: %s needs
> manual installation. Its dependencies are successfully installed\n",
> argv0, pkg)
> if !errors {
>     "%s: %s's dependencies are installed."
>
> http://codereview.appspot.com/1301043/diff/5/5002
> File src/cmd/goinstall/make.go (right):
>
> http://codereview.appspot.com/1301043/diff/5/5002#newcode28
> src/cmd/goinstall/make.go:28: if err != nil || makefile == nil {
> Can this happen?
> It shouldn't.
>
> http://codereview.appspot.com/1301043/diff/5/5003
> File src/cmd/goinstall/parse.go (right):
>
> http://codereview.appspot.com/1301043/diff/5/5003#newcode59
> src/cmd/goinstall/parse.go:59: if allowMain {
> I'd drop this if statement just to simplify the logic.
> It is epsilon slower but that doesn't matter.
>
> http://codereview.appspot.com/1301043/show
>

[rsc1, 2010-06-20 21:18:51]

LGTM

Sorry for the long delay.
I'm not managing to keep up with email recently.

[rog, 2010-06-21 07:57:42]

no problem. even superheroes have their limitations :-)

still needs the commit BTW, but i'm sure you know that.

On 20 June 2010 22:18,  <rsc@google.com> wrote:
> LGTM
>
> Sorry for the long delay.
> I'm not managing to keep up with email recently.
>
>
> http://codereview.appspot.com/1301043/show
>

[rsc, 2010-06-21 18:01:26]

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

goinstall: process dependencies for package main

Currently to install a command, you have to manually
goinstall each of the remote packages that it depends on.
This patch lets goinstall P work where P is
contains files in package main.
It does not actually build the package, but
it installs all of its dependencies and prints a message
to that effect.

R=rsc
CC=golang-dev
http://codereview.appspot.com/1301043

Committer: Russ Cox <rsc@golang.org>