code review 10234044: testing: add -outputdir flag so "go test" controls wher...

testing: add -outputdir flag so "go test" controls where the files are written
Obscure misfeature now fixed: When run from "go test", profiles were always
written in the package's source directory. This change puts them in the directory
where "go test" is run.
Also fix a couple of problems causing errors in testing.after to go unreported
unless -v was set.

[r, 2013-06-12 19:01:34]

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

I'd like you to review this change to
https://code.google.com/p/go

[rsc, 2013-06-12 19:12:07]

https://codereview.appspot.com/10234044/diff/1/src/pkg/testing/testing.go
File src/pkg/testing/testing.go (right):

https://codereview.appspot.com/10234044/diff/1/src/pkg/testing/testing.go#new...
src/pkg/testing/testing.go:525: // Simple implementation to avoid pulling in
path/filepath.
I hate to say this but I think you have to pull in path/filepath. This code is
wrong on Windows, and you don't want to try to maintain a second copy of the
logic here.

[minux1, 2013-06-12 19:24:00]

cmd/go docs for the new flag -outputdir?

[r, 2013-06-12 21:31:53]

https://codereview.appspot.com/10234044/diff/1/src/pkg/testing/testing.go
File src/pkg/testing/testing.go (right):

https://codereview.appspot.com/10234044/diff/1/src/pkg/testing/testing.go#new...
src/pkg/testing/testing.go:525: // Simple implementation to avoid pulling in
path/filepath.
i think all i need of any complexity is a version of IsAbs, which is small and
easy to parameterize using runtime.GOOS. how does that sound?

the join is ok as is, i believe.

[r, 2013-06-12 21:45:40]

Apologies for my Windows ignorance, but now I don't even see how to do this
using path/filepath, let alone custom code. Can someone capable please advise?

[r, 2013-06-12 22:00:05]

OK, i have a plan. Sheesh.

-rob

[minux1, 2013-06-12 22:04:21]

On Thu, Jun 13, 2013 at 5:45 AM, <r@golang.org> wrote:

> Apologies for my Windows ignorance, but now I don't even see how to do
> this using path/filepath, let alone custom code. Can someone capable
> please advise?
>
i must have missed something obvious, but what's wrong with this direct
translation of your absolute function into one that uses path/filepath?

// absolute returns the file name as an absolute path if outputDir is set
// (assuming *outputDir is itself absolute).
// Simple implementation that uses path/filepath.
func absolute(path string) string {
if *outputDir == "" || path == "" || filepath.IsAbs(path) {
 return path
}
return filepath.Join(*outputDir, path)
}

the only problem i can think of is when *outputDir itself is relative, but
i guess
cmd/go should rewrite any relative directory into absolute path (or one that
relative to the package source directory).

[r, 2013-06-12 22:26:09]

Hello rsc@golang.org, minux.ma@gmail.com (cc: golang-dev@googlegroups.com),

Please take another look.

[r, 2013-06-12 22:26:59]

I don't want to import filepath into testing.

-rob

[r, 2013-06-12 22:27:40]

Also that test is wrong on Windows. (But nothing is right, either.)
See the comments in the updated CL.

-rob

[minux1, 2013-06-12 22:45:50]

On windows, is \a\b\c an absolute path (relative to the current volume)?

could we instead try to solve a simpler problem?
we can say that when outputdir is set, we will always treat other file paths
from arguments as relative to outputdir.
(we can even undocument outputdir and treat it as an internal option just
for the go tool)

then the burden moves to cmd/go to correctly handle outputdir and other
user provided filepaths (because it does import path/filepath, that should
be easy to do)

[iant, 2013-06-12 22:49:19]

https://codereview.appspot.com/10234044/diff/8001/src/pkg/testing/testing.go
File src/pkg/testing/testing.go (right):

