code review 5697050: misc/dist: implement binary distribution scripts in go

misc/dist: implement binary distribution scripts in go

[adg, 2012-02-24 04:47:31]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://go.googlecode.com/hg

[r, 2012-02-24 05:01:19]

http://codereview.appspot.com/5697050/diff/5006/misc/dist/bindist.go
File misc/dist/bindist.go (right):

http://codereview.appspot.com/5697050/diff/5006/misc/dist/bindist.go#newcode1
misc/dist/bindist.go:1: // This is a tool for packaging binary releases.
copyright

http://codereview.appspot.com/5697050/diff/5006/misc/dist/bindist.go#newcode40
misc/dist/bindist.go:40: os.Exit(2)
move this into Usage?

[adg, 2012-02-24 05:58:49]

PTAL

http://codereview.appspot.com/5697050/diff/5006/misc/dist/bindist.go
File misc/dist/bindist.go (right):

http://codereview.appspot.com/5697050/diff/5006/misc/dist/bindist.go#newcode1
misc/dist/bindist.go:1: // This is a tool for packaging binary releases.
On 2012/02/24 05:01:19, r wrote:
> copyright

Done.

http://codereview.appspot.com/5697050/diff/5006/misc/dist/bindist.go#newcode40
misc/dist/bindist.go:40: os.Exit(2)
On 2012/02/24 05:01:19, r wrote:
> move this into Usage?

Done.

[adg, 2012-02-24 06:00:10]

This now includes code to upload the archives to Google Code.

[brainman, 2012-02-24 06:10:08]

http://codereview.appspot.com/5697050/diff/5006/misc/dist/bindist.go
File misc/dist/bindist.go (right):

