Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

x/tools/cmd/godoc: add module support #33655

Closed
dmitshur opened this issue Aug 14, 2019 · 16 comments
Assignees
Milestone

Comments

@dmitshur
Copy link
Member

@dmitshur dmitshur commented Aug 14, 2019

This is the tracking issue specifically for the golang.org/x/tools/cmd/godoc command to have support for displaying package documentation in module mode. The parent umbrella issue for all “godoc”-like projects is #26827.

At this time, the godoc command is only able to display packages that are located in a GOPATH workspace. When invoked in module mode, it should be able to show documentation for packages in the build list of the main module (as can be determined by running go list -m all), even if those packages are not inside a GOPATH workspace.

The plan is to make this change while maintaining GOPATH support. godoc will use the same logic as the go command to determine whether to use module mode or GOPATH mode (i.e., the value of GO111MODULE environment variable and the presence of a go.mod file).

Scope

There may be future enhancements to add features that weren’t previously viable in GOPATH mode, like being able to select the version via a dropdown UI element, but that’s not in scope for this issue.

This issue is only about maintaining the ability to view the documentation for packages when in module mode.

The -analysis flag is not in scope of this issue, since it's not required to view documentation. It will take more work and is being tracked separately in issue #34473.

@dmitshur

This comment has been minimized.

Copy link
Member Author

@dmitshur dmitshur commented Sep 23, 2019

Possible Solutions

I have spent some time investigating ways this issue can be resolved. There are many potential long-term solutions that involve making large changes, such as moving towards a newer documentation HTML renderer, making API changes to the godoc library and having it use go/packages to load packages, etc. There is also another solution that requires making a very small, targeted change that reuses much of existing godoc code and functionality. (Another upside is that it allows maintaining -analysis flag support with a small amount of additional effort, tracking issue #34473.) That is the solution I plan to use to resolve this issue.

Godoc functionality is currently spread between the golang.org/x/tools/cmd/godoc command, and the golang.org/x/tools/godoc library. The godoc library API operates on top of a virtual filesystem abstraction, which offers a healthy amount of flexibility. In fact, it's possible to add module support to the godoc command without making any changes to the godoc library API.

Implementation Plan

The goal is to make the godoc command run in module mode whenever the go command would run in module mode. To implement this reliably, godoc will run go env GOMOD with the same environment and working directory, and interpret the result. The -json flag will be used.

If the go env GOMOD result is the empty string, GOPATH mode is being used, and godoc will behave as it currently does: it will find each GOPATH workspace and bind it to the virtual filesystem:

// Bind $GOPATH trees into Go root.
for _, p := range filepath.SplitList(build.Default.GOPATH) {
	fs.Bind("/src", gatefs.New(vfs.OS(p), fsGate), "/src", vfs.BindAfter)
}

If the go env GOMOD result is a non-empty string, module mode is being used. godoc will use go list -m all to determine all modules in the build list, and bind their roots in appropriate locations within the filesystem:

// Determine modules in the build list.
type mod struct {
	Path    string // Module path.
	Version string // Module version.
	Dir     string // Directory holding files for this module, if any.
}
var mods []mod
[... populate mods with output from 'go list -m -json all']

// Bind module trees into Go root.
for _, m := range mods {
	if m.Dir == "" {
		[... either use 'go mod download' to fill module cache, or skip]
	}
	fs.Bind(path.Join("/src", m.Path), gatefs.New(vfs.OS(m.Dir), fsGate), "/", vfs.BindAfter))
}

This way, we outsource the work of doing version selection to the go command, which guarantees consistent results. It will result in a virtual filesystem that accurately contains the source code of Go packages that would be used in the build. It respects local replace directives in the go.mod file, if any. (It will not pick up changes to go.mod file that are made after godoc is started, that will take more changes.)

There may be small changes to the virtual filesystem implementation so that modules are not considered to be inside GOROOT. Some additional care may need to be taken to ensure overlapping paths, in case of nested modules, are supported.

There are tests in the golang.org/x/tools/cmd/godoc package that verify functionality. They will be converted to use packagestest and made to run in both GOPATH mode and module mode.

Decision on automatic downloading of modules not in module cache

Documentation for go list -m notes:

When listing modules, the -f flag still specifies a format template applied to a Go struct, but now a Module struct:

type Module struct {
    [...]
    Dir string // directory holding files for this module, if any
    [...]
}

An important part here is "if any". Most of the time, modules in the build list of the main module are available in the module cache, and the Dir field points to a location on disk where that module is available.

In some cases, for example when a new module has been downloaded but not yet built, some dependencies are not yet available in the module cache. The Dir field will be the empty string, and godoc will not be able to display documentation for that module.

A potential solution here is to simulate what the go command currently does in similar scenarios: automatically start to download the missing modules into the module cache. This can be implemented by invoking go mod download on modules in the build list that are not already present in the module cache.

The alternative solution is to rely on the user to manually run go mod tidy, go test ./..., go build ./..., or any other go command that would populate the missing module cache entries. I am still evaluating which solution to go with here, and currently leaning slightly towards the behavior that is consistent with the go command. Feedback on this decision is welcome.

