code review 7129048: go/build: ImportDir reject directories that don't exist

go/build: ImportDir reject directories that don't exist

Fixes issue 3428.

[adg, 2013-01-17 05:21:39]

Hello golang-dev@googlegroups.com,

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

[adg, 2013-01-17 05:25:51]

This fix works and (IMO) is certainly correct, but I'm not sure that there
isn't a deeper bug in build.Import.

[dfc, 2013-01-17 05:50:53]

LGTM for the simple fix. I share your concern that something deeper may be
incorrect.

https://codereview.appspot.com/7129048/diff/4/src/pkg/go/build/build_test.go
File src/pkg/go/build/build_test.go (right):

https://codereview.appspot.com/7129048/diff/4/src/pkg/go/build/build_test.go#...
src/pkg/go/build/build_test.go:91: 
// golang.org/issue/3248

[remyoudompheng, 2013-01-17 06:09:51]

https://codereview.appspot.com/7129048/diff/4/src/pkg/go/build/build.go
File src/pkg/go/build/build.go (right):

https://codereview.appspot.com/7129048/diff/4/src/pkg/go/build/build.go#newco...
src/pkg/go/build/build.go:425: // It's okay that we didn't find a root
containing dir.
The bug seems intentional.

[adg, 2013-01-17 06:32:57]

On 17 January 2013 17:09, <remyoudompheng@gmail.com> wrote:

> The bug seems intentional.


Well, that's in Import, where it's feasible that if FindOnly is set true
you might want to return success anyway. But should ImportDir ever succeed
when the specified directory doesn't exist?

[rsc, 2013-01-17 14:15:40]

NOT LGTM, sorry

This breaks the property that ImportDir (like Import) always returns a valid
Package, even when returning an error.

Why does the NoGoError toward the bottom of Import not trigger in this case?

[adg, 2013-01-17 23:35:12]

On 18 January 2013 01:15, <rsc@golang.org> wrote:

> This breaks the property that ImportDir (like Import) always returns a
> valid Package, even when returning an error.
>

Fair call, sorry I missed that.


> Why does the NoGoError toward the bottom of Import not trigger in this
> case?
>

It never gets there because this bug is only apparent with the FindOnly
flag set.

Would it be reasonable to add code to the bottom of Import's IsLocalImport
code branch, below this comment

                // It's okay that we didn't find a root containing dir.
                // Keep going with the information we have.

to do a stat and return an error of the nominated local path doesn't
actually exist?

Andrew

[adg, 2013-01-17 23:36:49]

Hello golang-dev@googlegroups.com, dave@cheney.net, remyoudompheng@gmail.com,
rsc@golang.org (cc: golang-dev@googlegroups.com),

Please take another look.

[rsc, 2013-01-18 16:31:08]

https://codereview.appspot.com/7129048/diff/8003/src/pkg/go/build/build.go
File src/pkg/go/build/build.go (right):

https://codereview.appspot.com/7129048/diff/8003/src/pkg/go/build/build.go#ne...
src/pkg/go/build/build.go:423: // Test that p.Dir exists, and if so keep going
with the
I'm still not convinced this isn't breaking things.
Perhaps ImportDir can do this check after Import returns?

if err == nil && !ctxt.IsDir(p.Dir) { 
   err = os.ErrNotExist
}

[adg, 2013-01-18 23:07:12]

https://codereview.appspot.com/7129048/diff/4/src/pkg/go/build/build_test.go
File src/pkg/go/build/build_test.go (right):

https://codereview.appspot.com/7129048/diff/4/src/pkg/go/build/build_test.go#...
src/pkg/go/build/build_test.go:91: 
On 2013/01/17 05:50:53, dfc wrote:
> // golang.org/issue/3248

Done.

https://codereview.appspot.com/7129048/diff/8003/src/pkg/go/build/build.go
File src/pkg/go/build/build.go (right):

https://codereview.appspot.com/7129048/diff/8003/src/pkg/go/build/build.go#ne...
src/pkg/go/build/build.go:423: // Test that p.Dir exists, and if so keep going
with the
On 2013/01/18 16:31:08, rsc wrote:
> I'm still not convinced this isn't breaking things.
> Perhaps ImportDir can do this check after Import returns?
> 
> if err == nil && !ctxt.IsDir(p.Dir) { 
>    err = os.ErrNotExist
> }

Done.

[adg, 2013-01-18 23:07:31]

Hello dave@cheney.net, remyoudompheng@gmail.com, rsc@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[rsc, 2013-01-22 22:44:08]

LGTM after fixing error

(Should include the path itself, and we don't know that it doesn't exist, only
that it's not a directory.)

https://codereview.appspot.com/7129048/diff/3003/src/pkg/go/build/build.go
File src/pkg/go/build/build.go (right):

https://codereview.appspot.com/7129048/diff/3003/src/pkg/go/build/build.go#ne...
src/pkg/go/build/build.go:326: err = os.ErrNotExist
err = fmt.Errorf("%q is not a directory", p.Dir)

[adg, 2013-01-23 00:28:46]

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

go/build: ImportDir reject directories that don't exist

Fixes issue 3428.

R=dave, remyoudompheng, rsc
CC=golang-dev
https://codereview.appspot.com/7129048

[rsc, 2013-01-28 17:47:35]

I don't understand how, but this broke 'godoc net/http'. Rolling back by
hand.