http://codereview.appspot.com/5697050/diff/5006/misc/dist/bindist.go#newcode173
misc/dist/bindist.go:173: strings.HasPrefix(e, "GOARCH=") {
You should also skip GOBIN variable. Otherwise dist bin directory will be empty,
and all binaries will end-up in your GOBIN directory instead.

[adg, 2012-02-24 06:18:45]

http://codereview.appspot.com/5697050/diff/5006/misc/dist/bindist.go
File misc/dist/bindist.go (right):

http://codereview.appspot.com/5697050/diff/5006/misc/dist/bindist.go#newcode173
misc/dist/bindist.go:173: strings.HasPrefix(e, "GOARCH=") {
On 2012/02/24 06:10:09, brainman wrote:
> You should also skip GOBIN variable. Otherwise dist bin directory will be
empty,
> and all binaries will end-up in your GOBIN directory instead.

Done.

[brainman, 2012-02-24 07:30:32]

LGTM

[r, 2012-02-24 08:44:35]

http://codereview.appspot.com/5697050/diff/5/misc/dist/bindist.go
File misc/dist/bindist.go (right):

http://codereview.appspot.com/5697050/diff/5/misc/dist/bindist.go#newcode6
misc/dist/bindist.go:6: // It currently supports FreeBSD, Linux, and OS X.
s/currently //

http://codereview.appspot.com/5697050/diff/5/misc/dist/bindist.go#newcode30
misc/dist/bindist.go:30: password = flag.String("password", "", "Google Code
upload password")

don't do this. clear text, on the command line, visible in ps output, etc.

http://codereview.appspot.com/5697050/diff/5/misc/dist/bindist.go#newcode69
misc/dist/bindist.go:69: OS, Arch string
put on two lines

http://codereview.appspot.com/5697050/diff/5/misc/dist/bindist.go#newcode112
misc/dist/bindist.go:112: for _, name := range cleanFiles {
scary. instead, i'd test it's clean and stop processing if it's not, let the
user clean up. RemoveAll is not to be done lightly.

http://codereview.appspot.com/5697050/diff/5/misc/dist/bindist.go#newcode159
misc/dist/bindist.go:159: "--target", "10.5")
is that true?

[adg, 2012-02-24 23:01:56]

http://codereview.appspot.com/5697050/diff/5/misc/dist/bindist.go
File misc/dist/bindist.go (right):

http://codereview.appspot.com/5697050/diff/5/misc/dist/bindist.go#newcode112
misc/dist/bindist.go:112: for _, name := range cleanFiles {
On 2012/02/24 08:44:35, r wrote:
> scary. instead, i'd test it's clean and stop processing if it's not, let the
> user clean up. RemoveAll is not to be done lightly.

This is to remove the (many megabytes of) mercurial metadata. It's not a state
that we can get into any other way.

[r2, 2012-02-24 23:05:39]

On 25/02/2012, at 10:01 AM, adg@golang.org wrote:

> 
> http://codereview.appspot.com/5697050/diff/5/misc/dist/bindist.go
> File misc/dist/bindist.go (right):
> 
> http://codereview.appspot.com/5697050/diff/5/misc/dist/bindist.go#newcode112
> misc/dist/bindist.go:112: for _, name := range cleanFiles {
> On 2012/02/24 08:44:35, r wrote:
>> scary. instead, i'd test it's clean and stop processing if it's not,
> let the
>> user clean up. RemoveAll is not to be done lightly.
> 
> This is to remove the (many megabytes of) mercurial metadata. It's not a
> state that we can get into any other way.

Is there some sanity check you can put on the file name before executing the
RemoveAll?

-rob

[adg, 2012-02-25 04:47:01]

http://codereview.appspot.com/5697050/diff/5/misc/dist/bindist.go
File misc/dist/bindist.go (right):

http://codereview.appspot.com/5697050/diff/5/misc/dist/bindist.go#newcode6
misc/dist/bindist.go:6: // It currently supports FreeBSD, Linux, and OS X.
On 2012/02/24 08:44:35, r wrote:
> s/currently //

Done.

http://codereview.appspot.com/5697050/diff/5/misc/dist/bindist.go#newcode30
misc/dist/bindist.go:30: password = flag.String("password", "", "Google Code
upload password")
On 2012/02/24 08:44:35, r wrote:
> 
> don't do this. clear text, on the command line, visible in ps output, etc.

Now I'm using the .gobuildkey file in the same way that gobuilder does. 2nd and
3rd lines of the file are username and password

http://codereview.appspot.com/5697050/diff/5/misc/dist/bindist.go#newcode69
misc/dist/bindist.go:69: OS, Arch string
On 2012/02/24 08:44:35, r wrote:
> put on two lines

Done.

http://codereview.appspot.com/5697050/diff/5/misc/dist/bindist.go#newcode159
misc/dist/bindist.go:159: "--target", "10.5")
On 2012/02/24 08:44:35, r wrote:
> is that true?

Yes, it's the minimum.

[adg, 2012-02-25 04:49:06]

What kind of check? What's the concern?

On 25 February 2012 10:05, Rob 'Commander' Pike <r@google.com> wrote:
>
> On 25/02/2012, at 10:01 AM, adg@golang.org wrote:
>
>>
>> http://codereview.appspot.com/5697050/diff/5/misc/dist/bindist.go
>> File misc/dist/bindist.go (right):
>>
>> http://codereview.appspot.com/5697050/diff/5/misc/dist/bindist.go#newcode112
>> misc/dist/bindist.go:112: for _, name := range cleanFiles {
>> On 2012/02/24 08:44:35, r wrote:
>>> scary. instead, i'd test it's clean and stop processing if it's not,
>> let the
>>> user clean up. RemoveAll is not to be done lightly.
>>
>> This is to remove the (many megabytes of) mercurial metadata. It's not a
>> state that we can get into any other way.
>
> Is there some sanity check you can put on the file name before executing the
RemoveAll?
>
> -rob
>
>

[r2, 2012-02-25 04:55:10]

On 25/02/2012, at 3:48 PM, Andrew Gerrand wrote:

> What kind of check? What's the concern?

i'm concerned about executing RemoveAll on a computed directory. i can convince
myself the code is correct but if there's a ../ hiding somewhere it'll do very
bad things.

but never mind. i don't have a good idea and it's your machine it'll be running
on.

-rob

[Mike.Rosset, 2012-02-25 08:01:37]

On 2012/02/24 04:47:31, adg wrote:
> Hello mailto:golang-dev@googlegroups.com,
> 
> I'd like you to review this change to
> https://go.googlecode.com/hg

I started hacking on something that does exactly this, so I'm glad I found this
CL. Our implementations are quite close, the only thing I might suggest is
possibly using an interface for Package, so you have structs like GzipPackager
DarwinPackager WixPackager, with a Package  interface method.

Also in the future I know we want to be less dependant on unix based command
utitlities. So if you like I could implement the tar Command in pure go. Which
gets rid of a tar dependency.

Also not for Go1 but possibly later, what would people think of having one
unified command line installer written in pure go, and using tarballs? this
would mean less dist depends for windows. ie wix and 7zip. Also it could handle
downloading, decompression, upgrading and security updates.

[adg, 2012-02-25 09:02:43]

This is just a tool for the go team to use. It is fine to assume tar exists
on the Unix builders. We control them.

I threw this together because I was irked by maintaining the shell scripts.
I don't want to make it much more complex than it already is.

As for an installer, that's a separate story altogether.

Andrew
On Feb 25, 2012 7:01 PM, <Mike.Rosset@gmail.com> wrote:

> On 2012/02/24 04:47:31, adg wrote:
>
>> Hello mailto:golang-dev@**googlegroups.com <golang-dev@googlegroups.com>,
>>
>
>  I'd like you to review this change to
>> https://go.googlecode.com/hg
>>
>
> I started hacking on something that does exactly this, so I'm glad I
> found this CL. Our implementations are quite close, the only thing I
> might suggest is possibly using an interface for Package, so you have
> structs like GzipPackager DarwinPackager WixPackager, with a Package
> interface method.
>
> Also in the future I know we want to be less dependant on unix based
> command utitlities. So if you like I could implement the tar Command in
> pure go. Which gets rid of a tar dependency.
>
> Also not for Go1 but possibly later, what would people think of having
> one unified command line installer written in pure go, and using
> tarballs? this would mean less dist depends for windows. ie wix and
> 7zip. Also it could handle downloading, decompression, upgrading and
> security updates.
>
>
http://codereview.appspot.com/**5697050/<http://codereview.appspot.com/5697050/>
>

[mike.rosset_gmail.com, 2012-02-25 09:21:52]

> This is just a tool for the go team to use. It is fine to assume tar exists
> on the Unix builders. We control them.
>
> I threw this together because I was irked by maintaining the shell scripts.
> I don't want to make it much more complex than it already is.
>
> As for an installer, that's a separate story altogether.
>
> Andrew

Understood.

[adg, 2012-03-01 03:35:04]

Can I get an amen?

[r, 2012-03-01 03:56:40]

LGTM

[adg, 2012-03-01 04:49:43]

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

misc/dist: implement binary distribution scripts in go

R=golang-dev, r, alex.brainman, r, mike.rosset
CC=golang-dev
http://codereview.appspot.com/5697050