code review 7333046: cmd/godoc: use go/build to determine package and exampl...

cmd/godoc: use go/build to determine package and example files

Also:
- faster code for example extraction
- simplify handling of command documentation:
  all "main" packages are treated as commands
- various minor cleanups along the way

For commands written in Go, any doc.go file containing
documentation must now be part of package main (rather
then package documentation), otherwise the documentation
won't show up in godoc (it will still build, though).

For commands written in C, documentation may still be
in doc.go files defining package documentation, but the
recommended way is to explicitly ignore those files with
a +build ignore constraint to define package main.

Fixes issue 4806.

[gri, 2013-02-18 07:11:00]

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

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

[dave_cheney.net, 2013-02-18 07:16:15]

Good riddance to the documentation package.

On Mon, Feb 18, 2013 at 6:11 PM,  <gri@golang.org> wrote:
> Reviewers: adg, rsc,
>
> Message:
> Hello adg@golang.org, rsc@golang.org (cc: golang-dev@googlegroups.com),
>
> I'd like you to review this change to
> https://code.google.com/p/go
>
>
> Description:
> cmd/godoc: use go/build to determine package and example files
>
> Also:
> - faster code for example extraction
> - simplify handling of command documentation
>   (no special "documentation" packages, instead treat "main"
>   packages as commands).
> - remove special handling of "documentation" packages from go/build
> - various minor cleanups along the way
>
> Fixes issue 4806.
>
> Please review this at https://codereview.appspot.com/7333046/
>
> Affected files:
>   M lib/godoc/package.html
>   M lib/godoc/package.txt
>   M misc/dashboard/builder/doc.go
>   M misc/goplay/doc.go
>   M src/cmd/5a/doc.go
>   M src/cmd/5c/doc.go
>   M src/cmd/5g/doc.go
>   M src/cmd/5l/doc.go
>   M src/cmd/6a/doc.go
>   M src/cmd/6c/doc.go
>   M src/cmd/6g/doc.go
>   M src/cmd/6l/doc.go
>   M src/cmd/8a/doc.go
>   M src/cmd/8c/doc.go
>   M src/cmd/8g/doc.go
>   M src/cmd/8l/doc.go
>   M src/cmd/cc/doc.go
>   M src/cmd/cgo/doc.go
>   M src/cmd/cov/doc.go
>   M src/cmd/fix/doc.go
>   M src/cmd/gc/doc.go
>   M src/cmd/go/doc.go
>   M src/cmd/go/main.go
>   M src/cmd/godoc/dirtrees.go
>   M src/cmd/godoc/doc.go
>   M src/cmd/godoc/godoc.go
>   M src/cmd/godoc/parser.go
>   M src/cmd/gofmt/doc.go
>   M src/cmd/ld/doc.go
>   M src/cmd/nm/doc.go
>   M src/cmd/pack/doc.go
>   M src/cmd/prof/doc.go
>   M src/cmd/vet/doc.go
>   M src/cmd/yacc/doc.go
>   M src/pkg/exp/README
>   M src/pkg/exp/ebnflint/doc.go
>   M src/pkg/exp/gotype/doc.go
>   M src/pkg/go/build/build.go
>
>
> --
>
> ---You received this message because you are subscribed to the Google Groups
> "golang-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-dev+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/groups/opt_out.
>
>

[bradfitz, 2013-02-18 15:42:38]

https://codereview.appspot.com/7333046/diff/13039/src/pkg/go/build/build.go
File src/pkg/go/build/build.go (left):

https://codereview.appspot.com/7333046/diff/13039/src/pkg/go/build/build.go#o...
src/pkg/go/build/build.go:378: //	- .go files in package documentation
this and below are an API change, no?

we can stop using package documentation, but can we make the tool no longer
respect it?

[gri, 2013-02-18 17:07:25]

I suppose that's correct. Existing binaries written in Go containing a
package documentation will have to rename it to main; if they are
written in C they will also have to add // build +ignore .

Let's see what rsc thinks. I'm somewhat in favor of pushing this
forward so we can get rid of all this special-casing which is purely
historic and from a time where we didn't enforce single-package
directories and where there was no go/build and go tool.

