-
Notifications
You must be signed in to change notification settings - Fork 17.8k
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
Comments
Possible SolutionsI 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 functionality is currently spread between the Implementation PlanThe goal is to make the If the // 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 // 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 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 Decision on automatic downloading of modules not in module cacheDocumentation for
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 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 A potential solution here is to simulate what the The alternative solution is to rely on the user to manually run /cc @andybons |
Change https://golang.org/cl/196978 mentions this issue: |
Change https://golang.org/cl/196979 mentions this issue: |
Change https://golang.org/cl/196981 mentions this issue: |
Change https://golang.org/cl/196977 mentions this issue: |
Change https://golang.org/cl/196980 mentions this issue: |
Change https://golang.org/cl/196983 mentions this issue: |
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>
I am in favor of this solution. I think |
FWIW I think we should emulate what
Gives:
That's to say, Because if you don't want the underlying |
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>
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>
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>
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>
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 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. |
Change https://golang.org/cl/205661 mentions this issue: |
CL 196983, which will resolve this issue, is now ready and I plan to submit it later today. It implements the behavior described above. |
This is a nice first cut, but it doesn't play nicely with automatic vendoring in 1.14 (#33848):
I left a few comments on CL 196983 suggesting the points in the implementation that might need to be adapted for vendoring. |
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. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
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>
@dmitshur according to your "Possible solution"
Does it mean that the module name must represent a real path on the locale filesystem? I faced #26827 (comment) where my module name is my valid VCS name but it can't be mapped according the existing solution. |
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 runninggo 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 thego
command to determine whether to use module mode or GOPATH mode (i.e., the value ofGO111MODULE
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.The text was updated successfully, but these errors were encountered: