code review 5713043: go/build: replace FindTree, ScanDir, Tree, DirInfo with...

go/build: replace FindTree, ScanDir, Tree, DirInfo with Import, Package

This is an API change, but one I have been promising would
happen when it was clear what the go command needed.

This is basically a complete replacement of what used to be here.

build.Tree is gone.

build.DirInfo is expanded and now called build.Package.

build.FindTree is now build.Import(package, srcDir, build.FindOnly).
The returned *Package contains information that FindTree returned,
but applicable only to a single package.

build.ScanDir is now build.ImportDir.

build.FindTree+build.ScanDir is now build.Import.

The new Import API allows specifying the source directory,
in order to resolve local imports (import "./foo") and also allows
scanning of packages outside of $GOPATH.  They will come back
with less information in the Package, but they will still work.

The old go/build API exposed both too much and too little.
This API is much closer to what the go command needs,
and it works well enough in the other places where it is
used.  Path is gone, so it can no longer be misused.  (Fixes issue 2749.)

This CL updates clients of go/build other than the go command.
The go command changes are in a separate CL, to be submitted
at the same time.

[rsc, 2012-02-29 20:48:09]

Hello golang-dev@googlegroups.com,

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

[r, 2012-02-29 22:12:10]

http://codereview.appspot.com/5713043/diff/1002/src/pkg/go/build/build.go
File src/pkg/go/build/build.go (right):

http://codereview.appspot.com/5713043/diff/1002/src/pkg/go/build/build.go#new...
src/pkg/go/build/build.go:102: JoinPath func(elem ...string) string
this is fine, but it feels odd that these funcs aren't methods of an embedded
interface object especially since *Context is already almost the thing

http://codereview.appspot.com/5713043/diff/1002/src/pkg/go/build/build.go#new...
src/pkg/go/build/build.go:202: func (ctxt *Context) openFile(path string)
(io.ReadCloser, error) {
function comments would be welcome, simple though they are.
even when trivial, they give a place to add details to aid the reader.

[rsc, 2012-02-29 22:16:49]

On Wed, Feb 29, 2012 at 17:12,  <r@golang.org> wrote:
> src/pkg/go/build/build.go:102: JoinPath func(elem ...string) string
> this is fine, but it feels odd that these funcs aren't methods of an
> embedded interface object especially since *Context is already almost
> the thing

It's definitely non-standard, but having separate function fields
lets callers override just a few of the methods if necessary.
The go command, for example, overrides just ReadDir and
JoinPath in order to allow packages made from the files named
on the command line.

>
http://codereview.appspot.com/5713043/diff/1002/src/pkg/go/build/build.go#new...
> src/pkg/go/build/build.go:202: func (ctxt *Context) openFile(path
> string) (io.ReadCloser, error) {
> function comments would be welcome, simple though they are.
> even when trivial, they give a place to add details to aid the reader.

Done.

[brainman, 2012-02-29 22:42:54]

http://codereview.appspot.com/5713043/diff/5001/src/pkg/go/build/build.go
File src/pkg/go/build/build.go (right):

http://codereview.appspot.com/5713043/diff/5001/src/pkg/go/build/build.go#new...
src/pkg/go/build/build.go:187: if !strings.HasPrefix(dir, root) {
I think we will have problems with this on windows. But, we agreed to disagree.
:-)

http://codereview.appspot.com/5713043/diff/5001/src/pkg/go/build/build.go#new...
src/pkg/go/build/build.go:999: filepath.HasPrefix(path, "./") ||
filepath.HasPrefix(path, "../")
s/filepath/strings/g

[rsc, 2012-02-29 22:46:43]

On Wed, Feb 29, 2012 at 17:42,  <alex.brainman@gmail.com> wrote:
> I think we will have problems with this on windows. But, we agreed to
> disagree. :-)

I would like to be able to depend on filepath.EvalSymlinks
returning the canonical spelling of its argument.
So if root is C:\GO and dir is c:\go\src\cmd,
I would like EvalSymlinks to return strings that
are the actual canonical case recorded in the file system
(say, C:\Go and C:\Go\src\cmd).  Then the strings.HasPrefix
is correct.

Does EvalSymlinks not do that on Windows?
Can we make it do that?

[brainman, 2012-02-29 22:52:50]

On 2012/02/29 22:46:43, rsc wrote:
> On Wed, Feb 29, 2012 at 17:42,  <mailto:alex.brainman@gmail.com> wrote:
> > I think we will have problems with this on windows. But, we agreed to
> > disagree. :-)
> 
> I would like to be able to depend on filepath.EvalSymlinks
> returning the canonical spelling of its argument.
> So if root is C:\GO and dir is c:\go\src\cmd,
> I would like EvalSymlinks to return strings that
> are the actual canonical case recorded in the file system
> (say, C:\Go and C:\Go\src\cmd).  Then the strings.HasPrefix
> is correct.
> 
> Does EvalSymlinks not do that on Windows?

