Skip to content

Commit

Permalink
cmd/go: avoid stamping VCS metadata in test binaries
Browse files Browse the repository at this point in the history
Invoking a VCS tool requires that the VCS tool be installed, and also
adds latency to build commands. Unfortunately, we had been mistakenly
loading VCS metadata for tests of "main" packages.

Users almost never care about versioning for test binaries, because
'go test' runs the test in the source tree and test binaries are only
rarely used outside of 'go test'. So the user already knows exactly
which version the test is built against, because the source code is
right there — it's not worth the overhead to stamp.

Fixes #51723.

Change-Id: I96f191c5a765f5183e5e10b6dfb75a0381c99814
Reviewed-on: https://go-review.googlesource.com/c/go/+/393894
Trust: Bryan Mills <bcmills@google.com>
Run-TryBot: Bryan Mills <bcmills@google.com>
Reviewed-by: Daniel Martí <mvdan@mvdan.cc>
Trust: Daniel Martí <mvdan@mvdan.cc>
TryBot-Result: Gopher Robot <gobot@golang.org>
  • Loading branch information
Bryan C. Mills committed Mar 18, 2022
1 parent 9465878 commit 67f6b8c
Show file tree
Hide file tree
Showing 5 changed files with 123 additions and 11 deletions.
1 change: 1 addition & 0 deletions src/cmd/go/internal/list/list.go
Original file line number Diff line number Diff line change
Expand Up @@ -567,6 +567,7 @@ func runList(ctx context.Context, cmd *base.Command, args []string) {
pkgOpts := load.PackageOpts{
IgnoreImports: *listFind,
ModResolveTests: *listTest,
LoadVCS: cfg.BuildBuildvcs,
}
pkgs := load.PackagesAndErrors(ctx, pkgOpts, args)
if !*listE {
Expand Down
31 changes: 25 additions & 6 deletions src/cmd/go/internal/load/pkg.go
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,18 @@ func (p *Package) Desc() string {
return p.ImportPath
}

// IsTestOnly reports whether p is a test-only package.
//
// A “test-only” package is one that:
// - is a test-only variant of an ordinary package, or
// - is a synthesized "main" package for a test binary, or
// - contains only _test.go files.
func (p *Package) IsTestOnly() bool {
return p.ForTest != "" ||
p.Internal.TestmainGo != nil ||
len(p.TestGoFiles)+len(p.XTestGoFiles) > 0 && len(p.GoFiles)+len(p.CgoFiles) == 0
}

type PackageInternal struct {
// Unexported fields are not part of the public API.
Build *build.Package
Expand Down Expand Up @@ -1926,8 +1938,12 @@ func (p *Package) load(ctx context.Context, opts PackageOpts, path string, stk *
}
p.Internal.Imports = imports
p.collectDeps()
if p.Error == nil && p.Name == "main" && len(p.DepsErrors) == 0 {
p.setBuildInfo()
if p.Error == nil && p.Name == "main" && !p.Internal.ForceLibrary && len(p.DepsErrors) == 0 {
// TODO(bcmills): loading VCS metadata can be fairly slow.
// Consider starting this as a background goroutine and retrieving the result
// asynchronously when we're actually ready to build the package, or when we
// actually need to evaluate whether the package's metadata is stale.
p.setBuildInfo(opts.LoadVCS)
}

// unsafe is a fake package.
Expand Down Expand Up @@ -2216,7 +2232,7 @@ var vcsStatusCache par.Cache
//
// Note that the GoVersion field is not set here to avoid encoding it twice.
// It is stored separately in the binary, mostly for historical reasons.
func (p *Package) setBuildInfo() {
func (p *Package) setBuildInfo(includeVCS bool) {
// TODO: build and vcs information is not embedded for executables in GOROOT.
// cmd/dist uses -gcflags=all= -ldflags=all= by default, which means these
// executables always appear stale unless the user sets the same flags.
Expand Down Expand Up @@ -2346,8 +2362,8 @@ func (p *Package) setBuildInfo() {
// Add VCS status if all conditions are true:
//
// - -buildvcs is enabled.
// - p is contained within a main module (there may be multiple main modules
// in a workspace, but local replacements don't count).
// - p is a non-test contained within a main module (there may be multiple
// main modules in a workspace, but local replacements don't count).
// - Both the current directory and p's module's root directory are contained
// in the same local repository.
// - We know the VCS commands needed to get the status.
Expand All @@ -2359,7 +2375,7 @@ func (p *Package) setBuildInfo() {
var vcsCmd *vcs.Cmd
var err error
const allowNesting = true
if cfg.BuildBuildvcs && p.Module != nil && p.Module.Version == "" && !p.Standard {
if includeVCS && p.Module != nil && p.Module.Version == "" && !p.Standard && !p.IsTestOnly() {
repoDir, vcsCmd, err = vcs.FromDir(base.Cwd(), "", allowNesting)
if err != nil && !errors.Is(err, os.ErrNotExist) {
setVCSError(err)
Expand Down Expand Up @@ -2648,6 +2664,9 @@ type PackageOpts struct {
// are not be matched, and their dependencies may not be loaded. A warning
// may be printed for non-literal arguments that match no main packages.
MainOnly bool

// LoadVCS controls whether we also load version-control metadata for main packages.
LoadVCS bool
}

// PackagesAndErrors returns the packages named by the command line arguments
Expand Down
6 changes: 3 additions & 3 deletions src/cmd/go/internal/load/test.go
Original file line number Diff line number Diff line change
Expand Up @@ -368,9 +368,9 @@ func TestPackagesAndErrors(ctx context.Context, opts PackageOpts, p *Package, co
if err != nil && pmain.Error == nil {
pmain.Error = &PackageError{Err: err}
}
if data != nil {
pmain.Internal.TestmainGo = &data
}
// Set TestmainGo even if it is empty: the presence of a TestmainGo
// indicates that this package is, in fact, a test main.
pmain.Internal.TestmainGo = &data

return pmain, ptest, pxtest
}
Expand Down
4 changes: 2 additions & 2 deletions src/cmd/go/internal/work/build.go
Original file line number Diff line number Diff line change
Expand Up @@ -379,7 +379,7 @@ func runBuild(ctx context.Context, cmd *base.Command, args []string) {
var b Builder
b.Init()

pkgs := load.PackagesAndErrors(ctx, load.PackageOpts{}, args)
pkgs := load.PackagesAndErrors(ctx, load.PackageOpts{LoadVCS: cfg.BuildBuildvcs}, args)
load.CheckPackageErrors(pkgs)

explicitO := len(cfg.BuildO) > 0
Expand Down Expand Up @@ -603,7 +603,7 @@ func runInstall(ctx context.Context, cmd *base.Command, args []string) {

modload.InitWorkfile()
BuildInit()
pkgs := load.PackagesAndErrors(ctx, load.PackageOpts{}, args)
pkgs := load.PackagesAndErrors(ctx, load.PackageOpts{LoadVCS: cfg.BuildBuildvcs}, args)
if cfg.ModulesEnabled && !modload.HasModRoot() {
haveErrors := false
allMissingErrors := true
Expand Down
92 changes: 92 additions & 0 deletions src/cmd/go/testdata/script/test_buildvcs.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
# https://go.dev/issue/51723: 'go test' should not stamp VCS metadata
# in the build settings. (It isn't worth the latency hit, given that
# test binaries are almost never distributed to users.)

[short] skip
[!exec:git] skip

exec git init

# The test binaries should not have VCS settings stamped.
# (The test itself verifies that.)
go test . ./testonly


# Remove 'git' from $PATH. The test should still build.
# This ensures that we aren't loading VCS metadata that
# we subsequently throw away.
env PATH=''
env path=''

# Compiling the test should not require the VCS tool.
go test -c -o $devnull .


# When listing a main package, in general we need its VCS metadata to determine
# the .Stale and .StaleReason fields.
! go list .
stderr '^go: missing Git command\. See https://golang\.org/s/gogetcmd\nerror obtaining VCS status: .*\n\tUse -buildvcs=false to disable VCS stamping.'

# Adding the -test flag should be strictly additive — it should not suppress the error.
! go list -test .
stderr '^go: missing Git command\. See https://golang\.org/s/gogetcmd\nerror obtaining VCS status: .*\n\tUse -buildvcs=false to disable VCS stamping.'

# Adding the suggested flag should suppress the error.
go list -test -buildvcs=false .
! stderr .


# Since the ./testonly package can't produce an actual binary, we shouldn't
# invoke a VCS tool to compute a build stamp when listing it.
go list ./testonly
! stderr .
go list -test ./testonly
! stderr .


-- go.mod --
module example

go 1.18
-- example.go --
package main
-- example_test.go --
package main

import (
"runtime/debug"
"strings"
"testing"
)

func TestDetail(t *testing.T) {
bi, ok := debug.ReadBuildInfo()
if !ok {
t.Fatal("BuildInfo not present")
}
for _, s := range bi.Settings {
if strings.HasPrefix(s.Key, "vcs.") {
t.Fatalf("unexpected VCS setting: %s=%s", s.Key, s.Value)
}
}
}
-- testonly/main_test.go --
package main

import (
"runtime/debug"
"strings"
"testing"
)

func TestDetail(t *testing.T) {
bi, ok := debug.ReadBuildInfo()
if !ok {
t.Fatal("BuildInfo not present")
}
for _, s := range bi.Settings {
if strings.HasPrefix(s.Key, "vcs.") {
t.Fatalf("unexpected VCS setting: %s=%s", s.Key, s.Value)
}
}
}

0 comments on commit 67f6b8c

Please sign in to comment.