code review 5708054: cmd/go: fixes

cmd/go: fixes
* Install tools into tool dir always
  (Fixes issue 3049.  Fixes issue 2868.  Fixes issue 2925.)
* Make packages depend on compiler, linker (Fixes issue 3036.)
* Do not recompile packages across roots (Fixes issue 3149.)
* Allow use of binary-only packages (Fixes issue 2775.)
* Avoid duplicate cgo dependencies (Fixes issue 3001.)
* Show less in go get -x.  (Fixes issue 2926.)
* Do not force repo root for existing checkout (Fixes issue 2969.)
* Show full syntax error list always (Fixes issue 2811.)
* Clean arguments before processing (Fixes issue 3034.)
* Add flags for compiler, linker arguments (Fixes issue 2996.)
* Pass flags in make.bash (Fixes issue 3091.)
* Unify build flags, defined in one place.
* Clean up error messages (Fixes issue 3075.  Fixes issue 2923.)
* Support local import paths (Fixes issue 3118.)
* Allow top-level package outside $GOPATH (Fixes issue 3009.)

In addition to these fixes, all commands now take a list of
go files as a way to specify a single package, just as go build and
go run always have.  This means you can:

        go list -json x.go
        go fix x.go
        go vet x.go
        go test x_test.go

Preliminary tests in test.bash.
Mainly testing things that the ordinary build does not.
I don't mind if the script doesn't run on Windows.

I expect that gccgo support is now broken, and I hope that
people will feel free to file issues and/or send CLs to fix it.  :-)

[rsc, 2012-03-01 04:26:17]

Hello golang-dev@googlegroups.com,

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

[dsymonds, 2012-03-01 04:28:37]

Holy moley that's a lot of bug fixes.

[mikio, 2012-03-01 04:33:33]

dependency issue?

// run clean.bash, all.bash
(snip)
cmd/go
/home/mikioh/go/src/cmd/go/build.go:97: undefined: build.Default
/home/mikioh/go/src/cmd/go/pkg.go:63: undefined: build.Package
/home/mikioh/go/src/cmd/go/pkg.go:74: undefined: build.Package
/home/mikioh/go/src/cmd/go/pkg.go:259: undefined: build.Package

[rsc, 2012-03-01 04:35:16]

Yes, you'll need 5713043 too.

[r, 2012-03-01 04:55:37]

LGTM
it does look good
run go vet before submitting - lots of probably untested prints

http://codereview.appspot.com/5708054/diff/6001/src/cmd/go/pkg.go
File src/cmd/go/pkg.go (right):

http://codereview.appspot.com/5708054/diff/6001/src/cmd/go/pkg.go#newcode97
src/cmd/go/pkg.go:97: p.XTestImports = pp.XTestImports
should it be *p = *pp
and then fix up the differences? less likely to rot.

http://codereview.appspot.com/5708054/diff/6001/src/cmd/go/pkg.go#newcode164
src/cmd/go/pkg.go:164: // loadImport scans directory named by arg, which must be
an import path,
s/directory/the &/

http://codereview.appspot.com/5708054/diff/6001/src/cmd/go/pkg.go#newcode315
src/cmd/go/pkg.go:315: p.gofiles = append(p.gofiles, filepath.Join(p.Dir, f))
helper function looks like the answer here

p.gofiles = apppendDir(p.gofiles, p.CgoFiles)
etc.

http://codereview.appspot.com/5708054/diff/6001/src/cmd/go/pkg.go#newcode341
src/cmd/go/pkg.go:341: println("LOCAL?", p.Dir, p1, p1.local, p1.Dir,
p1.ImportPath, path)
???

