code review 224043: goinstall: an experiment in (external) package installation

goinstall: an experiment in (external) package installation

[cw, 2010-02-26 10:27:08]

So far this seems like a really neat concept and direction.

I think I personally would prefer (maybe as an option) that downloads, builds,
etc. end up in a specified directly, either system-wide or per-user.

http://codereview.appspot.com/224043/diff/13/1016
File src/cmd/goinstall/doc.go (right):

http://codereview.appspot.com/224043/diff/13/1016#newcode70
src/cmd/goinstall/doc.go:70: the import path and the list of Go files in the
package.
This presumably requires that the Makefile is a 'Go-like' Makefile, in so far as
they typically have the first line be:

  include $(GOROOT)/src/Make.$(GOARCH)

My makefiles aren't based on this so I would have to make sure the behavior I
have here (targets and file-placement) is that same surely?

I personally would prefer things are built and installed into some file
hierarchy that is not inside GOROOT.

http://codereview.appspot.com/224043/diff/13/1017
File src/cmd/goinstall/download.go (right):

http://codereview.appspot.com/224043/diff/13/1017#newcode36
src/cmd/goinstall/download.go:36: var github =
regexp.MustCompile(`^(github\.com/[a-z0-9A-Z_.\-]+/[a-z0-9A-Z_.\-]+\.git)(/[a-z0-9A-Z_.\-/]*)?$`)
I assume the long term plan here is to have some table of patterns and
functions?  Or if things could be sufficiently well abstracted out a table of
patterns and actions? (Which could even be runtime at that point)

http://codereview.appspot.com/224043/diff/13/1018
File src/cmd/goinstall/main.go (right):

http://codereview.appspot.com/224043/diff/13/1018#newcode55
src/cmd/goinstall/main.go:55: gobin = os.Getenv("HOME") + "/bin"
Is that always a safe guess?  Might we want to check for the presence of gofmt
or something just to be certain?

http://codereview.appspot.com/224043/diff/13/1018#newcode193
src/cmd/goinstall/main.go:193: w, err := p.Wait(0)
Should there be some sort of timeout here?  The command could hand indefinitely.

[rsc, 2010-02-26 16:18:03]

> http://codereview.appspot.com/224043/diff/13/1016#newcode70
> src/cmd/goinstall/doc.go:70: the import path and the list of Go files in
> the package.
> This presumably requires that the Makefile is a 'Go-like' Makefile, in
> so far as they typically have the first line be:
>
>  include $(GOROOT)/src/Make.$(GOARCH)
>
> My makefiles aren't based on this so I would have to make sure the
> behavior I have here (targets and file-placement) is that same surely?

Your makefiles are ignored.  As long as you have a directory
of Go files from a single non-main pages, they'll get compiled
and built correctly by goinstall.

> I personally would prefer things are built and installed into some file
> hierarchy that is not inside GOROOT.

I know.  That will be next, but for now, $GOROOT is fine.
(See discussion on mailing list.)

> Is that always a safe guess?  Might we want to check for the presence of
> gofmt or something just to be certain?

Good idea, thanks.

> Should there be some sort of timeout here?  The command could hand
> indefinitely.

If it does, the user will surely get tired of waiting and interrupt it.
That seems reasonable.

Russ

[rsc, 2010-03-03 02:24:14]

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

I'd like you to review this change.

[adg, 2010-03-03 02:36:31]

LGTM

Let's see how this goes ...

On 3 March 2010 13:24,  <rsc@golang.org> wrote:
> Hello adg (cc: cw, golang-dev@googlegroups.com),
>
> I'd like you to review this change.
>
>
> http://codereview.appspot.com/224043/show
>

[adg, 2010-03-03 04:01:21]

adg /home/adg/godev$ goinstall github.com/hoisie/web.go
github.com/hoisie/web.go: unknown repository: github.com/hoisie/web.go

adg /home/adg/godev$ goinstall github.com/jacobsa/igo.git/set
github.com/jacobsa/igo.git/set: no such file or directory

