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

cmd/doc: 'go doc $pkg' prefers GOPATH instead of active module #28992

Closed
bcmills opened this issue Nov 28, 2018 · 12 comments
Closed

cmd/doc: 'go doc $pkg' prefers GOPATH instead of active module #28992

bcmills opened this issue Nov 28, 2018 · 12 comments
Labels
FrozenDueToAge GoCommand cmd/go modules NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@bcmills
Copy link
Member

bcmills commented Nov 28, 2018

go doc calls out to cmd/doc, and cmd/doc implicitly uses GOPATH to resolve the imports instead of looking in the active modules. That results in unexpected skew: it produces spurious failures when the package exists but is not in GOPATH, and spurious documentation if GOPATH contains unpublished changes (as in the example in the details below).

This is one instance of #24661, but the tool in question happens to be bundled with cmd/go and released with the Go toolchain: we should fix it sooner rather than later.

CC @robpike @mvdan @ianthehat @rsc

~/src/golang.org/x/build$ export GO111MODULE=on

~/src/golang.org/x/build$ go version
go version devel +d8b7a9286d Wed Nov 28 13:07:32 2018 -0500 linux/amd64

~/src/golang.org/x/build$ go list -m golang.org/x/tools
golang.org/x/tools v0.0.0-20181030000716-a0a13e073c7b

~/src/golang.org/x/build$ go list golang.org/x/tools/cmd/goforward
go: downloading golang.org/x/tools v0.0.0-20181030000716-a0a13e073c7b
go: extracting golang.org/x/tools v0.0.0-20181030000716-a0a13e073c7b
go: finding golang.org/x/tools/cmd/goforward latest
go: finding golang.org/x/tools/cmd latest
go: finding golang.org/x/tools latest
go: downloading golang.org/x/tools v0.0.0-20181127232545-e782529d0ddd
go: extracting golang.org/x/tools v0.0.0-20181127232545-e782529d0ddd
can't load package: package golang.org/x/tools/cmd/goforward: unknown import path "golang.org/x/tools/cmd/goforward": cannot find module providing package golang.org/x/tools/cmd/goforward

~/src/golang.org/x/build$ go doc golang.org/x/tools/cmd/goforward | head -n 2
The goforward command adds forwarding declarations from one Go package to
another.
@bcmills bcmills added help wanted NeedsFix The path to resolution is known, but the work has not been done. GoCommand cmd/go release-blocker modules labels Nov 28, 2018
@bcmills bcmills added this to the Go1.13 milestone Nov 28, 2018
@mvdan
Copy link
Member

mvdan commented Nov 28, 2018

I was under the impression that cmd/doc had been adapted to modules with the rest of the built-in tooling. Thanks for raising the issue.

I presume that we should keep the GOPATH logic for when the command is called outside a module, and introduce module logic otherwise.

I believe that only dirs.go would need changes, as that's the part that scans GOPATH and feeds the packages it finds to the rest of the code.

@bcmills
Copy link
Member Author

bcmills commented Nov 28, 2018

I presume that we should keep the GOPATH logic for when the command is called outside a module, and introduce module logic otherwise.

No, we should only use the GOPATH logic when not in module mode (that is, when go env GOMOD produces the empty string).

In module mode outside of a module (as of CL 148517) we should produce documentation for the latest version of the package, perhaps using go list and/or go mod download.

@mvdan
Copy link
Member

mvdan commented Nov 28, 2018

@myitcv rightfully pointed out https://go-review.googlesource.com/c/go/+/126799 - it does seem like support was added.

@bcmills
Copy link
Member Author

bcmills commented Nov 28, 2018

Ah, looks like the bug is here:

usingModules = bytes.Contains(stdout, []byte("go.mod"))

I'm setting GOMOD to os.DevNull in module mode outside a module. Might be a trivial fix — thanks for the reference!

@bcmills
Copy link
Member Author

bcmills commented Nov 28, 2018

Turns out not to be so trivial after all. CL 126799 added module-mode support for everything except packages, which apparently need more than just the source code for the package (i.e. need go/packages).

CC @matloob

@gopherbot
Copy link

Change https://golang.org/cl/151617 mentions this issue: cmd/doc: treat any non-empty GOMOD as module mode

gopherbot pushed a commit that referenced this issue Nov 29, 2018
Previously, we were looking for the string go.mod specifically, but
the module-mode-outside-a-module logic added in CL 148517 sets GOMOD
to os.DevNull

Updates #28992

Change-Id: I62a4baaa911a495350294d78bae96be3fe4866cb
Reviewed-on: https://go-review.googlesource.com/c/151617
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Russ Cox <rsc@golang.org>
@bcmills
Copy link
Member Author

bcmills commented Apr 11, 2019

CC @ianthehat @dmitshur

@dmitshur
Copy link
Contributor

dmitshur commented Apr 15, 2019

I tested with the following steps, using Go 1.12.4:

$ export GO111MODULE=on
$ export GOPATH="$(mktemp -d)"
$ cd $(mktemp -d)
$ go mod init m
$ go get rsc.io/quote/v3
$ go get dmitri.shuralyov.com/test/modtest1/inner/p@v1.0.0

$ go doc rsc.io/quote/v3
$ go doc dmitri.shuralyov.com/test/modtest1/inner/p
# worked as expected

$ mkdir -p $GOPATH/src/rsc.io/quote/v3 $GOPATH/src/dmitri.shuralyov.com/test/modtest1/inner/p
$ echo 'package p; const Foo = "bar"' > $GOPATH/src/rsc.io/quote/v3/p.go
$ echo 'package p; const Foo = "bar"' > $GOPATH/src/dmitri.shuralyov.com/test/modtest1/inner/p/p.go

$ go doc rsc.io/quote/v3
$ go doc dmitri.shuralyov.com/test/modtest1/inner/p
# incorrectly displays documentation for package from GOPATH rather than in module cache

It seems that go doc works as expected in module mode when a package is available only in the local module cache and not in a GOPATH workspace.

There is an issue of go doc, when in module mode, still preferring a package in GOPATH workspace when it's available in both.

@dmitshur dmitshur changed the title cmd/doc: 'go doc $pkg' uses GOPATH instead of active module cmd/doc: 'go doc $pkg' prefers GOPATH instead of active module Apr 15, 2019
@dmitshur dmitshur self-assigned this Apr 18, 2019
@bcmills
Copy link
Member Author

bcmills commented Jun 26, 2019

@dmitshur, is this still in progress? (If so, are you still planning to fix it for 1.13, or should we move it to the 1.14 milestone?)

@dmitshur
Copy link
Contributor

dmitshur commented Jun 26, 2019

This is in progress, I am investigating it now.

If the bug fix ends up being reasonable, it can still go into 1.13. If it turns out to be very complicated and risky, we can consider pushing it to 1.14.

@dmitshur
Copy link
Contributor

The problem was that a few build.Import calls did not provide the current working directory, which is a hard requirement for finding packages in module mode. So the fix is small. I sent CL 183991.

@gopherbot
Copy link

Change https://golang.org/cl/183991 mentions this issue: cmd/doc: provide working directory to build.Import calls

@golang golang locked and limited conversation to collaborators Jun 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge GoCommand cmd/go modules NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

4 participants