http://codereview.appspot.com/5708054/diff/6001/src/cmd/go/pkg.go#newcode414
src/cmd/go/pkg.go:414: func isStale(p *Package, topRoot map[string]bool) bool {
// isStale ....

http://codereview.appspot.com/5708054/diff/6001/src/cmd/go/pkg.go#newcode415
src/cmd/go/pkg.go:415: if p.Standard && p.ImportPath == "unsafe" {
builtin too, i think

http://codereview.appspot.com/5708054/diff/6001/src/cmd/go/vcs.go
File src/cmd/go/vcs.go (right):

http://codereview.appspot.com/5708054/diff/6001/src/cmd/go/vcs.go#newcode282
src/cmd/go/vcs.go:282: if len(dir) <= len(srcRoot) || dir[len(srcRoot)] !=
filepath.Separator {
doesn't this reject "a" for "a/"? must path be a a strict prefix?

http://codereview.appspot.com/5708054/diff/6001/src/cmd/go/vet.go
File src/cmd/go/vet.go (right):

http://codereview.appspot.com/5708054/diff/6001/src/cmd/go/vet.go#newcode9
src/cmd/go/vet.go:9: UsageLine: "vet [packages]",
package...

[rog, 2012-03-01 09:54:58]

happy to see these changes (particularly the one that will allow me to execute
"go run $%" under acme...)

http://codereview.appspot.com/5708054/diff/6001/src/cmd/go/doc.go
File src/cmd/go/doc.go (right):

http://codereview.appspot.com/5708054/diff/6001/src/cmd/go/doc.go#newcode488
src/cmd/go/doc.go:488: patterns.  For example, encoding/... expands to all
package
s/package/packages/

[rsc, 2012-03-01 17:00:01]

On Thu, Mar 1, 2012 at 04:54,  <rogpeppe@gmail.com> wrote:
> http://codereview.appspot.com/5708054/diff/6001/src/cmd/go/doc.go#newcode488
> src/cmd/go/doc.go:488: patterns.  For example, encoding/... expands to
> all package
> s/package/packages/

done

[rsc, 2012-03-01 17:09:09]

On Wed, Feb 29, 2012 at 23:55,  <r@golang.org> wrote:
> LGTM
> it does look good
> run go vet before submitting - lots of probably untested prints

done - found some in go/build

> http://codereview.appspot.com/5708054/diff/6001/src/cmd/go/pkg.go#newcode97
> src/cmd/go/pkg.go:97: p.XTestImports = pp.XTestImports
> should it be *p = *pp
> and then fix up the differences? less likely to rot.

would love to, but they are different types.

> http://codereview.appspot.com/5708054/diff/6001/src/cmd/go/pkg.go#newcode164
> src/cmd/go/pkg.go:164: // loadImport scans directory named by arg, which
> must be an import path,
> s/directory/the &/

done

> http://codereview.appspot.com/5708054/diff/6001/src/cmd/go/pkg.go#newcode315
> src/cmd/go/pkg.go:315: p.gofiles = append(p.gofiles,
> filepath.Join(p.Dir, f))
> helper function looks like the answer here
>
> p.gofiles = apppendDir(p.gofiles, p.CgoFiles)
> etc.

done, using pre-existing stringList.

> http://codereview.appspot.com/5708054/diff/6001/src/cmd/go/pkg.go#newcode341
> src/cmd/go/pkg.go:341: println("LOCAL?", p.Dir, p1, p1.local, p1.Dir,
> p1.ImportPath, path)
> ???

can't happen anymore. removed.

> http://codereview.appspot.com/5708054/diff/6001/src/cmd/go/pkg.go#newcode414
> src/cmd/go/pkg.go:414: func isStale(p *Package, topRoot map[string]bool)
> bool {
> // isStale ....

done.

> http://codereview.appspot.com/5708054/diff/6001/src/cmd/go/pkg.go#newcode415
> src/cmd/go/pkg.go:415: if p.Standard && p.ImportPath == "unsafe" {
> builtin too, i think

not sure what you meant.  changed to
// fake, builtin package

it comes up because there is a fake package in src/pkg/unsafe.

> http://codereview.appspot.com/5708054/diff/6001/src/cmd/go/vcs.go#newcode282
> src/cmd/go/vcs.go:282: if len(dir) <= len(srcRoot) || dir[len(srcRoot)]
> != filepath.Separator {
> doesn't this reject "a" for "a/"? must path be a a strict prefix?

the cleaning should remove trailing slashes, but yes
this is requiring a strict prefix.
added a comment.

> http://codereview.appspot.com/5708054/diff/6001/src/cmd/go/vet.go#newcode9
> src/cmd/go/vet.go:9: UsageLine: "vet [packages]",
> package...

i used packages because of 'go help packages', also because sometimes
one argument can mean multiple packages and ..., while standard in our
world, is not really standard on modern unix and also to avoid confusion
between ... meaning repetition in a usage message and ... meaning the
wildcard that can appear there.

[rsc, 2012-03-01 17:12:25]

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

cmd/go: fixes
* Install tools into tool dir always
  (Fixes issue 3049.  Fixes issue 2868.  Fixes issue 2925.)
* Make packages depend on compiler, linker (Fixes issue 3036.)
* Do not recompile packages across roots (Fixes issue 3149.)
* Allow use of binary-only packages (Fixes issue 2775.)
* Avoid duplicate cgo dependencies (Fixes issue 3001.)
* Show less in go get -x.  (Fixes issue 2926.)
* Do not force repo root for existing checkout (Fixes issue 2969.)
* Show full syntax error list always (Fixes issue 2811.)
* Clean arguments before processing (Fixes issue 3034.)
* Add flags for compiler, linker arguments (Fixes issue 2996.)
* Pass flags in make.bash (Fixes issue 3091.)
* Unify build flags, defined in one place.
* Clean up error messages (Fixes issue 3075.  Fixes issue 2923.)
* Support local import paths (Fixes issue 3118.)
* Allow top-level package outside $GOPATH (Fixes issue 3009.)

In addition to these fixes, all commands now take a list of
go files as a way to specify a single package, just as go build and
go run always have.  This means you can:

        go list -json x.go
        go fix x.go
        go vet x.go
        go test x_test.go

Preliminary tests in test.bash.
Mainly testing things that the ordinary build does not.
I don't mind if the script doesn't run on Windows.

I expect that gccgo support is now broken, and I hope that
people will feel free to file issues and/or send CLs to fix it.  :-)

R=golang-dev, dsymonds, r, rogpeppe
CC=golang-dev
http://codereview.appspot.com/5708054

[mattn, 2012-03-02 10:50:08]

This break on windows.

-------------
WORK=C:\DOCUME~1\mattn\LOCALS~1\Temp\go-build794590762
mkdir -p
$WORK\local\C:\temp\github\go-mattn\src\github.com\mattn\go-sqlite3\_obj\
mkdir C:\DOCUME~1\mattn\LOCALS~1\Temp\go-build794590762\local\C:: The filename,
directory name, or volume label syntax is incorrect.
-------------



On 2012/03/01 17:12:25, rsc wrote:
> *** Submitted as http://code.google.com/p/go/source/detail?r=a461bcce05f6 ***
> 
> cmd/go: fixes
> * Install tools into tool dir always
>   (Fixes issue 3049.  Fixes issue 2868.  Fixes issue 2925.)
> * Make packages depend on compiler, linker (Fixes issue 3036.)
> * Do not recompile packages across roots (Fixes issue 3149.)
> * Allow use of binary-only packages (Fixes issue 2775.)
> * Avoid duplicate cgo dependencies (Fixes issue 3001.)
> * Show less in go get -x.  (Fixes issue 2926.)
> * Do not force repo root for existing checkout (Fixes issue 2969.)
> * Show full syntax error list always (Fixes issue 2811.)
> * Clean arguments before processing (Fixes issue 3034.)
> * Add flags for compiler, linker arguments (Fixes issue 2996.)
> * Pass flags in make.bash (Fixes issue 3091.)
> * Unify build flags, defined in one place.
> * Clean up error messages (Fixes issue 3075.  Fixes issue 2923.)
> * Support local import paths (Fixes issue 3118.)
> * Allow top-level package outside $GOPATH (Fixes issue 3009.)
> 
> In addition to these fixes, all commands now take a list of
> go files as a way to specify a single package, just as go build and
> go run always have.  This means you can:
> 
>         go list -json x.go
>         go fix x.go
>         go vet x.go
>         go test x_test.go
> 
> Preliminary tests in test.bash.
> Mainly testing things that the ordinary build does not.
> I don't mind if the script doesn't run on Windows.
> 
> I expect that gccgo support is now broken, and I hope that
> people will feel free to file issues and/or send CLs to fix it.  :-)
> 
> R=golang-dev, dsymonds, r, rogpeppe
> CC=golang-dev
> http://codereview.appspot.com/5708054

[rsc, 2012-03-02 16:38:18]

On Fri, Mar 2, 2012 at 05:50,  <mattn.jp@gmail.com> wrote:
> This break on windows.
>
> -------------
> WORK=C:\DOCUME~1\mattn\LOCALS~1\Temp\go-build794590762
> mkdir -p
> $WORK\local\C:\temp\github\go-mattn\src\github.com\mattn\go-sqlite3\_obj\
> mkdir C:\DOCUME~1\mattn\LOCALS~1\Temp\go-build794590762\local\C:: The
> filename, directory name, or volume label syntax is incorrect.
> -------------

Sorry about that.  That's issue 3169 and is today's project.

[mattn, 2012-03-04 13:52:24]

Don't mind. And thank you for working about this. :)

On Saturday, March 3, 2012 1:38:18 AM UTC+9, rsc wrote:
On Fri, Mar 2, 2012 at 05:50, <mattn.jp@gmail.com> wrote:
> This break on windows.
>
> -------------
> WORK=C:\DOCUME~1\mattn\LOCALS~1\Temp\go-build794590762
> mkdir -p
> $WORK\local\C:\temp\github\go-mattn\src\github.com\mattn\go-sqlite3\_obj\
> mkdir C:\DOCUME~1\mattn\LOCALS~1\Temp\go-build794590762\local\C:: The
> filename, directory name, or volume label syntax is incorrect.
> -------------

Sorry about that. That's issue 3169 and is today's project.