What am I doing wrong here?

On 3 March 2010 13:36, Andrew Gerrand <adg@golang.org> wrote:
> LGTM
>
> Let's see how this goes ...
>
> On 3 March 2010 13:24,  <rsc@golang.org> wrote:
>> Hello adg (cc: cw, golang-dev@googlegroups.com),
>>
>> I'd like you to review this change.
>>
>>
>> http://codereview.appspot.com/224043/show
>>
>

[r, 2010-03-03 04:21:55]

didn't look at the python

http://codereview.appspot.com/224043/diff/17/1041
File src/cmd/goinstall/doc.go (right):

http://codereview.appspot.com/224043/diff/17/1041#newcode9
src/cmd/goinstall/doc.go:9: control systems, and it maintains a list of public
Go packages at
'version control systems'?  how about 'sites'? or even 'the internet'?

(also 'from public version control systems' is hard to parse - i stumbled over
it)

http://codereview.appspot.com/224043/diff/17/1041#newcode13
src/cmd/goinstall/doc.go:13: goinstall [flags] importpath...
s/importpath/importPath/ ? or import_path? so it corresponds a little better
with the (also hard to parse) description below

http://codereview.appspot.com/224043/diff/17/1041#newcode20
src/cmd/goinstall/doc.go:20: Goinstall installs each of the packages identified
by import path on
... or just drop 'by import path'

http://codereview.appspot.com/224043/diff/17/1041#newcode26
src/cmd/goinstall/doc.go:26: path refers to a code hosting site, goinstall will
download it if necessary.
'it' has no referent.

http://codereview.appspot.com/224043/diff/17/1041#newcode48
src/cmd/goinstall/doc.go:48: If the directory (e.g.,
$GOROOT/src/pkg/bitbucket.org/user/project)
drop the parens.  s/directory/destination &/ ?

http://codereview.appspot.com/224043/diff/17/1041#newcode57
src/cmd/goinstall/doc.go:57: system (typically HEAD for git, tip for Mercurial).
drop the parens, put in a comma after system

http://codereview.appspot.com/224043/diff/17/1041#newcode78
src/cmd/goinstall/doc.go:78: FAQ:
i don't think the FAQ belongs here. every time i need to look up an argument i
shouldn't have to page back through this one-time stuff. put it on the mailing
list, maybe in a README or something if you think it deserves permanence.

http://codereview.appspot.com/224043/diff/17/1041#newcode82
src/cmd/goinstall/doc.go:82: Goinstall is an experiment, and it is easier if the
experiment does
"it is"  what is?

http://codereview.appspot.com/224043/diff/17/1041#newcode89
src/cmd/goinstall/doc.go:89: Using this notation does not precluding changing to
other
s/precluding/preclude/ s/other/another/ or s/to other//

http://codereview.appspot.com/224043/diff/17/1042
File src/cmd/goinstall/download.go (right):

http://codereview.appspot.com/224043/diff/17/1042#newcode57
src/cmd/goinstall/download.go:57: panic("missing case in download")
put pkg in the message

