code review 1947041: goinstall: added -a flag to mean "all remote packages"

goinstall: added -a flag to mean "all remote packages"
Fixes issue 897.

goinstall -a can be used to reinstall all packages after an upgrade
goinstall -a -u can be used to update all package
A history of remote package installs is stored in $GOROOT/goinstall.log

[bytbox, 2010-08-10 03:46:53]

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

I'd like you to review this change.

[rsc1, 2010-08-12 03:19:06]

Thanks for doing this.  It's a great addition.

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

http://codereview.appspot.com/1947041/diff/5/2003#newcode23
src/cmd/goinstall/main.go:23: fmt.Fprint(os.Stderr, "usage: goinstall
importpath...\n")
this should say

usage: goinstall -a | importpath...

http://codereview.appspot.com/1947041/diff/5/2003#newcode39
src/cmd/goinstall/main.go:39: verbose           = flag.Bool("v", false,
"verbose")
There should be a way to disable logging.

log = flag.Bool("l", true, "log installed packages to $GOROOT/goinstall.log for
use by -a")

http://codereview.appspot.com/1947041/diff/5/2003#newcode64
src/cmd/goinstall/main.go:64: var pkgs vector.StringVector
The logic here seems gratuitously different.
I think you want to keep the old code and then say

if *allpkg {
    if len(args) != 0 {
        usage()
    }
    ...
    args = ...
}

The change in variable name is unnecessary,
as is the use of StringVector.

http://codereview.appspot.com/1947041/diff/5/2003#newcode71
src/cmd/goinstall/main.go:71: fin, err := os.Open(path.Join(os.Getenv("GOROOT"),
".goinstall_log"), os.O_RDONLY, 0)
This can be done once during initialization and stored it in a global:

var logfile = path.Join(os.Getenv("GOROOT"), "goinstall.log")

Note the lack of a leading dot too.  I don't want to hide
the fact that goinstall is logging things.  In general dot files
are a bad idea - the fact that they are hidden due to a 40
year old bug in the shell means that they get overused
and you don't even notice until you've got 1000 "hidden"
files in your directory that are slowing everything else down.

http://codereview.appspot.com/1947041/diff/5/2003#newcode106
src/cmd/goinstall/main.go:106: // markPackage marks the named package as
installed in .goinstall_log
Should be logPackage.

http://codereview.appspot.com/1947041/diff/5/2003#newcode108
src/cmd/goinstall/main.go:108: fout, err :=
os.Open(path.Join(os.Getenv("GOROOT"), ".goinstall_log"),
Should it check to see if the package is already listed?

[bytbox, 2010-08-14 01:26:21]

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

Please take another look.

[bytbox, 2010-08-17 11:45:17]

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

Please take another look.

[rsc1, 2010-08-26 16:28:22]

http://codereview.appspot.com/1947041/diff/11001/12002
File src/cmd/goinstall/doc.go (right):

http://codereview.appspot.com/1947041/diff/11001/12002#newcode18
src/cmd/goinstall/doc.go:18: -l=true           log installed packages to
$GOROOT/goinstall.log for use by -a
Since saying -l is useless, a short name is probably not necessary here.
Might as well say -log=true.

http://codereview.appspot.com/1947041/diff/11001/12002#newcode24
src/cmd/goinstall/doc.go:24: itself.
Unless -log=false is specified, goinstall logs the import path of each installed
package to $GOROOT/goinstall.log for use by goinstall -a.

http://codereview.appspot.com/1947041/diff/11001/12002#newcode26
src/cmd/goinstall/doc.go:26: If the -a flag is given, goinstall ignores the
command line and instead uses a
It shouldn't ignore the command line.
It's probably an error, in which case you don't have to say anything.

If the -a flag is given, goinstall reinstalls all previously installed packages,
reading the list from $GOROOT/goinstall.log.  After updating to a new Go
release, which deletes all package binaries, running

    goinstall -a

will recompile and reinstall goinstalled packages.

Another common idiom is to use

    goinstall -a -u

to update, recompile, and reinstall all goinstalled packages.

http://codereview.appspot.com/1947041/diff/11001/12002#newcode31
src/cmd/goinstall/doc.go:31: goinstall -a -u
should be a tab

http://codereview.appspot.com/1947041/diff/11001/12003
File src/cmd/goinstall/main.go (right):

http://codereview.appspot.com/1947041/diff/11001/12003#newcode22
src/cmd/goinstall/main.go:22: fmt.Fprint(os.Stderr, "usage: goinstall -a |
importpath...\n")
Now that I see it, this should probably be two lines like you did in the docs.

fmt.Fprint(os.Stderr, "usage: goinstall importpath...\n")
fmt.Fprintf(os.Stderr, "\tgoinstall -a\n")

http://codereview.appspot.com/1947041/diff/11001/12003#newcode67
src/cmd/goinstall/main.go:67: // install all packages that were ever installed
if len(args) != 0 {
    usage()
}

http://codereview.appspot.com/1947041/diff/11001/12003#newcode72
src/cmd/goinstall/main.go:72: fmt.Printf("No installed packages.\n")
Errors should go to standard error and include the program name:

fmt.Fprintf(os.Stderr, "%s: no installed packages\n", argv0)

http://codereview.appspot.com/1947041/diff/11001/12003#newcode101
src/cmd/goinstall/main.go:101: pkglistdata, _ := ioutil.ReadFile(logfile)
Should probably put the strings into a map the
first time this is called and then consult the map for
future times.  Otherwise the file just keeps getting
read over and over while goinstall installs dependencies.

[bytbox, 2010-08-27 04:05:43]

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

Please take another look.

[rsc1, 2010-08-27 04:10:03]

http://codereview.appspot.com/1947041/diff/23001/24003
File src/cmd/goinstall/main.go (right):

http://codereview.appspot.com/1947041/diff/23001/24003#newcode36
src/cmd/goinstall/main.go:36: logfile = path.Join(os.Getenv("GOROOT"),
"goinstall.log")
s/os.Getenv("GOROOT")/root/

http://codereview.appspot.com/1947041/diff/23001/24003#newcode42
src/cmd/goinstall/main.go:42: logPkgs           = flag.Bool("l", true, "log
installed packages to $GOROOT/goinstall.log for use by -a")
s/"l"/"log"/

http://codereview.appspot.com/1947041/diff/23001/24003#newcode76
src/cmd/goinstall/main.go:76: pkglistdata, _ := ioutil.ReadFile(logfile)
call readPackageList

http://codereview.appspot.com/1947041/diff/23001/24003#newcode107
src/cmd/goinstall/main.go:107: func readPackageList() {
confused about where this gets called and
why it doesn't return anything.
also it should probably be a map[string]bool
at the end, to avoid the linear lookup in logPackage.

[bytbox, 2010-08-27 04:19:45]

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

Please take another look.

[rsc1, 2010-08-27 04:26:13]

http://codereview.appspot.com/1947041/diff/29001/30003
File src/cmd/goinstall/main.go (right):

http://codereview.appspot.com/1947041/diff/29001/30003#newcode77
src/cmd/goinstall/main.go:77: readPackageList()
doesn't seem right - you won't have
a package list during normal installations.

seems like up above you want

if *allpkg || *logPkgs {
    readPackageList()
}

http://codereview.appspot.com/1947041/diff/29001/30003#newcode117
src/cmd/goinstall/main.go:117: for _, pkgname := range pkglist {
should use installedPkgs

[bytbox, 2010-08-28 01:24:28]

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

Please take another look.

[adg, 2010-08-30 07:13:04]

http://codereview.appspot.com/1947041/diff/35001/36003
File src/cmd/goinstall/main.go (right):

http://codereview.appspot.com/1947041/diff/35001/36003#newcode39
src/cmd/goinstall/main.go:39: pkglist []string
Seems like installedPkgs and pkglist are the same. Drop the latter.

http://codereview.appspot.com/1947041/diff/35001/36003#newcode118
src/cmd/goinstall/main.go:118: for pkgname := range installedPkgs {
More simply/efficiently:

if _, ok := installedPkgs[pkgname]; ok {
  return
}

[rsc, 2010-08-30 14:32:14]

> http://codereview.appspot.com/1947041/diff/35001/36003#newcode39
> src/cmd/goinstall/main.go:39: pkglist []string
> Seems like installedPkgs and pkglist are the same. Drop the latter.

Not exactly.  One preserves the order in the file, one does not.

> if _, ok := installedPkgs[pkgname]; ok {
>  return
> }

Even simpler:

if installedPkgs[pkgname] {

Russ

[bytbox, 2010-08-30 23:23:32]

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

Please take another look.

[bytbox, 2010-08-30 23:23:46]

> Not exactly.  One preserves the order in the file, one does not.
Although the order isn't used, so it can be safely dropped.

On 8/30/10, Russ Cox <rsc@golang.org> wrote:
>> http://codereview.appspot.com/1947041/diff/35001/36003#newcode39
>> src/cmd/goinstall/main.go:39: pkglist []string
>> Seems like installedPkgs and pkglist are the same. Drop the latter.
>
> Not exactly.  One preserves the order in the file, one does not.
>
>> if _, ok := installedPkgs[pkgname]; ok {
>>  return
>> }
>
> Even simpler:
>
> if installedPkgs[pkgname] {
>
> Russ
>


-- 
Scott Lawrence

[rsc, 2010-08-30 23:26:07]

On Mon, Aug 30, 2010 at 19:23, Scott Lawrence <bytbox@gmail.com> wrote:
>> Not exactly.  One preserves the order in the file, one does not.
> Although the order isn't used, so it can be safely dropped.

I liked that the order was deterministic across runs
but it's probably fine.

Russ

[bytbox, 2010-08-30 23:33:03]

Assuming packages aren't broken, requiring manual installation in a
certain order, it should work. That seems a safe assumption.

(The only such packages I know of at the moment are ... mine.)


On 8/30/10, Russ Cox <rsc@golang.org> wrote:
> On Mon, Aug 30, 2010 at 19:23, Scott Lawrence <bytbox@gmail.com> wrote:
>>> Not exactly.  One preserves the order in the file, one does not.
>> Although the order isn't used, so it can be safely dropped.
>
> I liked that the order was deterministic across runs
> but it's probably fine.
>
> Russ
>


-- 
Scott Lawrence

[rsc, 2010-08-30 23:35:19]

On Mon, Aug 30, 2010 at 19:33, Scott Lawrence <bytbox@gmail.com> wrote:
> Assuming packages aren't broken, requiring manual installation in a
> certain order, it should work. That seems a safe assumption.
>
> (The only such packages I know of at the moment are ... mine.)

I'm not saying it's wrong, just that it would be nice
for repeatability.  But again it's fine.

Russ

[rsc1, 2010-08-31 01:14:02]

LGTM

[rsc, 2010-09-02 17:48:32]

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

goinstall: added -a flag to mean "all remote packages"
Fixes issue 897.

goinstall -a can be used to reinstall all packages after an upgrade
goinstall -a -u can be used to update all package
A history of remote package installs is stored in $GOROOT/goinstall.log

R=rsc, adg
CC=golang-dev
http://codereview.appspot.com/1947041

Committer: Russ Cox <rsc@golang.org>