sweet: apply flags to cockroachdb 'go build' based on 'go version'
Currently, the cockroachdb benchmark is built by passing a build tag
(untested_go_version) unconditionally, and we also try to build with and
without '-checklinkname=0' passed to the linker. These are required
because some CockroachDB packages reach into the runtime internals.
The current approach doesn't work well across Go versions because
setting "untested_go_version" for older toolchains causes a symbol
conflict (it ends up enabling multiple files with the same definition).
We can fix this upstream, but we also need a fix in the short term.
Plus, it'll help to clean up this logic and make it more explicit.
So, this CL parses 'go version' in a dead-simple way to determine what
to do. If we're using an older version of Go and the version is
well-defined, then we never use 'untested_go_version'. But if the
version of Go is explicitly a development version or newer than Go
1.24, then we do.
Simultaneously, we only pass -checklinkname=0 to the linker if the
toolchain version is >= 1.23. We always pass it for development Go
versions.
Fixes golang/go#68555.
Change-Id: Ieaad89cb6a92bdeee2636dc62b5b3a26ac317b5f
Cq-Include-Trybots: luci.golang.try:x_benchmarks-gotip-linux-amd64-longtest,x_benchmarks-go1.22-linux-amd64-longtest
Reviewed-on: https://go-review.googlesource.com/c/benchmarks/+/600475
Reviewed-by: Austin Clements <austin@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Michael Knyszek <mknyszek@google.com>
diff --git a/sweet/common/gotool.go b/sweet/common/gotool.go
index 08dd1a7..3fc2b6d 100644
--- a/sweet/common/gotool.go
+++ b/sweet/common/gotool.go
@@ -72,6 +72,17 @@
return g.Do("", "build", "-o", out, pkg)
}
+func (g *Go) Version() (string, error) {
+ cmd := exec.Command(g.Tool, "version")
+ cmd.Env = g.Env.Collapse()
+ log.TraceCommand(cmd, false)
+ out, err := cmd.Output()
+ if err != nil {
+ return "", fmt.Errorf("error running 'go version': %w", err)
+ }
+ return string(out), nil
+}
+
func (g *Go) BuildPath(path, out string, args ...string) error {
if path[0] != '/' && path[0] != '.' {
path = "./" + path
diff --git a/sweet/harnesses/cockroachdb.go b/sweet/harnesses/cockroachdb.go
index 75a4b86..04cecb7 100644
--- a/sweet/harnesses/cockroachdb.go
+++ b/sweet/harnesses/cockroachdb.go
@@ -5,12 +5,12 @@
package harnesses
import (
- "errors"
"fmt"
"os"
"os/exec"
"path/filepath"
"runtime"
+ "strings"
"time"
"golang.org/x/benchmarks/sweet/common"
@@ -98,22 +98,32 @@
return err
}
- // Finally build the cockroach binary with `go build`. Build the
- // cockroach-short binary as it is functionally the same, but
- // without the UI, making it much quicker to build.
+ // Get the Go version. Then, finall build the cockroach binary with `go build`.
//
- // As of go1.23, we need to pass the `-ldflags=-checklinkname=0` flag
- // to build cockroach. However, benchmark release branches are on older
- // versions that don't recognize the flag. Try first with the flag and
- // again without if there is an error.
+ // Build the cockroach-short binary as it is functionally the same, but without
+ // the UI, making it much quicker to build.
//
- // Cockroachdb reaches into runtime internals, so it has a negative build
- // tag to exclude unreleased Go versions. Set untested_go_version so we can
- // run at tip (and cross our fingers).
- if buildWithFlagErr := cfg.GoTool().BuildPath(filepath.Join(bcfg.SrcDir, "pkg/cmd/cockroach-short"), bcfg.BinDir, "-tags=untested_go_version", "-ldflags=-checklinkname=0"); buildWithFlagErr != nil {
- if buildWithoutFlagErr := cfg.GoTool().BuildPath(filepath.Join(bcfg.SrcDir, "pkg/cmd/cockroach-short"), bcfg.BinDir, "-tags=untested_go_version"); buildWithoutFlagErr != nil {
- return errors.Join(buildWithFlagErr, buildWithoutFlagErr)
- }
+ // CockroachDB reaches into runtime internals, so we need to pass -checklinkname=0
+ // to the linker to get it to build with Go 1.23+. Also, CockroachDB has a negative
+ // build tag to exclude unreleased Go versions, so we need to explicitly set it for
+ // new-enough Go versions.
+ //
+ // Note that since the version of CockroachDB is pinned, we can generally rely on
+ // the checking below. However as CockroachDB and Go evolve, the logic below may need
+ // to change.
+ ver, err := cfg.GoTool().Version()
+ if err != nil {
+ return fmt.Errorf("getting go version for toolchain: %v", err)
+ }
+ var goBuildArgs []string
+ if v := strings.TrimPrefix(ver, "go version "); strings.HasPrefix(v, "devel ") || v >= "go1.23" {
+ goBuildArgs = append(goBuildArgs, "-ldflags=-checklinkname=0")
+ }
+ if v := strings.TrimPrefix(ver, "go version "); strings.HasPrefix(v, "devel ") || v >= "go1.24" {
+ goBuildArgs = append(goBuildArgs, "-tags=untested_go_version")
+ }
+ if err := cfg.GoTool().BuildPath(filepath.Join(bcfg.SrcDir, "pkg/cmd/cockroach-short"), bcfg.BinDir, goBuildArgs...); err != nil {
+ return err
}
// Rename the binary from cockroach-short to cockroach for
@@ -126,7 +136,6 @@
if err := cfg.GoTool().BuildPath(bcfg.BenchDir, filepath.Join(bcfg.BinDir, "cockroachdb-bench")); err != nil {
return err
}
-
return nil
}
