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: Does not infer $GOROOT from path to binary #23445

Closed
fd0 opened this issue Jan 15, 2018 · 29 comments

Comments

Projects
None yet
@fd0
Copy link

commented Jan 15, 2018

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

This comment has been minimized.

Copy link
Author

commented Jan 15, 2018

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).

@ianlancetaylor ianlancetaylor changed the title godoc: Does not infer $GOPATH from path to binary x/tools/cmd/godoc: Does not infer $GOPATH from path to binary Jan 15, 2018

@gopherbot gopherbot added this to the Unreleased milestone Jan 15, 2018

@fd0

This comment has been minimized.

Copy link
Author

commented Jan 15, 2018

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...

@fd0 fd0 changed the title x/tools/cmd/godoc: Does not infer $GOPATH from path to binary x/tools/cmd/godoc: Does not infer $GOROOT from path to binary Jan 15, 2018

@ALTree

This comment has been minimized.

Copy link
Member

commented Jan 15, 2018

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

This comment has been minimized.

Copy link
Member

commented Jan 17, 2018

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

This comment has been minimized.

Copy link
Member

commented Feb 19, 2018

@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 comment has been minimized.

Copy link
Author

commented Feb 19, 2018

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

This comment has been minimized.

Copy link
Member

commented Feb 20, 2018

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

This comment has been minimized.

Copy link
Author

commented Feb 20, 2018

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

This comment has been minimized.

Copy link

commented Mar 21, 2018

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

This comment has been minimized.

Copy link
Member

commented Mar 26, 2018

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

This comment has been minimized.

Copy link
Author

commented Mar 26, 2018

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

This comment has been minimized.

Copy link
Author

commented Mar 26, 2018

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

This comment has been minimized.

Copy link
Member

commented Mar 26, 2018

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

This comment has been minimized.

Copy link
Contributor

commented Apr 23, 2018

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

This comment has been minimized.

Copy link
Member

commented Apr 23, 2018

Related- #13296.

@agnivade

This comment has been minimized.

Copy link
Member

commented Jun 2, 2018

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.

@agnivade agnivade added NeedsDecision and removed NeedsFix labels Jun 2, 2018

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Jun 5, 2018

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

This comment has been minimized.

Copy link
Contributor

commented Jun 6, 2018

cc: @dsnet

@rsc

This comment has been minimized.

Copy link
Contributor

commented Jun 11, 2018

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

@rsc rsc added the NeedsFix label Jun 11, 2018

@rsc rsc modified the milestones: Unreleased, Go1.12 Jun 11, 2018

@rsc rsc modified the milestones: Go1.12, Go1.11 Jun 11, 2018

@tsuna

This comment has been minimized.

Copy link
Contributor

commented Jun 11, 2018

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

This comment has been minimized.

Copy link

commented Jun 11, 2018

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

@gopherbot

This comment has been minimized.

Copy link

commented Jun 12, 2018

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

gopherbot pushed a commit that referenced this issue Jun 12, 2018

cmd/go/internal/cfg: note the copy of this code in x/tools/cmd/godoc
Updates #23445

Change-Id: I4b09073e53b1cf04de698b711fb5fb0d08bc02df
Reviewed-on: https://go-review.googlesource.com/118077
Reviewed-by: Ian Lance Taylor <iant@golang.org>
@autarch

This comment has been minimized.

Copy link

commented Sep 18, 2018

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

This comment has been minimized.

Copy link
Member

commented Sep 18, 2018

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

@autarch

This comment has been minimized.

Copy link

commented Sep 18, 2018

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

This comment has been minimized.

Copy link
Member

commented Sep 18, 2018

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

This comment has been minimized.

Copy link

commented Sep 18, 2018

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

This comment has been minimized.

Copy link

commented Sep 18, 2018

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

@trivigy

This comment has been minimized.

Copy link

commented Mar 26, 2019

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.