No it does not. It checks that file exists (os.Lstat) and then it Clean the
path.

> Can we make it do that?

Sure. But shouldn't it be called ConvertPathToCanonecalOSForm (or something like
that) instead? EvalSymlinks is confusing for what we are trying to do,
especially considering Windows does not have symlinks.

Alex

[rsc, 2012-02-29 22:58:35]

I know.  Really it should be called RealPath or
something like that, but for now it is called
EvalSymlinks.

[brainman, 2012-02-29 23:06:03]

On 2012/02/29 22:58:35, rsc wrote:
> I know.  Really it should be called RealPath or
> something like that, but for now it is called
> EvalSymlinks.

Fair enough.

I will see if I can find "canonical" name inside EvalSymlinks. I said "sure",
but I really have no idea. :-)

I am still not convinced it is the right way of going about it. But it is
certainly "righter" then before.

Alex

[adg, 2012-03-01 00:21:59]

LGTM

I was troubled by this package since the beginning. This is much better.

On 1 March 2012 09:58, Russ Cox <rsc@golang.org> wrote:
> I know.  Really it should be called RealPath or
> something like that, but for now it is called
> EvalSymlinks.

Hm. http://codereview.appspot.com/4235060/#msg12

[r, 2012-03-01 00:28:03]

Fixes 3118?

[r, 2012-03-01 00:37:10]

Fixes 2775?

http://codereview.appspot.com/5713043/diff/5001/src/pkg/go/build/build.go
File src/pkg/go/build/build.go (right):

http://codereview.appspot.com/5713043/diff/5001/src/pkg/go/build/build.go#new...
src/pkg/go/build/build.go:89: GOROOT      string   // build Go root
the comment is unclear; it sounds like a command
// root of Go build tree

http://codereview.appspot.com/5713043/diff/5001/src/pkg/go/build/build.go#new...
src/pkg/go/build/build.go:90: GOPATH      string   // build Go path
// GOPATH entry used to build this tree
?

http://codereview.appspot.com/5713043/diff/5001/src/pkg/go/build/build.go#new...
src/pkg/go/build/build.go:98: // callers can set the following functions.  It is
allowed to set some
"allowed" sounds odd. OK? legal? or maybe be proactive:
They all have default behaviors, so clients need only set the functions required
to satisfy particular properties.

http://codereview.appspot.com/5713043/diff/5001/src/pkg/go/build/build.go#new...
src/pkg/go/build/build.go:117: // HasSubdir reports whether if dir is a
subdirectory of
s/if // and move "of" after the parens.

http://codereview.appspot.com/5713043/diff/5001/src/pkg/go/build/build.go#new...
src/pkg/go/build/build.go:214: // isFile determines whether path is a file by
trying to open it.
are we guaranteed that opening a directory will fail on all platforms?
why is the implementation so different from isDir?

[rsc, 2012-03-01 01:15:20]

On Wed, Feb 29, 2012 at 19:21, Andrew Gerrand <adg@golang.org> wrote:
>> I know.  Really it should be called RealPath or
>> something like that, but for now it is called
>> EvalSymlinks.
>
> Hm. http://codereview.appspot.com/4235060/#msg12