https://codereview.appspot.com/10234044/diff/8001/src/pkg/testing/testing.go#...
src/pkg/testing/testing.go:545: } else if path[0] == os.PathSeparator {
Better to call os.IsPathSeparator(path[0]) here; it will handle both `/` and `\`
on Windows.

[rsc, 2013-06-12 22:58:47]

FWIW, making testing depend on path/filepath will bring in only
path/filepath: all the dependencies of path/filepath are already used by
testing.

(go list -f '{{.ImportPath}}: {{.Deps}}' path/filepath testing)

[r, 2013-06-12 23:02:41]

I'd rather not expand testing's dependencies at all, plus it also
requires updating the package tests for path/fliepath.

[r, 2013-06-12 23:03:19]

https://codereview.appspot.com/10234044/diff/8001/src/pkg/testing/testing.go
File src/pkg/testing/testing.go (right):

https://codereview.appspot.com/10234044/diff/8001/src/pkg/testing/testing.go#...
src/pkg/testing/testing.go:545: } else if path[0] == os.PathSeparator {
On 2013/06/12 22:49:20, iant wrote:
> Better to call os.IsPathSeparator(path[0]) here; it will handle both `/` and
`\`
> on Windows.

This condition isn't used on Windows.

[rsc, 2013-06-12 23:05:25]

(It should be.)

https://codereview.appspot.com/10234044/diff/8001/src/pkg/testing/testing.go
File src/pkg/testing/testing.go (right):

https://codereview.appspot.com/10234044/diff/8001/src/pkg/testing/testing.go#...
src/pkg/testing/testing.go:534: // Problem: if path == "C:A" and outputdir ==
"D:B" it's unclear
s/"D:B"/"C:\Go"/

The real problem only occurs when the two are on the same drive (each drive has
its own "current directory").

https://codereview.appspot.com/10234044/diff/8001/src/pkg/testing/testing.go#...
src/pkg/testing/testing.go:545: } else if path[0] == os.PathSeparator {
On 2013/06/12 22:49:20, iant wrote:
> Better to call os.IsPathSeparator(path[0]) here; it will handle both `/` and
`\`
> on Windows.

Yes, and s/else/\n/, so that on Windows, -cpuprofile=\foo is treated as an
absolute path.

[r, 2013-06-12 23:34:57]

Hello rsc@golang.org, minux.ma@gmail.com, iant@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[iant, 2013-06-13 00:02:26]

LGTM

[brainman, 2013-06-13 00:05:19]

https://codereview.appspot.com/10234044/diff/18001/src/pkg/testing/testing.go
File src/pkg/testing/testing.go (right):

https://codereview.appspot.com/10234044/diff/18001/src/pkg/testing/testing.go...
src/pkg/testing/testing.go:524: // (assuming outputDir is itself absolute).
I don't see why outputDir should be absolute path. But, if it should be, this
(assumption) should be documented in help flags. I also think you should verify
that outputDir is in fact absolute path.

https://codereview.appspot.com/10234044/diff/18001/src/pkg/testing/testing.go...
src/pkg/testing/testing.go:526: func absolute(path string) string {
This function looks good to me.

[r, 2013-06-13 00:12:23]

Hello rsc@golang.org, minux.ma@gmail.com, iant@golang.org,
alex.brainman@gmail.com (cc: golang-dev@googlegroups.com),

Please take another look.

[r, 2013-06-13 00:12:53]

The solution to that is to change the function name. PTAL.

[brainman, 2013-06-13 00:14:01]

LGTM. For what it worth :-)

Alex

[minux1, 2013-06-13 00:14:12]

LGTM.

[minux1, 2013-06-13 00:15:48]

On Thu, Jun 13, 2013 at 8:14 AM, <minux.ma@gmail.com> wrote:

> LGTM.

If you don't plan to update "go help testflag".

[rsc, 2013-06-13 01:11:57]

LGTM

No need for docs on this flag. It's only for use by the go command.

[r, 2013-06-13 01:13:37]

*** Submitted as https://code.google.com/p/go/source/detail?r=3018dd1d3e6f ***

testing: add -outputdir flag so "go test" controls where the files are written
Obscure misfeature now fixed: When run from "go test", profiles were always
written in the package's source directory. This change puts them in the
directory
where "go test" is run.
Also fix a couple of problems causing errors in testing.after to go unreported
unless -v was set.

R=rsc, minux.ma, iant, alex.brainman
CC=golang-dev
https://codereview.appspot.com/10234044

[r, 2013-06-13 01:14:22]

You got them anyway.