code review 4627056: goinstall: build with make by default, add -make flag

goinstall: build with make by default, add -make flag

This is a temporary measure until go/build can build cgo packages.

[adg, 2011-06-23 05:09:30]

Hello golang-dev@googlegroups.com,

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

[adg, 2011-06-23 05:27:44]

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

Please take another look.

[r, 2011-06-24 01:39:24]

http://codereview.appspot.com/4627056/diff/9001/src/cmd/goinstall/make.go
File src/cmd/goinstall/make.go (right):

http://codereview.appspot.com/4627056/diff/9001/src/cmd/goinstall/make.go#new...
src/cmd/goinstall/make.go:1: // Copyright 2010 The Go Authors.  All rights
reserved.
2011?

http://codereview.appspot.com/4627056/diff/9001/src/cmd/goinstall/make.go#new...
src/cmd/goinstall/make.go:118: if c := s[i]; c < 0x80 &&
bytes.IndexByte(safeBytes, c) < 0 {
not sure about a "../" prefix. i think it should be unsafe. actually a /../
anywhere in the name is probalby bad too. cleanpath it first?

http://codereview.appspot.com/4627056/diff/9001/src/cmd/goinstall/make.go#new...
src/cmd/goinstall/make.go:143: {.section GoFiles}
doesn't this put you down a level by setting the cursor? that is, i don't
understand why you say GoFiles twice

[adg, 2011-06-24 01:46:33]

http://codereview.appspot.com/4627056/diff/9001/src/cmd/goinstall/make.go
File src/cmd/goinstall/make.go (right):

http://codereview.appspot.com/4627056/diff/9001/src/cmd/goinstall/make.go#new...
src/cmd/goinstall/make.go:1: // Copyright 2010 The Go Authors.  All rights
reserved.
On 2011/06/24 01:39:24, r wrote:
> 2011?

Done.

http://codereview.appspot.com/4627056/diff/9001/src/cmd/goinstall/make.go#new...
src/cmd/goinstall/make.go:118: if c := s[i]; c < 0x80 &&
bytes.IndexByte(safeBytes, c) < 0 {
On 2011/06/24 01:39:24, r wrote:
> not sure about a "../" prefix. i think it should be unsafe. actually a /../
> anywhere in the name is probalby bad too. cleanpath it first?

I'd rather just reject the name outright if it contains "..".

This is only hanging around for a short time, anyway.

http://codereview.appspot.com/4627056/diff/9001/src/cmd/goinstall/make.go#new...
src/cmd/goinstall/make.go:143: {.section GoFiles}
On 2011/06/24 01:39:24, r wrote:
> doesn't this put you down a level by setting the cursor? that is, i don't
> understand why you say GoFiles twice

We don't wanna output GOFILES= unless there's at least one.

[r, 2011-06-24 01:52:40]

http://codereview.appspot.com/4627056/diff/3007/src/cmd/goinstall/make.go
File src/cmd/goinstall/make.go (right):

http://codereview.appspot.com/4627056/diff/3007/src/cmd/goinstall/make.go#new...
src/cmd/goinstall/make.go:118: if strings.Index(s, "..") > -1 {
>= 0

or even just strings.Contains

http://codereview.appspot.com/4627056/diff/3007/src/cmd/goinstall/make.go#new...
src/cmd/goinstall/make.go:122: if c := s[i]; c < 0x80 &&
bytes.IndexByte(safeBytes, c) < 0 {
by 0x80 do you mean RuneSelf or are you trying to do something else? i'm unclear

http://codereview.appspot.com/4627056/diff/3007/src/cmd/goinstall/make.go#new...
src/cmd/goinstall/make.go:149: {.repeated section GoFiles}
i guess i'm confused. i expect this to be @ because section set @ to GoFiles.

[adg, 2011-06-24 01:57:43]

http://codereview.appspot.com/4627056/diff/3007/src/cmd/goinstall/make.go
File src/cmd/goinstall/make.go (right):

http://codereview.appspot.com/4627056/diff/3007/src/cmd/goinstall/make.go#new...
src/cmd/goinstall/make.go:118: if strings.Index(s, "..") > -1 {
On 2011/06/24 01:52:40, r wrote:
> >= 0
> 
> or even just strings.Contains

Done.

http://codereview.appspot.com/4627056/diff/3007/src/cmd/goinstall/make.go#new...
src/cmd/goinstall/make.go:122: if c := s[i]; c < 0x80 &&
bytes.IndexByte(safeBytes, c) < 0 {
On 2011/06/24 01:52:40, r wrote:
> by 0x80 do you mean RuneSelf or are you trying to do something else? i'm
unclear

Russ wrote this line. From my reading it's just an ASCII test followed by a list
of allowed characters.

http://codereview.appspot.com/4627056/diff/3007/src/cmd/goinstall/make.go#new...
src/cmd/goinstall/make.go:149: {.repeated section GoFiles}
On 2011/06/24 01:52:40, r wrote:
> i guess i'm confused. i expect this to be @ because section set @ to GoFiles.

Done.

[r, 2011-06-24 02:04:32]

LGTM

[adg, 2011-06-24 03:01:35]

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

goinstall: build with make by default, add -make flag

This is a temporary measure until go/build can build cgo packages.

R=golang-dev, r
CC=golang-dev
http://codereview.appspot.com/4627056