Go: x/tools/cmd/godoc: Does not infer $GOROOT from path to binary

[fd0]

Please answer these questions before submitting your issue. Thanks!

What version of Go are you using (go version)?

go version go1.10beta2 linux/amd64

Does this issue reproduce with the latest release?

Yes, but Go 1.10 is supposed to change that, https://tip.golang.org/doc/go1.10#goroot

(Awesome change by the way!)

What operating system and processor architecture are you using (go env)?

GOARCH="amd64"
GOBIN=""
GOCACHE="/home/fd0/.cache/go-build"
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/fd0/shared/work/go"
GORACE=""
GOROOT="/home/fd0/Downloads/go1.10beta2"
GOTMPDIR=""
GOTOOLDIR="/home/fd0/Downloads/go1.10beta2/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build797922256=/tmp/go-build -gno-record-gcc-switches"

What did you do?

Without $GOROOT explicitely set, Go 1.10 infers the Go root directory from the location of the go binary as it called:

$ unset GOROOT

$ pwd
/home/fd0/Downloads/go1.10beta2

$ bin/go version
go version go1.10beta2 linux/amd64

$ bin/go env GOROOT
/home/fd0/Downloads/go1.10beta2

But this does not work for godoc:

$ bin/godoc os Exit
2018/01/15 14:17:19 cannot find package "." in:
    /src/os

$ bin/godoc -http :6060
2018/01/15 14:17:34 newDirectory(/): stat /usr/local/go: no such file or directory
2018/01/15 14:17:34 godoc: corpus fstree is nil

Explicitely setting GOROOT (like in Go < 1.10) works:

$ GOROOT=$PWD bin/godoc os Exit
use 'godoc cmd/os' for documentation on the os command 

func Exit(code int)
    Exit causes the current program to exit with the given status code.
    Conventionally, code zero indicates success, non-zero an error. The
    program terminates immediately; deferred functions are not run.

What did you expect to see?

godoc finds the Go root directory ($GOROOT) by itself, like the go binary does. This is advertised as one of the (awesome) changes of Go 1.10, apparently it does not work for godoc yet, which is unexpected.

What did you see instead?

The Go root directory is not detected automatically, I needed to set it manually via $GOROOT.

[fd0]

I spent half an hour trying to find out where godoc gets its GOROOT when none is specified (via command line -goroot or environment), but did not succeed. If someone can point me into the right direction, I'll try to prepare a PR (or whatever that's called for Gerrit).

[fd0]

Is this something that's relevant to get into 1.10? Or just "sometime"? I'm asking because I can spend some time on it this week, if it's relevant enough...

[ALTree]

go1.10 is pretty much done at this point.

Also note that the release notes say

the go tool attempts to deduce GOROOT

"the go tool" == the go binary. the godoc binary is distributed in the archives but it's another thing. So I don't think this is actually a "bug". The release notes are correct. It's just that the godoc binary does not do this specific thing that was implemented for the go binary.

[agnivade]

It seems like you have a fresh machine without a previous go installation. Because I have tried to repro your issue and it seems to work fine for me. Only when I renamed the GOROOT directory (/usr/local/go) to /usr/local/something, I can see your issue.

$unset GOROOT
$~/play/go/bin/godoc os Exit
use 'godoc cmd/os' for documentation on the os command 

func Exit(code int)
    Exit causes the current program to exit with the given status code.
    Conventionally, code zero indicates success, non-zero an error. The
    program terminates immediately; deferred functions are not run.

But, if I do -

$unset GOROOT
$sudo mv /usr/local/go /usr/local/something
$~/play/go/bin/godoc os Exit
2018/01/17 11:23:04 main.go:345: cannot find package "." in:
    /src/os

[ysmolsky]

@fd0
Did you do go get -u golang.org/x/tools/cmd/godoc with your version of compiler?
I fail to reproduce this with go 1.10. Can you try the final release?

[fd0]

This issue is about the godoc binary that comes with the 1.10 release, it does not work out of the box:

$ unset GOROOT

$ sdk/go1.10/bin/go version
go version go1.10 linux/amd64

$ sdk/go1.10/bin/go env GOROOT
/home/fd0/sdk/go1.10

$ sdk/go1.10/bin/godoc strings
2018/02/19 19:41:53 cannot find package "." in:
        /src/strings

It only works when users manually set GOROOT:

$ GOROOT=sdk/go1.10 sdk/go1.10/bin/godoc strings
PACKAGE DOCUMENTATION
package strings
    import "strings"
[...]

I can build a new godoc binary with go get, and that indeed works, but this issue is about the godoc binary included with the binary releases.

I think it'd be great to make godoc work out of the box, without having to set GOROOT :)

[ysmolsky]

Thanks. It seems to me that godoc is not bugged. It has built in $GOROOT path into it. Also it might be problematic to solve this bug because godoc can be installed into $GOPATH/bin and it would be not able to infer the correct path to $GOROOT.

