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/go: go test -buildvcs=true does not include vcs information #52648

Closed
earthboundkid opened this issue May 2, 2022 · 12 comments
Closed

cmd/go: go test -buildvcs=true does not include vcs information #52648

earthboundkid opened this issue May 2, 2022 · 12 comments
Assignees
Labels
FrozenDueToAge GoCommand cmd/go NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@earthboundkid
Copy link
Contributor

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

go version go1.18.1 darwin/amd64

Does this issue reproduce with the latest release?

Yes

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

go env Output
GO111MODULE="on"
GOARCH="amd64"
GOBIN="/Users/adhoc/bin"
GOCACHE="/Users/adhoc/Library/Caches/go-build"
GOENV="/Users/adhoc/Library/Application Support/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users/adhoc/src/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/adhoc/src/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/Cellar/go/1.18.1/libexec"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/Cellar/go/1.18.1/libexec/pkg/tool/darwin_amd64"
GOVCS=""
GOVERSION="go1.18.1"
GCCGO="gccgo"
GOAMD64="v1"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/dev/null"
GOWORK=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -arch x86_64 -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/p7/jc4qc9n94r3f6ylg0ssh1rq00000gs/T/go-build2620312267=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

Run go test -buildvcs=true .

What did you expect to see?

Have debug.ReadBuildInfo() include vcs information.

What did you see instead?

Information was not included.

Compare #52338, cc @bcmills

earthboundkid added a commit to earthboundkid/versioninfo that referenced this issue May 2, 2022
earthboundkid added a commit to earthboundkid/versioninfo that referenced this issue May 2, 2022
earthboundkid added a commit to earthboundkid/versioninfo that referenced this issue May 2, 2022
@seankhliao
Copy link
Member

CL 393894

@earthboundkid
Copy link
Contributor Author

I agree that the VCS information should not be included by default. But if you specifically add -buildvcs=true, it should either include the information or error out. In my case, I'm testing a package that parses debug.ReadBuildInfo, which is kind of a weird thing to do, but I don't think it's so weird that the VCS information should never be includable.

@bcmills
Copy link
Contributor

bcmills commented May 2, 2022

I suppose that now that we have -buildvcs=auto, we could theoretically make -buildvcs=true add stamping for tests. 🤔

@bcmills bcmills added Thinking NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. GoCommand cmd/go labels May 2, 2022
@bcmills bcmills added this to the Go1.19 milestone May 2, 2022
@seankhliao seankhliao changed the title test: go test -buildvcs=true does not include vcs information cmd/go: go test -buildvcs=true does not include vcs information May 2, 2022
earthboundkid added a commit to earthboundkid/versioninfo that referenced this issue May 2, 2022
@alexedwards
Copy link

I'd like to see consistency in behavior across the different go tools. If go test -buildvcs=true adds stamping for tests, then go run -buildvcs=true should add stamping too.

I wouldn't want to see us end up in a scenario like this:

@earthboundkid
Copy link
Contributor Author

I think it’s fine for go test to default to omitting and go run to disallow it, but it should error out if you request an impossible option.

@bcmills bcmills added the NeedsFix The path to resolution is known, but the work has not been done. label May 27, 2022
@bcmills bcmills self-assigned this May 27, 2022
@bcmills bcmills removed the Thinking label May 27, 2022
@gopherbot gopherbot removed the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label May 27, 2022
@gopherbot
Copy link

Change https://go.dev/cl/409177 mentions this issue: cmd/go: stamp VCS information in test binaries when -buildvcs=true

@bcmills
Copy link
Contributor

bcmills commented Jun 1, 2022

@gopherbot, please backport to Go 1.18. The requested -buildvcs behavior for this particular use-case was present in Go 1.18, but regressed in Go 1.18.1 as a byproduct of the fix for #51767. Now that -buildvcs=auto has been added for #51798, the Go 1.18 behavior for -buildvcs=true can be restored.

@gopherbot
Copy link

Backport issue(s) opened: #53177 (for 1.18).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://go.dev/wiki/MinorReleases.

@nightlyone
Copy link
Contributor

If this invalidates the test cache after every git commit, this sounds like a really bad idea to me.

What real world use case would be enabled by this behaviour?

@earthboundkid
Copy link
Contributor Author

Testing that your binary contains build information.

Most people should have it set to auto or off most of the time. What’s the problem with slow builds for people who deliberately turn it on?

@gopherbot gopherbot modified the milestones: Go1.19, Go1.20 Aug 2, 2022
@dr2chase
Copy link
Contributor

With respect to backports, what's the status of this bug/fix?
#53177 (1.18 backport) is still open.

@bcmills
Copy link
Contributor

bcmills commented Aug 25, 2022

The backport is waiting on a bit of soak time. (The need for soak time is why it didn't make 1.19 as well.)

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

No branches or pull requests

7 participants