cmd/go/internal/bug: the implementation should have more comments

[perillo]

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

$ go version
go version devel go1.17-6edd573218 Tue Apr 27 11:55:52 2021 +0000 linux/amd64

Does this issue reproduce with the latest release?

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

go env Output

$ go env
GO111MODULE="on"
GOARCH="amd64"
GOBIN="/home/manlio/.local/bin"
GOCACHE="/home/manlio/.cache/go-build"
GOENV="/home/manlio/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE="*.local"
GOMODCACHE="/home/manlio/.local/lib/go/pkg/mod"
GONOPROXY=""
GONOSUMDB="*.local"
GOOS="linux"
GOPATH="/home/manlio/.local/lib/go:/home/manlio/src/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/home/manlio/src/contrib/go/go.googlesource.com/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/home/manlio/src/contrib/go/go.googlesource.com/go/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="devel go1.17-6edd573218 Tue Apr 27 11:55:52 2021 +0000"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/dev/null"
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-build955556188=/tmp/go-build -gno-record-gcc-switches"
GOROOT/bin/go version: go version devel go1.17-6edd573218 Tue Apr 27 11:55:52 2021 +0000 linux/amd64
GOROOT/bin/go tool compile -V: compile version devel go1.17-82aae74cb5 Tue Apr 27 14:48:20 2021 +0200
uname -sr: Linux 5.11.16-arch1-1
/usr/lib/libc.so.6: GNU C Library (GNU libc) release release version 2.33.
gdb --version: GNU gdb (GDB) 10.1

What did you do?

Currently, the implementation of the go bug command is a bit confusing.

1. go version is obtained using runtime.Version
2. go env is obtained invoking os.Executable env, falling back to GOROOT/bin/go env
3. go env output is OS specific (UNIX, Windows and plan9), but it is not clear if using only UNIX output is possible
4. go version is printed a second time, using the label GOROOT/bin/go version, invoking GOROOT/bin/go env.
   The code don't says why there is a second version printed; is it for compatibility reasons?
   Another issue is that the go command invoked may be different from the one invoked in 2.
   Is this the intended behavior?
5. The label GOROOT/bin/go tool compile has the same issues as 4.

[perillo]

Note that if go env output is allowed to be in the UNIX style for all platforms, it will possible to use the envcmd API instead of invoking go env, as it was done for go version.

Here is a simple patch:

diff --git a/src/cmd/go/internal/bug/bug.go b/src/cmd/go/internal/bug/bug.go
index df63c579d9..c0dff1b831 100644
--- a/src/cmd/go/internal/bug/bug.go
+++ b/src/cmd/go/internal/bug/bug.go
@@ -20,6 +20,7 @@ import (
 
 	"cmd/go/internal/base"
 	"cmd/go/internal/cfg"
+	"cmd/go/internal/envcmd"
 	"cmd/go/internal/web"
 )
 
@@ -90,17 +91,23 @@ func printEnvDetails(w io.Writer) {
 	fmt.Fprintf(w, "### What operating system and processor architecture are you using (`go env`)?\n\n")
 	fmt.Fprintf(w, "<details><summary><code>go env</code> Output</summary><br><pre>\n")
 	fmt.Fprintf(w, "$ go env\n")
-	goexe, err := os.Executable()
-	if err != nil {
-		goexe = filepath.Join(runtime.GOROOT(), "bin/go")
-	}
-	printCmdOut(w, "", goexe, "env")
+	printGoEnv(w)
 	printGoDetails(w)
 	printOSDetails(w)
 	printCDetails(w)
 	fmt.Fprintf(w, "</pre></details>\n\n")
 }
 
+func printGoEnv(w io.Writer) {
+	env := envcmd.MkEnv()
+	env = append(env, envcmd.ExtraEnvVars()...)
+	env = append(env, envcmd.ExtraEnvVarsCostly()...)
+
+	for _, e := range env {
+		fmt.Fprintf(w, "%s=\"%s\"\n", e.Name, e.Value)
+	}
+}
+
 func printGoDetails(w io.Writer) {
 	printCmdOut(w, "GOROOT/bin/go version: ", filepath.Join(runtime.GOROOT(), "bin/go"), "version")
 	printCmdOut(w, "GOROOT/bin/go tool compile -V: ", filepath.Join(runtime.GOROOT(), "bin/go"), "tool", "compile", "-V")

[ianlancetaylor]

I don't really see a bug here, in the sense of behavior that is incorrect and should change. Discussion about the code can be on golang-nuts.

Also, please don't post patches on the issue tracker. Please only send them via GitHub or Gerrit. Patches on the issue tracker don't have any copyright licensing, so we can't use them. Thanks. Please do go ahead and send in the patch via GitHub or Gerrit if you want somebody to review it.

[cherrymui]

cc @bcmills

Thanks for filing this. As Ian said, this is probably not the best place to discuss this or to send patches. Thanks.

[perillo]

Excluding the patch (that was only an example, not intended to be used/submitted), isn't code not well documented and possibly ambiguous considered an issue?

Thanks.

[bcmills]

If the code is unclear or not well documented (and I agree that this particular code is at least the latter), it's fine to send a CL to improve the documentation, with or without an issue filed.

To the specific questions:

1. go version is obtained using runtime.Version

Yes. That is a recent and intentional change.

2. go env is obtained invoking os.Executable env, falling back to GOROOT/bin/go env

I think that falling back to go env is probably a mistake, but I expect that code path is taken approximately never, so it's a very benign mistake.
Better still would be to avoid starting a go subprocess entirely.

3. go env output is OS specific (UNIX, Windows and plan9), but it is not clear if using only UNIX output is possible

The go env output from go bug should exactly match what the user would see if they ran go env with the same go executable. So it isn't feasible to emit only UNIX output.

4. go version is printed a second time, using the label GOROOT/bin/go version, invoking GOROOT/bin/go env.
   The code don't says why there is a second version printed; is it for compatibility reasons?

A bit of code archaeology reveals that those lines were added in CL 32643, for #15877. They are there to help diagnose mismatches between the user's GOROOT and PATH, which was a more common error in the past than it is today (because in most cases users no longer need to set GOROOT explicitly), but is still surprisingly common (I think due to users working from outdated tutorials found around the web).

Another issue is that the go command invoked may be different from the one invoked in 2.
Is this the intended behavior?

Yes.

5. The label GOROOT/bin/go tool compile has the same issues as 4.

From the discussion on #15877, it appears that is intended to diagnose the case when cmd/compile is built at a version that doesn't match cmd/go, but I haven't the faintest idea why we would expect that to come up often enough to be worth including in go bug. The discussion on #15877 mentions using it to diagnose #15372, but #15372 was fixed a long time ago.

[perillo]

About 3., formatting an environment variable takes a lot of code:

go/src/cmd/go/internal/envcmd/env.go

Line 350 in 4c1a7ab

for _, e := range env {

So, in order to avoid starting a go subprocess, the envcmd package should export a FormatEnv function, so I'm not sure it is worth the effort.

Thanks.

[gopherbot]

Change https://golang.org/cl/314230 mentions this issue: cmd/go/internal/bug: use envcmd instead of go env