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.

Sign up for GitHub

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

Jump to bottom

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

Closed
perillo opened this issue Apr 27, 2021 · 7 comments
Closed

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

perillo opened this issue Apr 27, 2021 · 7 comments
Labels
FrozenDueToAge

Comments

@perillo
Copy link
Contributor

perillo commented Apr 27, 2021

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

This comment has been minimized.

Sign in to view
@ianlancetaylor
Copy link
Contributor

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
Copy link
Member

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
Copy link
Contributor Author

perillo commented Apr 27, 2021

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
Copy link
Contributor

bcmills commented Apr 27, 2021

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.

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

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

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

  1. 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
Copy link
Contributor Author

perillo commented Apr 27, 2021

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
Copy link

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

gopherbot pushed a commit that referenced this issue Apr 28, 2021
@perillo
cmd/go/internal/bug: use envcmd instead of go env
12af403 
Add the printGoEnv function to print the go environment variables, using
the envcmd package instead of invoking go env.

Add the PrintEnv function to the envcmd package, to avoid duplicating
code.

Updates #45803

Change-Id: I38d5b936c0ebb16e741ffbee4309b95d6d0ecc6c
Reviewed-on: https://go-review.googlesource.com/c/go/+/314230
Reviewed-by: Bryan C. Mills <bcmills@google.com>
Reviewed-by: Michael Matloob <matloob@golang.org>
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
@golang golang locked and limited conversation to collaborators Apr 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@ianlancetaylor @bcmills @perillo @gopherbot @cherrymui