- gri

On Mon, Feb 18, 2013 at 7:42 AM,  <bradfitz@golang.org> wrote:
>
> https://codereview.appspot.com/7333046/diff/13039/src/pkg/go/build/build.go
> File src/pkg/go/build/build.go (left):
>
>
https://codereview.appspot.com/7333046/diff/13039/src/pkg/go/build/build.go#o...
> src/pkg/go/build/build.go:378: //       - .go files in package documentation
> this and below are an API change, no?
>
> we can stop using package documentation, but can we make the tool no
> longer respect it?
>
> https://codereview.appspot.com/7333046/

[adg, 2013-02-19 05:19:21]

On 2013/02/18 15:42:38, bradfitz wrote:
> https://codereview.appspot.com/7333046/diff/13039/src/pkg/go/build/build.go
> File src/pkg/go/build/build.go (left):
> 
>
https://codereview.appspot.com/7333046/diff/13039/src/pkg/go/build/build.go#o...
> src/pkg/go/build/build.go:378: //	- .go files in package documentation
> this and below are an API change, no?
> 
> we can stop using package documentation, but can we make the tool no longer
> respect it?

I'm in favor of dropping support for it.

GitHub search yields <2000 results for "package documentation" in Go files:

https://github.com/search?q=%22package+documentation%22+extension%3A.go&type=...

Eyeballing the results, most of these are one of
a) generated cruft (must be some build tool that does this?)
b) instances of doc.go from clones of the main Go repo
c) short docs for commands (much fewer of these)

It's a shame that c should stop working, but there aren't many and they're easy
to find and fix.

[adg, 2013-02-19 05:35:39]

LGTM

https://codereview.appspot.com/7333046/diff/13039/src/cmd/godoc/godoc.go
File src/cmd/godoc/godoc.go (right):

https://codereview.appspot.com/7333046/diff/13039/src/cmd/godoc/godoc.go#newc...
src/cmd/godoc/godoc.go:974: // directories, PageInfo.Dirs is nil. If error
occurred, PageInfo.Err is set
s/error/an error/