Originally I thought this function was about evaluating
symlinks.  While it does that, I now understand that
when you need it, you really need a higher-level idea,
that of turning a path name into the canonical identifier
for that path.  I wish we had that.

Russ

[brainman, 2012-03-01 01:56:32]

On 2012/03/01 01:15:20, rsc wrote:
> ... you really need a higher-level idea,
> that of turning a path name into the canonical identifier
> for that path. ...
> 

I never come across of that thing in my life on Windows. I think it is wishful
thinking on your part.

Of course if you ask a particular api for a file path, you will get reproducible
answer. But different api might give you different answer.

For example "drive letter" in c:\a\b. Is it supposed to be "c" or "C"? It is not
part of any "file system". It is sort of mount point. No one "creates" it. It
appears in your path space once you "mount" a file system to it. Either "c" or
"C" will work when you talk about it. Even if all Windows apis refer to it as
"C", but not "c", "c" will still be acceped everywhere. People do not think
about it: "c" or "C" - they use either and it works. Always. So if Go will
stater here saying: "Windows api returns "C", but you are using "c", it is not
*canonical*, you can't do that", people will be confused.

Alex

[rsc, 2012-03-01 04:08:29]

On Wed, Feb 29, 2012 at 20:56,  <alex.brainman@gmail.com> wrote:
> For example "drive letter" in c:\a\b. Is it supposed to be "c" or "C"?
> It is not part of any "file system". It is sort of mount point. No one
> "creates" it. It appears in your path space once you "mount" a file
> system to it. Either "c" or "C" will work when you talk about it. Even
> if all Windows apis refer to it as "C", but not "c", "c" will still be
> acceped everywhere. People do not think about it: "c" or "C" - they use
> either and it works. Always. So if Go will stater here saying: "Windows
> api returns "C", but you are using "c", it is not *canonical*, you can't
> do that", people will be confused.

I am not planning to force people to write paths in canonical form,
but having a conversion to canonical form makes it possible to
write things like HasSubdir: step 1 is convert the paths to canonical
form, and then step 2 is to process them.  Making them different
operations means that you can make step 2 arbitrarily complex and
don't have to stick it in a library.  Part of the problem with HasPrefix
is that it combines canonicalization (in the form of ToLower) and an
operation on the canonical forms (the prefix check), but you might
have other things you want to do instead.

Windows definitely knows the "official" case of a file name: when you
list a directory you always get the same result, even though there
are 2^n possible casings for an n-letter name.  If that's too hard to
extract, it would be a fine stopgap to define that Go's canonical form
for a Windows path is all lowercase and make EvalSymlinks return
strings.ToLower(arg).

Russ

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

On Wed, Feb 29, 2012 at 19:28,  <r@golang.org> wrote:
> Fixes 3118?

No, nor 2775, but definitely part of the answer.

[rsc, 2012-03-01 04:13:23]

Done.

On Wed, Feb 29, 2012 at 19:37,  <r@golang.org> wrote:
>
http://codereview.appspot.com/5713043/diff/5001/src/pkg/go/build/build.go#new...
> src/pkg/go/build/build.go:89: GOROOT      string   // build Go root
> the comment is unclear; it sounds like a command
> // root of Go build tree

// Go root

>
http://codereview.appspot.com/5713043/diff/5001/src/pkg/go/build/build.go#new...
> src/pkg/go/build/build.go:90: GOPATH      string   // build Go path
> // GOPATH entry used to build this tree

// Go path

It's the whole GOPATH, not just one entry.

>
http://codereview.appspot.com/5713043/diff/5001/src/pkg/go/build/build.go#new...
> src/pkg/go/build/build.go:98: // callers can set the following
> functions.  It is allowed to set some
> "allowed" sounds odd. OK? legal? or maybe be proactive:
> They all have default behaviors, so clients need only set the functions
> required
> to satisfy particular properties.

	// callers can set the following functions.  They all have default
	// behaviors that use the local file system, so clients need only set
	// the functions whose behaviors they wish to change.

