code review 137430043: go.tools/buildutil: AllPackages: enumerate all packages...

go.tools/go/buildutil: AllPackages: enumerate all packages in a Go workspace.

This function has been copied at least 6 times throughout
go.tools.  This implementation is superior since it does
all I/O through the virtualized go/build file system, and it
is highly parallel (and much faster).

We expose two flavours, simple (for existing tests) and
parallel (for high-performance tools such as gorename).

This CL creates the go/buildutil package, which is intended for
utilities related to go/build.

+ test.

[adonovan, 2014-09-09 19:29:38]

Hello gri@golang.org (cc: golang-codereviews@googlegroups.com),

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

[gri, 2014-09-09 19:52:37]

https://codereview.appspot.com/137430043/diff/40001/buildutil/allpackages.go
File buildutil/allpackages.go (right):

https://codereview.appspot.com/137430043/diff/40001/buildutil/allpackages.go#...
buildutil/allpackages.go:1: // Package buildutil provides utilities related to
the go/build
missing copyright notice

https://codereview.appspot.com/137430043/diff/40001/buildutil/allpackages.go#...
buildutil/allpackages.go:18: //
the result doesn't contain empty packages - it contains imports paths for
directories that are empty

https://codereview.appspot.com/137430043/diff/40001/buildutil/allpackages.go#...
buildutil/allpackages.go:23: // which must concurrency-safe.
must be