https://codereview.appspot.com/7333046/diff/13039/src/cmd/godoc/godoc.go#newc...
src/cmd/godoc/godoc.go:993: if err != nil && !strings.HasPrefix(err.Error(), "no
Go source files") {
if _, nogo := err.(*build.NoGoError); err != nil && !nogo {

https://codereview.appspot.com/7333046/diff/13039/src/cmd/godoc/godoc.go#newc...
src/cmd/godoc/godoc.go:1034: files, err = parseFiles(fset, abspath,
append(pkginfo.TestGoFiles, pkginfo.XTestGoFiles...))
put the append in a separate var
testFiles := append(...

https://codereview.appspot.com/7333046/diff/13039/src/pkg/exp/README
File src/pkg/exp/README (right):

https://codereview.appspot.com/7333046/diff/13039/src/pkg/exp/README#newcode1
src/pkg/exp/README:1: This directory tree contains experimental packages and
This change isn't relevant to this CL

https://codereview.appspot.com/7333046/diff/13039/src/pkg/go/build/build.go
File src/pkg/go/build/build.go (left):

https://codereview.appspot.com/7333046/diff/13039/src/pkg/go/build/build.go#o...
src/pkg/go/build/build.go:378: //	- .go files in package documentation
On 2013/02/18 15:42:39, bradfitz wrote:
> this and below are an API change, no?
> 
> we can stop using package documentation, but can we make the tool no longer
> respect it?

Maybe we can just leave this support in go/build. When I wrote my original mail
about this I thought there would be more to it than this.

[gri, 2013-02-19 05:57:30]

PTAL

https://codereview.appspot.com/7333046/diff/13039/src/cmd/godoc/godoc.go
File src/cmd/godoc/godoc.go (right):

https://codereview.appspot.com/7333046/diff/13039/src/cmd/godoc/godoc.go#newc...
src/cmd/godoc/godoc.go:974: // directories, PageInfo.Dirs is nil. If error
occurred, PageInfo.Err is set
On 2013/02/19 05:35:40, adg wrote:
> s/error/an error/

Done.

https://codereview.appspot.com/7333046/diff/13039/src/cmd/godoc/godoc.go#newc...
src/cmd/godoc/godoc.go:993: if err != nil && !strings.HasPrefix(err.Error(), "no
Go source files") {
On 2013/02/19 05:35:40, adg wrote:
> if _, nogo := err.(*build.NoGoError); err != nil && !nogo {

ah, neat! I missed this.

https://codereview.appspot.com/7333046/diff/13039/src/cmd/godoc/godoc.go#newc...
src/cmd/godoc/godoc.go:1034: files, err = parseFiles(fset, abspath,
append(pkginfo.TestGoFiles, pkginfo.XTestGoFiles...))
On 2013/02/19 05:35:40, adg wrote:
> put the append in a separate var
> testFiles := append(...

Done.

https://codereview.appspot.com/7333046/diff/13039/src/pkg/exp/README
File src/pkg/exp/README (right):

https://codereview.appspot.com/7333046/diff/13039/src/pkg/exp/README#newcode1
src/pkg/exp/README:1: This directory tree contains experimental packages and
On 2013/02/19 05:35:40, adg wrote:
> This change isn't relevant to this CL

excluded

https://codereview.appspot.com/7333046/diff/13039/src/pkg/go/build/build.go
File src/pkg/go/build/build.go (left):

https://codereview.appspot.com/7333046/diff/13039/src/pkg/go/build/build.go#o...
src/pkg/go/build/build.go:378: //	- .go files in package documentation
On 2013/02/19 05:35:40, adg wrote:
> On 2013/02/18 15:42:39, bradfitz wrote:
> > this and below are an API change, no?
> > 
> > we can stop using package documentation, but can we make the tool no longer
> > respect it?
> 
> Maybe we can just leave this support in go/build. When I wrote my original
mail
> about this I thought there would be more to it than this.

Need to re-check. My preference is still to move forward.

[adg, 2013-02-19 06:13:22]

Looping in Gary Burd, one of the more prominent users of go/build.
How would removing support for "package documentation" affect godoc.org?


On 19 February 2013 02:42, <bradfitz@golang.org> wrote:

>
> https://codereview.appspot.**com/7333046/diff/13039/src/**
>
pkg/go/build/build.go<https://codereview.appspot.com/7333046/diff/13039/src/pkg/go/build/build.go>
> File src/pkg/go/build/build.go (left):
>
> https://codereview.appspot.**com/7333046/diff/13039/src/**
>
pkg/go/build/build.go#**oldcode378<https://codereview.appspot.com/7333046/diff/13039/src/pkg/go/build/build.go#oldcode378>
> src/pkg/go/build/build.go:378: //       - .go files in package
> documentation
> this and below are an API change, no?
>
> we can stop using package documentation, but can we make the tool no
> longer respect it?
>
>
https://codereview.appspot.**com/7333046/<https://codereview.appspot.com/7333...
>

[adg, 2013-02-19 06:15:01]

On 19 February 2013 16:57, <gri@golang.org> wrote:

> Need to re-check. My preference is still to move forward.
>

I think we can remove support from the godoc tool, but not from go/build
because of the compatibility guarantee. Happy to hear more opinions but I'm
not optimistic.

Maybe we could add a mode flag that disables 'package documentation' if
that makes it easier for you in godoc.

[rsc, 2013-02-19 16:05:59]

I would love to see package documentation disappear, but I think Andrew is right
that we have to preserve the behavior of go/build (namely, ignoring those
files). Otherwise, if someone has created a package with package p and package
documentation in one directory, it will stop building.

I think that if we drop the go/build change, everything else still works,
though, right?

[gri, 2013-02-19 19:10:50]

PTAL.

I removed the go/build changes. Packages and commands will still build
as before, unchanged.

However, godoc won't honor the documentation package anymore (consider
it an incentive to move on).

Consequences:

1) For pure commands written in Go (rather than C), any documentation
must simply be part of package main (otherwise the file is ignored) -
so all "package documentation" clauses must be changed to "package
main" in these cases. This is the natural and obvious way to do it -
like for any other package. (godoc could consider ignored files, such
as package "documentation", but in general ignored files may belong to
a different build, and thus doing this right would require extracting
the package name - it's not worth the effort to support a deprecated
style).

2) For commands written in C (more precisely, packages w/o any .go
files included in the build), the old style still works. Documentation
is extracted from any ignored .go files in this case. The file may be
ignored because it defines package "documentation" (old style -
deprecated), or because it's explicitly ignored via a build constraint
(new style - recommended).

- gri

On Tue, Feb 19, 2013 at 8:05 AM,  <rsc@golang.org> wrote:
> I would love to see package documentation disappear, but I think Andrew
> is right that we have to preserve the behavior of go/build (namely,
> ignoring those files). Otherwise, if someone has created a package with
> package p and package documentation in one directory, it will stop
> building.
>
> I think that if we drop the go/build change, everything else still
> works, though, right?
>
>
> https://codereview.appspot.com/7333046/

[bradfitz, 2013-02-19 19:18:14]

LGTM

[gri, 2013-02-19 19:20:02]

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

cmd/godoc: use go/build to determine package and example files

Also:
- faster code for example extraction
- simplify handling of command documentation:
  all "main" packages are treated as commands
- various minor cleanups along the way

For commands written in Go, any doc.go file containing
documentation must now be part of package main (rather
then package documentation), otherwise the documentation
won't show up in godoc (it will still build, though).

For commands written in C, documentation may still be
in doc.go files defining package documentation, but the
recommended way is to explicitly ignore those files with
a +build ignore constraint to define package main.

Fixes issue 4806.

R=adg, rsc, dave, bradfitz
CC=golang-dev
https://codereview.appspot.com/7333046

[gburd, 2013-02-20 06:20:12]

> How would removing support for "package documentation" 
> affect godoc.org?

I will remove the godoc.org code to handle "package documentation" when 1.1 is
released. I think it's more important for godoc.org to match the godoc tool than
it is to remain compatible with existing code.

[gri, 2013-02-20 06:28:03]

Thanks. Apologies for jumping the gun here.
- gri

On Tue, Feb 19, 2013 at 10:20 PM,  <gary.burd@gmail.com> wrote:
>> How would removing support for "package documentation"
>> affect godoc.org?
>
>
> I will remove the godoc.org code to handle "package documentation" when
> 1.1 is released. I think it's more important for godoc.org to match the
> godoc tool than it is to remain compatible with existing code.
>
> https://codereview.appspot.com/7333046/

[dave_cheney.net, 2013-02-21 01:45:29]

I believe this has caused a regression, see
https://groups.google.com/d/topic/golang-nuts/Jv70Gj2nSmc/discussion

[gri, 2013-02-21 02:40:43]

Thanks. I answered to that thread. This is not technically an API change
but a change of how godoc interprets package documentation. Clearly we need
to address this if it causes major issues, but at the same time we'd like
to move forward.
- gri


On Wed, Feb 20, 2013 at 5:45 PM, <dave@cheney.net> wrote:

> I believe this has caused a regression, see
>
https://groups.google.com/d/**topic/golang-nuts/Jv70Gj2nSmc/**discussion<http...
>
>
https://codereview.appspot.**com/7333046/<https://codereview.appspot.com/7333...
>

[dave_cheney.net, 2013-02-21 02:47:40]

Thanks Robert.
On 21 Feb 2013 13:40, "Robert Griesemer" <gri@golang.org> wrote:

> Thanks. I answered to that thread. This is not technically an API change
> but a change of how godoc interprets package documentation. Clearly we need
> to address this if it causes major issues, but at the same time we'd like
> to move forward.
> - gri
>
>
> On Wed, Feb 20, 2013 at 5:45 PM, <dave@cheney.net> wrote:
>
>> I believe this has caused a regression, see
>>
https://groups.google.com/d/**topic/golang-nuts/Jv70Gj2nSmc/**discussion<http...
>>
>>
https://codereview.appspot.**com/7333046/<https://codereview.appspot.com/7333...
>>
>
>