http://codereview.appspot.com/224043/diff/17/1042#newcode73
src/cmd/goinstall/download.go:73: type vcs struct {
if you want patches you should put comments on these things so they're easy to
understand by outsiders. also the FAQ should mention the file and hint at what
to do

http://codereview.appspot.com/224043/diff/17/1043
File src/cmd/goinstall/main.go (right):

http://codereview.appspot.com/224043/diff/17/1043#newcode20
src/cmd/goinstall/main.go:20: func usage() {
in the doc comment it's importpath, here it's package.

http://codereview.appspot.com/224043/diff/17/1043#newcode85
src/cmd/goinstall/main.go:85: // install installs the package named by path,
which is needed for parent.
s/for/by/?

http://codereview.appspot.com/224043/diff/17/1043#newcode153
src/cmd/goinstall/main.go:153: // Assume that if the first element has a dot,
its a domain name
s/its/it's/

http://codereview.appspot.com/224043/diff/17/1043#newcode168
src/cmd/goinstall/main.go:168: // quietRun is like run but prints nothing on
failure (unless -v is used).
drop parens

http://codereview.appspot.com/224043/diff/17/1043#newcode181
src/cmd/goinstall/main.go:181: fmt.Fprintf(os.Stderr, "%s; %s %s\n", dir, bin,
strings.Join(cmd[1:], " "))
put the program name on all these messages

http://codereview.appspot.com/224043/diff/17/1044
File src/cmd/goinstall/make.go (right):

http://codereview.appspot.com/224043/diff/17/1044#newcode16
src/cmd/goinstall/make.go:16: // If local is false, the package was downloaded
off the web.
s/off/from/
get rid of 'downloaded' too if you would; it's a long way to say 'got' or
'retrieved' or 'copied'.

http://codereview.appspot.com/224043/diff/17/1044#newcode18
src/cmd/goinstall/make.go:18: // domake generates a standard makefile and passes
it
here it's makefile, in the code it's Makefile

http://codereview.appspot.com/224043/diff/17/1044#newcode27
src/cmd/goinstall/make.go:27: makefile, err := makemakefile(dir, pkg)
makeMakefile

http://codereview.appspot.com/224043/diff/17/1044#newcode35
src/cmd/goinstall/make.go:35: // installing as package pkg.  It includes all go
files in the directory
s/go/.go/ ?

http://codereview.appspot.com/224043/diff/17/1044#newcode37
src/cmd/goinstall/make.go:37: func makemakefile(dir, pkg string) ([]byte,
os.Error) {
makeMakefile

http://codereview.appspot.com/224043/diff/17/1044#newcode50
src/cmd/goinstall/make.go:50: type makedata struct {
comment?

http://codereview.appspot.com/224043/diff/17/1044#newcode66
src/cmd/goinstall/make.go:66: nil)
this is cool

http://codereview.appspot.com/224043/diff/17/1045
File src/cmd/goinstall/parse.go (right):

http://codereview.appspot.com/224043/diff/17/1045#newcode20
src/cmd/goinstall/parse.go:20: // Go source files in dir.  The map keys are the
imported paths.
you're inconsistent across files about whether it's Go or go

http://codereview.appspot.com/224043/diff/17/1045#newcode38
src/cmd/goinstall/parse.go:38: continue
i see this twice. maybe write an iterator for directories to find the .go files?

http://codereview.appspot.com/224043/diff/17/1045#newcode56
src/cmd/goinstall/parse.go:56: for _, spec := range gd.Specs {
fold lines together?

http://codereview.appspot.com/224043/diff/17/1045#newcode60
src/cmd/goinstall/parse.go:60: log.Crashf("%s: parser returned invalid quoted
string: %s", filename, quoted)
going to be confusing: maybe <> around the second %s

http://codereview.appspot.com/224043/diff/17/1045#newcode71
src/cmd/goinstall/parse.go:71: func gofiles(dir string) ([]string, os.Error) {
goFiles

[rsc, 2010-03-03 05:12:38]

On Tue, Mar 2, 2010 at 20:00, Andrew Gerrand <adg@golang.org> wrote:

> adg /home/adg/godev$ goinstall github.com/hoisie/web.go
> github.com/hoisie/web.go: unknown repository: github.com/hoisie/web.go


this one you missed .git but i think i might take that out of the paths.


> adg /home/adg/godev$ goinstall github.com/jacobsa/igo.git/set
> github.com/jacobsa/igo.git/set: no such file or directory
>

don't know what's wrong on this one.

russ

[adg, 2010-03-03 05:14:50]

On 3 March 2010 16:12, Russ Cox <rsc@golang.org> wrote:
> On Tue, Mar 2, 2010 at 20:00, Andrew Gerrand <adg@golang.org> wrote:
>>
>> adg /home/adg/godev$ goinstall github.com/hoisie/web.go
>> github.com/hoisie/web.go: unknown repository: github.com/hoisie/web.go
>
> this one you missed .git but i think i might take that out of the paths.

adg /home/adg/godev/web$ goinstall github.com/hoisie/web.go.git
github.com/hoisie/web.go.git: no such file or directory

Now I get the same error as the below. I'll investigate.

Andrew

[rsc, 2010-03-04 02:41:55]

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

Please take another look.

[rsc, 2010-03-04 02:42:17]

Addressed Rob's comments.
Have not investigated Andrew's problem.

Russ

[r, 2010-03-04 04:36:36]

LGTM

http://codereview.appspot.com/224043/diff/17/1041
File src/cmd/goinstall/doc.go (right):

http://codereview.appspot.com/224043/diff/17/1041#newcode76
src/cmd/goinstall/doc.go:76: 
did this text go anywhere? there are no new files.
(it's fine if it didn't)

[rsc, 2010-03-04 17:38:30]

> did this text go anywhere? there are no new files.
> (it's fine if it didn't)

it didn't.  it's already been posted to the mailing list once.
i might add it to the package dashboard web page later.

russ

[adg, 2010-03-05 00:18:40]

On 4 March 2010 13:42, Russ Cox <rsc@golang.org> wrote:
> Have not investigated Andrew's problem.

I haven't had a chance to check this out yet. I'll have another look
now. I want to make sure that I can easily use (and explain) this tool
before we release it.

However, I've come across another issue in the meantime:

When you run GOROOT/src/all.bash it cleans out
$GOROOT/pkg/GOOS_GOARCH. This means any packages a user has installed
with goinstall will need to be rebuilt afterward. I suppose this makes
sense - as the compiler may have changed, necessitating a recompile.
Is there an easy way for me to recompile all of my 3rd party libs?

Andrew

[rsc, 2010-03-05 00:22:42]

> When you run GOROOT/src/all.bash it cleans out
> $GOROOT/pkg/GOOS_GOARCH. This means any packages a user has installed
> with goinstall will need to be rebuilt afterward. I suppose this makes
> sense - as the compiler may have changed, necessitating a recompile.
> Is there an easy way for me to recompile all of my 3rd party libs?

No but it's easy to recompile the program you
care about, which will automatically recompile
all the 3rd party libs it needs.

Russ

[rsc, 2010-03-05 00:29:12]

> adg /home/adg/godev/web$ goinstall github.com/hoisie/web.go.git
> github.com/hoisie/web.go.git: no such file or directory

I cannot reproduce this.
I have tried on Linux and Mac.
Is it possible you have a stale copy of the code?
I assume you did a clpatch.

hg revert @224043
hg change -D 224043
rm -r $GOROOT/src/cmd/goinstall $GOROOT/misc/dashboard/godashboard/package.py
hg clpatch 224043
cd $GOROOT/src/cmd/goinstall
make clean
make install

Russ

[adg, 2010-03-05 01:04:10]

LGTM

It seems to be working well now.

On 5 March 2010 11:29, Russ Cox <rsc@golang.org> wrote:
>> adg /home/adg/godev/web$ goinstall github.com/hoisie/web.go.git
>> github.com/hoisie/web.go.git: no such file or directory
>
> I cannot reproduce this.
> I have tried on Linux and Mac.
> Is it possible you have a stale copy of the code?
> I assume you did a clpatch.
>
> hg revert @224043
> hg change -D 224043
> rm -r $GOROOT/src/cmd/goinstall $GOROOT/misc/dashboard/godashboard/package.py
> hg clpatch 224043
> cd $GOROOT/src/cmd/goinstall
> make clean
> make install
>
> Russ
>

[rsc, 2010-03-05 01:04:59]

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

goinstall: an experiment in (external) package installation

R=adg, r
CC=cw, golang-dev
http://codereview.appspot.com/224043