My advise is to build godoc via get update -u if you cannot specify GOROOT for custom installed go package.

[fd0]

It seems to me that godoc is not bugged.

I think this is a bug. The godoc binary from a released version of Go behaves differently than the go binary, it requires users to manually set GOROOT before it can be used, that should be corrected. So let's agree to disagree :)

[davidpope]

I concur with @fd0 that this is undesirable behavior; whether it's a bug or feature request isn't a key distinction. Either way it certainly violates the principle of least surprise.

I never install Go in /usr/local since 1) it's just my developer tool, not something machine-wide, so why bother with 'sudo' and root access? and 2) I need to have multiple side-by-side Go versions available. I typically only add the latest one to my PATH, and I expected that this 1.10 godoc would have the same new feature that go 1.10 does for detecting GOROOT.

[ysmolsky]

Obvious solution would be to copy findGOROOT from go sources to godoc. But I am not sure if this is the best one. What do you think?

modified   cmd/godoc/main.go
@@ -82,7 +82,7 @@ var (

    // file system roots
    // TODO(gri) consider the invariant that goroot always end in '/'
-   goroot = flag.String("goroot", runtime.GOROOT(), "Go root directory")
+   goroot = flag.String("goroot", findGOROOT(), "Go root directory")

    // layout control
    tabWidth       = flag.Int("tabwidth", 4, "tab width")
@@ -161,6 +161,60 @@ func initCorpus(corpus *godoc.Corpus) {
    }
 }

+func findGOROOT() string {
+   if env := os.Getenv("GOROOT"); env != "" {
+       return filepath.Clean(env)
+   }
+   def := filepath.Clean(runtime.GOROOT())
+   exe, err := os.Executable()
+   if err == nil {
+       exe, err = filepath.Abs(exe)
+       if err == nil {
+           if dir := filepath.Join(exe, "../.."); isGOROOT(dir) {
+               // If def (runtime.GOROOT()) and dir are the same
+               // directory, prefer the spelling used in def.
+               if isSameDir(def, dir) {
+                   return def
+               }
+               return dir
+           }
+           exe, err = filepath.EvalSymlinks(exe)
+           if err == nil {
+               if dir := filepath.Join(exe, "../.."); isGOROOT(dir) {
+                   if isSameDir(def, dir) {
+                       return def
+                   }
+                   return dir
+               }
+           }
+       }
+   }
+   return def
+}
+
+// isSameDir reports whether dir1 and dir2 are the same directory.
+func isSameDir(dir1, dir2 string) bool {
+   if dir1 == dir2 {
+       return true
+   }
+   info1, err1 := os.Stat(dir1)
+   info2, err2 := os.Stat(dir2)
+   return err1 == nil && err2 == nil && os.SameFile(info1, info2)
+}
+
+// isGOROOT reports whether path looks like a GOROOT.
+//
+// It does this by looking for the path/pkg/tool directory,
+// which is necessary for useful operation of the cmd/go tool,
+// and is not typically present in a GOPATH.
+func isGOROOT(path string) bool {
+   stat, err := os.Stat(filepath.Join(path, "pkg", "tool"))
+   if err != nil {
+       return false
+   }
+   return stat.IsDir()
+}
+

[fd0]

I think this is a good solution. We need to make sure that should this behavior change in the future, both places are updated. Or is it maybe desirable to export the findGOROOT() function, maybe in an internal package?

[fd0]

Hm. GOROOT() is a simple function in src/runtime/extern.go:

// GOROOT returns the root of the Go tree. It uses the
// GOROOT environment variable, if set at process start,
// or else the root used during the Go build.
func GOROOT() string {
    s := gogetenv("GOROOT")
    if s != "" {
        return s
    }
    return sys.DefaultGoroot
}

Maybe this function should auto-detect the GOROOT? Otherwise all programs accessing runtime.GOROOT() won't behave correctly.

It feels to me I haven't understood the problem completely. Did I overlook something?

[ysmolsky]

findGOROOT works under the assumption that it is called from the process stored in $GOROOT/bin. User programs would not benefit from changing runtime.GOROOT().

Only the godoc needs access tofindGOROOT since it's distributed in the same dir as the go command.

Is there any other way to share findGOROOT with godoc without copying the code?

ping @rsc

[tsuna]

Regardless of the assumption used by findGOROOT, it's somewhat problematic that runtime.GOROOT() may not in fact return the correct GOROOT, especially as people start to depend on the fact that Go now "figures it out automatically".

[agnivade]

Related- https://github.com/golang/go/issues/13296.

[agnivade]

Indeed this is a bit problematic. Ideally, we should not keep duplicate copies of findGOROOT, but it's not exactly clear how should this proceed.

Should we -

• expose a new function inside runtime (or somewhere else) ?
• modify runtime.GOROOT functionality itself ? That seems risky.
• copy over logic inside godoc binary ?