https://codereview.appspot.com/137430043/diff/40001/buildutil/allpackages.go#...
buildutil/allpackages.go:25: func AllPackages(ctxt *build.Context) []string {
s/AllPackages/AllPackagesList/

https://codereview.appspot.com/137430043/diff/40001/buildutil/allpackages.go#...
buildutil/allpackages.go:44: func AllPackagesParallel(ctxt *build.Context, found
func(string)) {
Parallel is not the right word - it should be concurrent.

Also, I'd rather have an extra arg: concurrent bool . That way I can use this
even if I don't want to be concurrent, w/o allocating a list.

https://codereview.appspot.com/137430043/diff/40001/buildutil/allpackages.go#...
buildutil/allpackages.go:67: root = filepath.Clean(root) +
string(os.PathSeparator)
doesn't Clean already add a separator?

https://codereview.appspot.com/137430043/diff/40001/buildutil/allpackages.go#...
buildutil/allpackages.go:73: // Prune search if we encounter any directory with
these base name:
s/name/names/

https://codereview.appspot.com/137430043/diff/40001/buildutil/allpackages.go#...
buildutil/allpackages.go:83: case "builtin", "pkg":
do we still need "pkg" ?

https://codereview.appspot.com/137430043/diff/40001/buildutil/allpackages_tes...
File buildutil/allpackages_test.go (right):

https://codereview.appspot.com/137430043/diff/40001/buildutil/allpackages_tes...
buildutil/allpackages_test.go:1: package buildutil_test
missing copyright notice

https://codereview.appspot.com/137430043/diff/40001/buildutil/allpackages_tes...
buildutil/allpackages_test.go:18: if got, want := len(all), 250; got < want {
not a big fan of this notation

const want = 250
if len(all) < want { ... }

using len(all) is just as efficient as introducing a new variable - there's no
need to be clever here

[adonovan, 2014-09-09 20:22:52]

https://codereview.appspot.com/137430043/diff/40001/buildutil/allpackages.go
File buildutil/allpackages.go (right):

https://codereview.appspot.com/137430043/diff/40001/buildutil/allpackages.go#...
buildutil/allpackages.go:1: // Package buildutil provides utilities related to
the go/build
On 2014/09/09 19:52:36, gri wrote:
> missing copyright notice

Done.

https://codereview.appspot.com/137430043/diff/40001/buildutil/allpackages.go#...
buildutil/allpackages.go:18: //
On 2014/09/09 19:52:36, gri wrote:
> the result doesn't contain empty packages - it contains imports paths for
> directories that are empty

Done.

https://codereview.appspot.com/137430043/diff/40001/buildutil/allpackages.go#...
buildutil/allpackages.go:23: // which must concurrency-safe.
On 2014/09/09 19:52:36, gri wrote:
> must be

Done.

https://codereview.appspot.com/137430043/diff/40001/buildutil/allpackages.go#...
buildutil/allpackages.go:25: func AllPackages(ctxt *build.Context) []string {
On 2014/09/09 19:52:36, gri wrote:
> s/AllPackages/AllPackagesList/

Done.

https://codereview.appspot.com/137430043/diff/40001/buildutil/allpackages.go#...
buildutil/allpackages.go:44: func AllPackagesParallel(ctxt *build.Context, found
func(string)) {
On 2014/09/09 19:52:36, gri wrote:
> Parallel is not the right word - it should be concurrent.
> 
> Also, I'd rather have an extra arg: concurrent bool . That way I can use this
> even if I don't want to be concurrent, w/o allocating a list.

Yes, I think we need something like that, but this is just the N=1 special case
of concurrency limiting, which we may want to impose for the sake of the
underlying file system.  I'd rather not implement it until we know what we need.
 There's already a TODO item here.

https://codereview.appspot.com/137430043/diff/40001/buildutil/allpackages.go#...
buildutil/allpackages.go:67: root = filepath.Clean(root) +
string(os.PathSeparator)
On 2014/09/09 19:52:36, gri wrote:
> doesn't Clean already add a separator?

Not at the end.  I add the trailing / so that TrimPrefix computes the relative
path.

https://codereview.appspot.com/137430043/diff/40001/buildutil/allpackages.go#...
buildutil/allpackages.go:73: // Prune search if we encounter any directory with
these base name:
On 2014/09/09 19:52:36, gri wrote:
> s/name/names/

Done.

https://codereview.appspot.com/137430043/diff/40001/buildutil/allpackages.go#...
buildutil/allpackages.go:83: case "builtin", "pkg":
On 2014/09/09 19:52:36, gri wrote:
> do we still need "pkg" ?

Nope.  Done.

https://codereview.appspot.com/137430043/diff/40001/buildutil/allpackages_tes...
File buildutil/allpackages_test.go (right):

https://codereview.appspot.com/137430043/diff/40001/buildutil/allpackages_tes...
buildutil/allpackages_test.go:1: package buildutil_test
On 2014/09/09 19:52:36, gri wrote:
> missing copyright notice

Done.

https://codereview.appspot.com/137430043/diff/40001/buildutil/allpackages_tes...
buildutil/allpackages_test.go:18: if got, want := len(all), 250; got < want {
On 2014/09/09 19:52:37, gri wrote:
> not a big fan of this notation
> 
> const want = 250
> if len(all) < want { ... }
> 
> using len(all) is just as efficient as introducing a new variable - there's no
> need to be clever here

Done.

[adonovan, 2014-09-09 20:23:35]

Hello gri@golang.org (cc: golang-codereviews@googlegroups.com),

Please take another look.

[gri, 2014-09-09 20:37:23]

update the CL desc

- tone down the superiority piece
- remove parallel

https://codereview.appspot.com/137430043/diff/80001/go/buildutil/allpackages.go
File go/buildutil/allpackages.go (right):

https://codereview.appspot.com/137430043/diff/80001/go/buildutil/allpackages....
go/buildutil/allpackages.go:48: func AllPackagesConcurrent(ctxt *build.Context,
found func(string)) {
func(string, error)

get rid of one of the TODOs

https://codereview.appspot.com/137430043/diff/80001/go/buildutil/allpackages....
go/buildutil/allpackages.go:52: // Explore each root in parallel.
// Explore each root concurrently.

[adonovan, 2014-09-09 20:54:55]

https://codereview.appspot.com/137430043/diff/80001/go/buildutil/allpackages.go
File go/buildutil/allpackages.go (right):

https://codereview.appspot.com/137430043/diff/80001/go/buildutil/allpackages....
go/buildutil/allpackages.go:48: func AllPackagesConcurrent(ctxt *build.Context,
found func(string)) {
On 2014/09/09 20:37:22, gri wrote:
> func(string, error)
> 
> get rid of one of the TODOs

Which one?  None is redundant.

https://codereview.appspot.com/137430043/diff/80001/go/buildutil/allpackages....
go/buildutil/allpackages.go:52: // Explore each root in parallel.
On 2014/09/09 20:37:22, gri wrote:
> // Explore each root concurrently.

But this is parallelism!  The structure of the problem is parallel, and the
execution is parallel. 

I thought Rob had a talk saying that concurrency is the expression of a program
as the composition of independent activities, and that parallelism is the
property of a problem, algorithm or piece of hardware, that n copies of the same
activity could run at the same time.

[gri, 2014-09-09 21:11:45]

https://codereview.appspot.com/137430043/diff/120001/go/buildutil/allpackages.go
File go/buildutil/allpackages.go (right):

https://codereview.appspot.com/137430043/diff/120001/go/buildutil/allpackages...
go/buildutil/allpackages.go:22: // of $GOPATH).
should mention that they are sorted

https://codereview.appspot.com/137430043/diff/120001/go/buildutil/allpackages...
go/buildutil/allpackages.go:31: var pkgs []string
s/pkgs/list/

https://codereview.appspot.com/137430043/diff/120001/go/buildutil/allpackages...
go/buildutil/allpackages.go:46: // AllPackagesConcurrent is highly parallel, so
the found function must be
leave away "highly parallel" - no need to describe implementation details.

// The found function and the  build.Context virtual file system accessors must
be concurrency safe.

https://codereview.appspot.com/137430043/diff/120001/go/buildutil/allpackages...
go/buildutil/allpackages.go:54: // Explore each root in parallel.
// Explore roots concurrently.

https://codereview.appspot.com/137430043/diff/120001/go/buildutil/allpackages...
go/buildutil/allpackages.go:89: case "builtin":
"unsafe" is missing

The documentation should probably say that "fake" packages are excluded.

https://codereview.appspot.com/137430043/diff/120001/go/buildutil/allpackages...
go/buildutil/allpackages.go:101: log.Printf("readdir failed: %s", err)
call found with empty path and error?

[adonovan, 2014-09-09 21:31:35]

https://codereview.appspot.com/137430043/diff/120001/go/buildutil/allpackages.go
File go/buildutil/allpackages.go (right):

https://codereview.appspot.com/137430043/diff/120001/go/buildutil/allpackages...
go/buildutil/allpackages.go:22: // of $GOPATH).
On 2014/09/09 21:11:44, gri wrote:
> should mention that they are sorted

Why specify that?  I sorted it in the interests of determinism, but we needn't
commit to it.

https://codereview.appspot.com/137430043/diff/120001/go/buildutil/allpackages...
go/buildutil/allpackages.go:31: var pkgs []string
On 2014/09/09 21:11:44, gri wrote:
> s/pkgs/list/

Done.

https://codereview.appspot.com/137430043/diff/120001/go/buildutil/allpackages...
go/buildutil/allpackages.go:46: // AllPackagesConcurrent is highly parallel, so
the found function must be
On 2014/09/09 21:11:44, gri wrote:
> leave away "highly parallel" - no need to describe implementation details.
> 
> // The found function and the  build.Context virtual file system accessors
must
> be concurrency safe.

Done.

https://codereview.appspot.com/137430043/diff/120001/go/buildutil/allpackages...
go/buildutil/allpackages.go:54: // Explore each root in parallel.
On 2014/09/09 21:11:44, gri wrote:
> // Explore roots concurrently.

Comment deleted.

https://codereview.appspot.com/137430043/diff/120001/go/buildutil/allpackages...
go/buildutil/allpackages.go:89: case "builtin":
On 2014/09/09 21:11:44, gri wrote:
> "unsafe" is missing
> 
> The documentation should probably say that "fake" packages are excluded. 

"unsafe" belongs here; it is a valid package import path that you can use in an
import declaration.

https://codereview.appspot.com/137430043/diff/120001/go/buildutil/allpackages...
go/buildutil/allpackages.go:101: log.Printf("readdir failed: %s", err)
On 2014/09/09 21:11:44, gri wrote:
> call found with empty path and error?
Done.

[gri, 2014-09-09 22:05:44]

https://codereview.appspot.com/137430043/diff/120001/go/buildutil/allpackages.go
File go/buildutil/allpackages.go (right):

https://codereview.appspot.com/137430043/diff/120001/go/buildutil/allpackages...
go/buildutil/allpackages.go:22: // of $GOPATH).
On 2014/09/09 21:31:34, adonovan wrote:
> On 2014/09/09 21:11:44, gri wrote:
> > should mention that they are sorted
> 
> Why specify that?  I sorted it in the interests of determinism, but we needn't
> commit to it.

Because we do whenever we return something is sorted. If it's not specified, I
cannot rely on it. I may be inclined to sort again. That seems weird.

https://codereview.appspot.com/137430043/diff/140001/go/buildutil/allpackages.go
File go/buildutil/allpackages.go (right):

https://codereview.appspot.com/137430043/diff/140001/go/buildutil/allpackages...
go/buildutil/allpackages.go:43: // build context (e.g. $GOROOT or an element of
$GOPATH).
It should explain the error parameter and what path = "" means.

https://codereview.appspot.com/137430043/diff/140001/go/buildutil/allpackages...
go/buildutil/allpackages.go:48: func AllPackagesConcurrent(ctxt *build.Context,
found func(string, error)) {
perhaps mention path string, err error for documention

https://codereview.appspot.com/137430043/diff/140001/go/buildutil/allpackages...
go/buildutil/allpackages.go:50: // and reporting of ReadDir errors?
this is now done

[adonovan, 2014-09-09 22:12:27]

https://codereview.appspot.com/137430043/diff/120001/go/buildutil/allpackages.go
File go/buildutil/allpackages.go (right):

https://codereview.appspot.com/137430043/diff/120001/go/buildutil/allpackages...
go/buildutil/allpackages.go:22: // of $GOPATH).
On 2014/09/09 22:05:44, gri wrote:
> On 2014/09/09 21:31:34, adonovan wrote:
> > On 2014/09/09 21:11:44, gri wrote:
> > > should mention that they are sorted
> > 
> > Why specify that?  I sorted it in the interests of determinism, but we
needn't
> > commit to it.
> 
> Because we do whenever we return something is sorted. If it's not specified, I
> cannot rely on it. I may be inclined to sort again. That seems weird.

Done.

https://codereview.appspot.com/137430043/diff/140001/go/buildutil/allpackages.go
File go/buildutil/allpackages.go (right):

https://codereview.appspot.com/137430043/diff/140001/go/buildutil/allpackages...
go/buildutil/allpackages.go:43: // build context (e.g. $GOROOT or an element of
$GOPATH).
On 2014/09/09 22:05:44, gri wrote:
> It should explain the error parameter and what path = "" means.

Done.

https://codereview.appspot.com/137430043/diff/140001/go/buildutil/allpackages...
go/buildutil/allpackages.go:48: func AllPackagesConcurrent(ctxt *build.Context,
found func(string, error)) {
On 2014/09/09 22:05:44, gri wrote:
> perhaps mention path string, err error for documention

Done.

https://codereview.appspot.com/137430043/diff/140001/go/buildutil/allpackages...
go/buildutil/allpackages.go:50: // and reporting of ReadDir errors?
On 2014/09/09 22:05:44, gri wrote:
> this is now done

Done.

[gri, 2014-09-09 22:20:50]

LGTM

but please consider the rename.

https://codereview.appspot.com/137430043/diff/160001/go/buildutil/allpackages.go
File go/buildutil/allpackages.go (right):

https://codereview.appspot.com/137430043/diff/160001/go/buildutil/allpackages...
go/buildutil/allpackages.go:52: // TODO(adonovan): options for serial traversal
and/or rate-limiting.
remove this

- rate limiting is overkill
- serial traversal is odd for a function called xConcurrent
- it's just a TODO for extra work - it's not missing functionality

Actually, I would just call it AllPackages. We don't call each concurrent
function ...Concurrent.

https://codereview.appspot.com/137430043/diff/160001/go/buildutil/allpackages...
go/buildutil/allpackages.go:106: }
insert a newline

[adonovan, 2014-09-09 22:36:09]

https://codereview.appspot.com/137430043/diff/160001/go/buildutil/allpackages.go
File go/buildutil/allpackages.go (right):

https://codereview.appspot.com/137430043/diff/160001/go/buildutil/allpackages...
go/buildutil/allpackages.go:52: // TODO(adonovan): options for serial traversal
and/or rate-limiting.
On 2014/09/09 22:20:50, gri wrote:
> remove this
> 
> - rate limiting is overkill
> - serial traversal is odd for a function called xConcurrent
> - it's just a TODO for extra work - it's not missing functionality
> 
> Actually, I would just call it AllPackages. We don't call each concurrent
> function ...Concurrent.

Done.

https://codereview.appspot.com/137430043/diff/160001/go/buildutil/allpackages...
go/buildutil/allpackages.go:106: }
On 2014/09/09 22:20:50, gri wrote:
> insert a newline

Done.

[adonovan, 2014-09-09 22:39:17]

*** Submitted as
https://code.google.com/p/go/source/detail?r=f7cd03c2866c&repo=tools ***

go.tools/go/buildutil: AllPackages: enumerate all packages in a Go workspace.

This function has been copied at least 6 times throughout
go.tools.  This implementation is superior since it does
all I/O through the virtualized go/build file system, and it
is highly parallel (and much faster).

We expose two flavours, simple (for existing tests) and
parallel (for high-performance tools such as gorename).

This CL creates the go/buildutil package, which is intended for
utilities related to go/build.

+ test.

LGTM=gri
R=gri
CC=golang-codereviews
https://codereview.appspot.com/137430043

[gri, 2014-09-10 01:21:03]

https://codereview.appspot.com/137430043/diff/170001/go/buildutil/allpackages.go
File go/buildutil/allpackages.go (right):

https://codereview.appspot.com/137430043/diff/170001/go/buildutil/allpackages...
go/buildutil/allpackages.go:64: func allPackages(ctxt *build.Context, root
string, found func(string, error)) {
FYI, elsewhere in the std lib, we have walker functions of the form

func(path string, err error) error

where the returned error controls whether the walk/iteration should continue or
not. May or may not make sense in a concurrent version.

[adonovan, 2014-09-10 02:39:40]

Yeah; the original filepath.Walk had this feature and we used it to prune
the "testdata" subtree, for example, but now all the pruning logic is
hardcoded and the func is just an observer, which I think makes sense for
this use-case.

I had an idea on the way home.  Are these better names?
  func AllPackages(*build.Context) []string
  func ForEachPackage(*build.Context, func(string, error))





On 9 September 2014 21:21, <gri@golang.org> wrote:

>
> https://codereview.appspot.com/137430043/diff/170001/go/
> buildutil/allpackages.go
> File go/buildutil/allpackages.go (right):
>
> https://codereview.appspot.com/137430043/diff/170001/go/
> buildutil/allpackages.go#newcode64
> go/buildutil/allpackages.go:64: func allPackages(ctxt *build.Context,
> root string, found func(string, error)) {
> FYI, elsewhere in the std lib, we have walker functions of the form
>
> func(path string, err error) error
>
> where the returned error controls whether the walk/iteration should
> continue or not. May or may not make sense in a concurrent version.
>
> https://codereview.appspot.com/137430043/
>

[gri, 2014-09-10 18:58:40]

I like AllPackages for the list.

How about EnumPackage instead of ForEachPackage?

- gri


On Tue, Sep 9, 2014 at 7:39 PM, Alan Donovan <adonovan@google.com> wrote:

> Yeah; the original filepath.Walk had this feature and we used it to prune
> the "testdata" subtree, for example, but now all the pruning logic is
> hardcoded and the func is just an observer, which I think makes sense for
> this use-case.
>
> I had an idea on the way home.  Are these better names?
>   func AllPackages(*build.Context) []string
>   func ForEachPackage(*build.Context, func(string, error))
>
>
>
>
>
> On 9 September 2014 21:21, <gri@golang.org> wrote:
>
>>
>> https://codereview.appspot.com/137430043/diff/170001/go/
>> buildutil/allpackages.go
>> File go/buildutil/allpackages.go (right):
>>
>> https://codereview.appspot.com/137430043/diff/170001/go/
>> buildutil/allpackages.go#newcode64
>> go/buildutil/allpackages.go:64: func allPackages(ctxt *build.Context,
>> root string, found func(string, error)) {
>> FYI, elsewhere in the std lib, we have walker functions of the form
>>
>> func(path string, err error) error
>>
>> where the returned error controls whether the walk/iteration should
>> continue or not. May or may not make sense in a concurrent version.
>>
>> https://codereview.appspot.com/137430043/
>>
>
>

[adonovan, 2014-09-11 18:10:20]

On 10 September 2014 14:58, Robert Griesemer <gri@golang.org> wrote:

> I like AllPackages for the list.
>
> How about EnumPackage instead of ForEachPackage?
>

I prefer ForEach; it has a long tradition of specifically meaning "apply
this function to each item".  Enumerate has a range of meanings: C 'enum'
data types, Java 'Enumeraton' iterators, Python's enumerate builtin
analogous to "for i, x := range slice".

[adonovan, 2014-09-11 18:24:30]

https://codereview.appspot.com/142930043

On 11 September 2014 14:10, Alan Donovan <adonovan@google.com> wrote:

> On 10 September 2014 14:58, Robert Griesemer <gri@golang.org> wrote:
>
>> I like AllPackages for the list.
>>
>> How about EnumPackage instead of ForEachPackage?
>>
>
> I prefer ForEach; it has a long tradition of specifically meaning "apply
> this function to each item".  Enumerate has a range of meanings: C 'enum'
> data types, Java 'Enumeraton' iterators, Python's enumerate builtin
> analogous to "for i, x := range slice".
>
>