>
http://codereview.appspot.com/5713043/diff/5001/src/pkg/go/build/build.go#new...
> src/pkg/go/build/build.go:117: // HasSubdir reports whether if dir is a
> subdirectory of
> s/if // and move "of" after the parens.

Did first half; if I move of it says "levels below) of root".

>
http://codereview.appspot.com/5713043/diff/5001/src/pkg/go/build/build.go#new...
> src/pkg/go/build/build.go:214: // isFile determines whether path is a
> file by trying to open it.
> are we guaranteed that opening a directory will fail on all platforms?
> why is the implementation so different from isDir?

// isFile determines whether path is a file by trying to open it.
// It reuses openFile instead of adding another function to the
// list in Context.

[rsc, 2012-03-01 04:14:02]

Hello golang-dev@googlegroups.com, r@golang.org, alex.brainman@gmail.com,
adg@golang.org (cc: golang-dev@googlegroups.com),

Please take another look.

[r, 2012-03-01 04:26:32]

LGTM
yay

[brainman, 2012-03-01 04:28:06]

On 2012/03/01 04:08:29, rsc wrote:

LGTM

> ...
> I am not planning to force people to write paths in canonical form,
> but having a conversion to canonical form makes it possible to
> write things like HasSubdir ...

But you are forcing the to write paths in "canonical" form. Because it is
impossible to write things like HasSubdir otherwise. Then we need to define what
"canonical" form means. And, probably, provide some utilities do convert from /
to it.

> ...  Part of the problem with HasPrefix
> is that it combines canonicalization (in the form of ToLower) and an
> operation on the canonical forms (the prefix check), but you might
> have other things you want to do instead.

I am not argue that our current interface is good. I am argue that if we take
that away, we should replace it with something else.

> Windows definitely knows the "official" case of a file name: when you
> list a directory you always get the same result, even though there
> are 2^n possible casings for an n-letter name. ...

No arguments about that. In fact NTFS file system (that you probably use) will
remember file name as is, case and everything. But this property thrown away at
system interface: you can create file as c:\a.txt and open it as c:\A.TXT later
with no problem.

> ... If that's too hard to
> extract, it would be a fine stopgap to define that Go's canonical form
> for a Windows path is all lowercase and make EvalSymlinks return
> strings.ToLower(arg).

Sounds good.

Alex

[rsc, 2012-03-01 04:37:05]

On Wed, Feb 29, 2012 at 23:28,  <alex.brainman@gmail.com> wrote:
> But you are forcing the to write paths in "canonical" form. Because it
> is impossible to write things like HasSubdir otherwise. Then we need to
> define what "canonical" form means. And, probably, provide some
> utilities do convert from / to it.

Your first sentence lost a noun, so I'm not entirely sure what
you meant, but I'm not forcing anyone or anything to hand us
paths in canonical form.  I am only forcing EvalSymlinks
(which HasSubdir calls, twice, internally) to return paths in
canonical form.

The utility to convert to canonical form is, for better or worse,
EvalSymlinks.
There is no utility to convert away from a canonical form.

Russ

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

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

go/build: replace FindTree, ScanDir, Tree, DirInfo with Import, Package

This is an API change, but one I have been promising would
happen when it was clear what the go command needed.

This is basically a complete replacement of what used to be here.

build.Tree is gone.

build.DirInfo is expanded and now called build.Package.

build.FindTree is now build.Import(package, srcDir, build.FindOnly).
The returned *Package contains information that FindTree returned,
but applicable only to a single package.

build.ScanDir is now build.ImportDir.

build.FindTree+build.ScanDir is now build.Import.

The new Import API allows specifying the source directory,
in order to resolve local imports (import "./foo") and also allows
scanning of packages outside of $GOPATH.  They will come back
with less information in the Package, but they will still work.

The old go/build API exposed both too much and too little.
This API is much closer to what the go command needs,
and it works well enough in the other places where it is
used.  Path is gone, so it can no longer be misused.  (Fixes issue 2749.)

This CL updates clients of go/build other than the go command.
The go command changes are in a separate CL, to be submitted
at the same time.

R=golang-dev, r, alex.brainman, adg
CC=golang-dev
http://codereview.appspot.com/5713043