@tsuna - runtime.GOROOT works correctly as documented. - It uses the GOROOT environment variable, if set at process start, or else the root used during the Go build. So it is not guaranteed to return the correct GOROOT if you have put the Go distribution somewhere else.

I am going to bring this back to NeedsDecision as I don't see a clear way for a fix.

@ianlancetaylor , @andybons , @rsc - Feel free to comment.

[ianlancetaylor]

Sorry, I haven't tried to keep track of the various varieties of godoc (I think we're up to four now). CC @griesemer in case he has something to add.

[griesemer]

cc: @dsnet

[rsc]

Copying the GOROOT-finding code from cmd/go is OK to do in godoc.

[tsuna]

I still think it's somewhat problematic that runtime.GOROOT() doesn't return the correct GOROOT (i.e. not what go env would print). Ultimately this problem affects all the callers of runtime.GOROOT(), not just godoc.

[gopherbot]

Change https://golang.org/cl/118075 mentions this issue: cmd/godoc: use same GOROOT discovery as Go 1.10's cmd/go

[gopherbot]

Change https://golang.org/cl/118077 mentions this issue: cmd/go/internal/cfg: note the copy of this code in x/tools/cmd/godoc

[autarch]

It's not clear to me why this was closed. I just installed the latest version of godoc by running go get -u golang.org/x/tools/cmd/godoc and I'm still seeing the same error:

$> godoc -http ':8080'
2018/09/18 11:50:51 godoc: corpus fstree is nil

$> printenv GOROOT
/usr/lib/go-1.10

$> ls -l $GOROOT
total 12
drwxr-xr-x 2 root root 4096 Mar 21 17:43 bin
lrwxrwxrwx 1 root root   36 Mar  7  2018 doc -> ../../share/doc/golang-1.10-doc/html
lrwxrwxrwx 1 root root   46 Mar  7  2018 favicon.ico.gz -> ../../share/doc/golang-1.10-doc/favicon.ico.gz
drwxr-xr-x 8 root root 4096 Mar  7  2018 pkg
lrwxrwxrwx 1 root root   23 Mar  7  2018 src -> ../../share/go-1.10/src
lrwxrwxrwx 1 root root   24 Mar  7  2018 test -> ../../share/go-1.10/test
-rw-r--r-- 1 root root    6 Feb 16  2018 VERSION

I'm on an Ubuntu 16.04 system and I installed Go using the xenial-backports package 1.10-1ubuntu1~16.04.1.

[agnivade]

Umm .. you are using Go 1.10. This is fixed in 1.11.

[autarch]

I installed the latest godoc from the repo with go get -u. Is the fix in godoc or in Go 1.11? I assumed that it was the former so that updating just the godoc binary would be sufficient.

[agnivade]

Oh I see. The godoc needs to come from the installed go distribution. If you update godoc manually, it will get installed in $GOBIN. Please check the output of which godoc and which go. If they are same, and you still see the issue, please open a new issue. If they are different, then there is no issue.

[autarch]

The only version of godoc I have is in $GOPATH/bin. There is no godoc at /usr/lib/go-1.10/bin/, which is where the go binary lives.

[autarch]

Also, I checked the file size before & after running go get -u and it was bigger afterwards, so _something_ changed.

[trivigy]

If you stumble on this issue, make sure that your golang installation comes from the original instructions rather than some third party unofficial launchpad. To point to @agnivade remark which lead me to understand how to fix this issue.

[tv42]

Go SDKs installed with go get golang.org/dl/go1.13.1 and friends do not contain godoc.

[bradfitz]

@tv42, neither does Go 1.13: https://golang.org/doc/go1.13#godoc

[tv42]

@bradfitz So it really sounds like this should not have been closed, then?

[bradfitz]

Why do you say that?

[tv42]

1. If I use the prebuilt SDKs, my only means of obtaining godoc is through go get golang.org/x/tools/cmd/godoc

2. go get golang.org/x/tools/cmd/godoc results in a godoc that fails to work unless GOROOT is set in environment (edit: or the sdk is in old school /usr/local value, which go get golang.org/dl/go1.13.1 does not do).

3. We've been telling people for the longest time that setting GOROOT explicitly is no longer necessary, so on your average gopher's setup, it's not set.

As far as I can see, godoc is broken in exactly the ways the previous commenters have described.

[agnivade]

Indeed, once godoc stopped being part of the distribution, it can't detect GOROOT unless the distribution is in the usual place, or the godoc binary is placed inside the $GOROOT/bin directory. One option is to shell out to go env GOROOT. But note that to point to custom distributions like go1.xx.x, you _have_ to set GOROOT, there is no other way.

@bradfitz - This will diverge from the findGOROOT implementation in cmd/go. But do you see a better way ?

[bradfitz]

Reopening.

Diverging from cmd/go's logic is fine.

Anybody want to take this?

[agnivade]

Anybody want to take this?

Sure.

[gopherbot]

Change https://golang.org/cl/199279 mentions this issue: cmd/godoc: update findGOROOT