/cc @andybons

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Sep 23, 2019

Change https://golang.org/cl/196978 mentions this issue: godoc: remove Corpus.testDir field

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Sep 23, 2019

Change https://golang.org/cl/196979 mentions this issue: cmd/godoc: check if server exited when waiting

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Sep 23, 2019

Change https://golang.org/cl/196981 mentions this issue: cmd/godoc: convert tests to packagestest, cover third party packages

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Sep 23, 2019

Change https://golang.org/cl/196977 mentions this issue: godoc, godoc/vfs: improve documentation of getPageInfo, hasPathPrefix

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Sep 23, 2019

Change https://golang.org/cl/196980 mentions this issue: go/packages/packagestest: add package use example

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Sep 23, 2019

Change https://golang.org/cl/196983 mentions this issue: cmd/godoc: add initial support for module mode

gopherbot pushed a commit to golang/tools that referenced this issue Sep 24, 2019
It was indeed unused.

Updates golang/go#33655

Change-Id: Icb9b9a3d201cc573ae294063b64e38890f37b9ab
Reviewed-on: https://go-review.googlesource.com/c/tools/+/196978
Reviewed-by: Agniva De Sarker <agniva.quicksilver@gmail.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@agnivade

This comment has been minimized.

Copy link
Contributor

@agnivade agnivade commented Sep 24, 2019

The alternative solution is to rely on the user to manually run go mod tidy, go test ./..., go build ./..., or any other go command that would populate the missing module cache entries.

I am in favor of this solution. I think godoc should be just a plain documentation viewer. I wouldn't want it to automatically populate the module cache by itself. If some dependencies are missing from the cache let godoc mark them as empty (or something similar). I can run a command myself to populate it and then run godoc. Perhaps it can recommend a command for me to run in case it finds empty Dir entries, but I personally, wouldn't want it doing stuff by itself.

@myitcv

This comment has been minimized.

Copy link
Member

@myitcv myitcv commented Sep 24, 2019

FWIW I think we should emulate what go doc does:

export GOPATH=$(mktemp -d)
cd $(mktemp -d)
go mod init example.com/hello
go mod edit -require=golang.org/x/tools@latest
go doc golang.org/x/tools/go/packages

Gives:

go: finding golang.org/x/tools latest
go: finding golang.org/x/crypto v0.0.0-20190308221718-c2843e01d9a2
go: finding golang.org/x/net v0.0.0-20190620200207-3b0461eec859
go: finding golang.org/x/sync v0.0.0-20190423024810-112230192c58
go: finding golang.org/x/sys v0.0.0-20190215142949-d0b11bdaac8a
go: finding golang.org/x/text v0.3.0
go: finding golang.org/x/xerrors v0.0.0-20190717185122-a985d3407aa7
package packages // import "golang.org/x/tools/go/packages"

Package packages loads Go packages for inspection and analysis.

...

That's to say, go doc fetches modules as required.

Because if you don't want the underlying go command to do any fetching you can simply set GOPROXY=off.

clintjedwards added a commit to clintjedwards/tools that referenced this issue Sep 25, 2019
It was indeed unused.

Updates golang/go#33655

Change-Id: Icb9b9a3d201cc573ae294063b64e38890f37b9ab
Reviewed-on: https://go-review.googlesource.com/c/tools/+/196978
Reviewed-by: Agniva De Sarker <agniva.quicksilver@gmail.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
gopherbot pushed a commit to golang/tools that referenced this issue Sep 29, 2019
Previously, the waitForServer family of helpers would wait anywhere
between 15 seconds to 2 minutes for the server to become ready.
But if there's a problem that results in the server exiting early,
that wasn't being detected quickly.

This change modifies tests to also wait for command to exit,
and fail the test quickly if so. This helps during development.

Updates golang/go#33655

Change-Id: I16195715449015d7250a2d0de5e55ab9a1ef078d
Reviewed-on: https://go-review.googlesource.com/c/tools/+/196979
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Andrew Bonventre <andybons@golang.org>
gopherbot pushed a commit to golang/tools that referenced this issue Sep 29, 2019
This change converts cmd/godoc tests to use the packagestest package
in its basic integration tests. For now, those tests continue to run
in GOPATH mode only. When module support is added to cmd/godoc, then
the same tests will be made to run in module mode too.

Previously, the basic integration test covered godoc functionality
on Go packages in GOROOT only. This change also adds some third
party packages to increase test coverage. This is easy to do with
the packagestest API.

Updates golang/go#33655

Change-Id: If3fce913140b81ed9340556d6bb4b963f5f98813
Reviewed-on: https://go-review.googlesource.com/c/tools/+/196981
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Andrew Bonventre <andybons@golang.org>
gopherbot pushed a commit to golang/tools that referenced this issue Sep 29, 2019
Change GetPageInfo method documentation to match the method name.

Prefer using "reports whether" in a function that returns a boolean.
This style is more idiomatic.

Updates golang/go#33655

Change-Id: I1a781e7b4f5b4b629fdf4f48e2e97183f63508f9
Reviewed-on: https://go-review.googlesource.com/c/tools/+/196977
Reviewed-by: Agniva De Sarker <agniva.quicksilver@gmail.com>
@rsc rsc modified the milestones: Go1.14, Backlog Oct 9, 2019
@dmitshur dmitshur modified the milestones: Backlog, Go1.14 Oct 10, 2019
@dmitshur

This comment has been minimized.

Copy link
Member Author

@dmitshur dmitshur commented Nov 6, 2019

I agree with the argument Paul made, and plan to include the behavior that will automatically fill the module cache when some of the dependencies of the main module are missing. This behavior can be disabled by setting GOPROXY=off, which is consistent with other tools and doesn't add anything new.

I've considered not doing automatic downloads, but this leads to an inconsistent user experience. Part of the problem is that modifying code in your project can result in previously existing dependencies disappearing. For example, if some dependency is removed, it can result in a different version of other dependencies to be selected via MVS algorithm. And those versions may not be in the module cache. Unlike the GOPATH mode, if the exact version of the required module isn't the in module cache, we can't simply display some other other existing version because that would be misleading.

Let's start with having this behavior on since it's consistent with other similar tools. I've included it in Patch Set 3 of CL 196983. We can adjust it in the future as we learn more.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Nov 6, 2019

Change https://golang.org/cl/205661 mentions this issue: godoc/vfs: include dirs needed to reach mount points in Stat

@dmitshur

This comment has been minimized.

Copy link
Member Author

@dmitshur dmitshur commented Nov 7, 2019

CL 196983, which will resolve this issue, is now ready and I plan to submit it later today. It implements the behavior described above.

@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Nov 7, 2019

This is a nice first cut, but it doesn't play nicely with automatic vendoring in 1.14 (#33848):

~/go/src/cmd$ go version
go version devel +abec5d0879 Wed Nov 6 16:49:40 2019 -0500 linux/amd64

~/go/src/cmd$ go version $(which godoc)
/usr/local/google/home/bcmills/bin/godoc: devel +abec5d0879 Wed Nov 6 16:49:40 2019 -0500

~/go/src/cmd$ godoc
using module mode; GOMOD=/usr/local/google/home/bcmills/go/src/cmd/go.mod
failed to determine the build list of the main module: go command exited unsuccessfully: exit status 1
go list -m: can't compute 'all' using the vendor directory
        (Use -mod=mod or -mod=readonly to bypass.)

I left a few comments on CL 196983 suggesting the points in the implementation that might need to be adapted for vendoring.

@dmitshur

This comment has been minimized.

Copy link
Member Author

@dmitshur dmitshur commented Nov 7, 2019

Thanks for pointing that out and leaving the comments on the CL Bryan.

I'd like to track that in a separate issue, since it's specific to Go 1.14, which isn't out yet. I want this issue to be available for discussion of module support in the currently released and supported versions of Go. Opened #35429 for it.

@mcandre

This comment was marked as off-topic.

Copy link

@mcandre mcandre commented Nov 11, 2019

Ten years, Go. Ten years without a comprehensive plan for dependency management.

@mvdan

This comment has been minimized.

Copy link
Member

@mvdan mvdan commented Nov 11, 2019

@mcandre please be nice. Also, I don't see how your comment is relevant to this closed issue.

Bogdan-D pushed a commit to Bogdan-D/godocx that referenced this issue Nov 13, 2019
It was indeed unused.

Updates golang/go#33655

Change-Id: Icb9b9a3d201cc573ae294063b64e38890f37b9ab
Reviewed-on: https://go-review.googlesource.com/c/tools/+/196978
Reviewed-by: Agniva De Sarker <agniva.quicksilver@gmail.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Bogdan-D pushed a commit to Bogdan-D/godocx that referenced this issue Nov 13, 2019
Change GetPageInfo method documentation to match the method name.

Prefer using "reports whether" in a function that returns a boolean.
This style is more idiomatic.

Updates golang/go#33655

Change-Id: I1a781e7b4f5b4b629fdf4f48e2e97183f63508f9
Reviewed-on: https://go-review.googlesource.com/c/tools/+/196977
Reviewed-by: Agniva De Sarker <agniva.quicksilver@gmail.com>
gopherbot pushed a commit to golang/tools that referenced this issue Nov 13, 2019
The purpose and value of packagestest is easy to understand. However,
the current API may not be easy to get started with. It includes
identifiers such as Exporter, Exported, Export, whose names may not
make it very clear in what order they are to be used, and whether
the Exporter interfaces needs to be implemented by the caller.

There are fairly common patterns of usage spread out across various
packages that use packagestest. Add an example of basic usage to the
documentation of this package that connects all of the pieces together,
so that users don't have to look for it elsewhere.

I would've preferred to add the example as example code¹, but it
doesn't seem viable to write example code for test helper packages.
There isn't a way to get a functional *testing.T in a func Example.

¹ https://golang.org/pkg/testing/#hdr-Examples

Updates golang/go#33655

Change-Id: I0b15ff7974be25a71dfd4b68f470441ce7331d18
Reviewed-on: https://go-review.googlesource.com/c/tools/+/196980
Reviewed-by: Michael Matloob <matloob@golang.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.