cmd/releasebot, cmd/release: include long tests in release process
The goal of this change is to reduce the chance of issuing a release
with an unintended regression that would be caught by running long
tests. This change adds long tests that are run during the -prepare
step, in addition to all the existing short tests that are run.
Executing the long tests is implemented by adding two new test-only
release targets. For a release to be considered complete, all release
targets must complete.
These test-only targets are built only for the purpose of verifying
their tests succeed, or to block the release otherwise.
They do not produce release artifacts.
The new test-only targets are named after the builder which is used
to perform their tests, and they are:
• linux-amd64-longtest
• windows-amd64-longtest
More builders may be added in the future, but care must be taken
to ensure the test execution environment is as close as possible
to that of build.golang.org post-submit builders, in order to
avoid false positives and false negatives.
As part of a gradual rollout, this change also adds a flag to skip
longtest builders. It's meant to be used in case a long test proves
to be flaky, and enough confidence can be gained through testing
elsewhere that the failure is not a regression caused by a change
merged to the release branch.
For now, its default value includes both longtest builders, so they
are currently opt-in and this CL is a no-op. After testing proves
that it is viable to rely on this (and any issues preventing that
from being possible are resolved), the default value of the flag
will be changed to the empty string.
For golang/go#29252.
For golang/go#39054.
For golang/go#37827.
Fixes golang/go#24614.
Change-Id: I33ea6601aade2873f857c63f00a0c11821f35a95
Reviewed-on: https://go-review.googlesource.com/c/build/+/234531
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Carlos Amedee <carlos@golang.org>
Reviewed-by: Alexander Rakoczy <alex@golang.org>
diff --git a/cmd/release/release.go b/cmd/release/release.go
index 83df39a..488ef8a 100644
--- a/cmd/release/release.go
+++ b/cmd/release/release.go
@@ -44,7 +44,7 @@
tarball = flag.String("tarball", "", "Go tree tarball to build, alternative to -rev")
version = flag.String("version", "", "Version string (go1.5.2)")
user = flag.String("user", username(), "coordinator username, appended to 'user-'")
- skipTests = flag.Bool("skip_tests", false, "skip tests; run make.bash instead of all.bash (only use if you ran trybots first)")
+ skipTests = flag.Bool("skip_tests", false, "skip tests; run make.bash but not all.bash (only use if sufficient testing was done elsewhere)")
uploadMode = flag.Bool("upload", false, "Upload files (exclusive to all other flags)")
uploadKick = flag.String("edge_kick_command", "", "Command to run to kick off an edge cache update")
@@ -108,20 +108,26 @@
Race bool // Build race detector.
- Builder string // Key for dashboard.Builders.
+ Builder string // Key for dashboard.Builders.
+ TestOnly bool // Run tests only; don't produce a release artifact.
- Goarm int // GOARM value if set.
- MakeOnly bool // don't run tests; needed by cross-compile builders (s390x)
+ Goarm int // GOARM value if set.
+ SkipTests bool // skip tests (run make.bash but not all.bash); needed by cross-compile builders (s390x)
}
func (b *Build) String() string {
- if b.Source {
+ switch {
+ case b.Source:
return "src"
- }
- if b.Goarm != 0 {
+ case b.TestOnly:
+ // Test-only builds are named after the builder used to
+ // perform them. For example, "linux-amd64-longtest".
+ return b.Builder
+ case b.Goarm != 0:
return fmt.Sprintf("%v-%vv%vl", b.OS, b.Arch, b.Goarm)
+ default:
+ return fmt.Sprintf("%v-%v", b.OS, b.Arch)
}
- return fmt.Sprintf("%v-%v", b.OS, b.Arch)
}
func (b *Build) toolDir() string { return "go/pkg/tool/" + b.OS + "_" + b.Arch }
@@ -149,13 +155,13 @@
Goarm: 6, // for compatibility with all Raspberry Pi models.
// The tests take too long for the release packaging.
// Much of the time the whole buildlet times out.
- MakeOnly: true,
+ SkipTests: true,
},
{
OS: "linux",
Arch: "amd64",
Race: true,
- Builder: "linux-amd64-jessie", // using Jessie for at least [Go 1.11, Go 1.13] due to golang.org/issue/31336
+ Builder: "linux-amd64-jessie", // using Jessie for at least [Go 1.11, Go 1.13] due to golang.org/issue/31293
},
{
OS: "linux",
@@ -191,20 +197,32 @@
Builder: "darwin-amd64-10_15",
},
{
- OS: "linux",
- Arch: "s390x",
- MakeOnly: true,
- Builder: "linux-s390x-crosscompile",
+ OS: "linux",
+ Arch: "s390x",
+ SkipTests: true,
+ Builder: "linux-s390x-crosscompile",
},
// TODO(bradfitz): switch this ppc64 builder to a Kubernetes
// container cross-compiling ppc64 like the s390x one? For
// now, the ppc64le builders (5) are back, so let's see if we
// can just depend on them not going away.
{
- OS: "linux",
- Arch: "ppc64le",
- MakeOnly: true,
- Builder: "linux-ppc64le-buildlet",
+ OS: "linux",
+ Arch: "ppc64le",
+ SkipTests: true,
+ Builder: "linux-ppc64le-buildlet",
+ },
+
+ // Test-only builds.
+ {
+ Builder: "linux-amd64-longtest",
+ OS: "linux", Arch: "amd64",
+ TestOnly: true,
+ },
+ {
+ Builder: "windows-amd64-longtest",
+ OS: "windows", Arch: "amd64",
+ TestOnly: true,
},
}
@@ -236,7 +254,7 @@
}
var hostArch string // non-empty if we're cross-compiling (s390x)
- if b.MakeOnly && bc.IsContainer() && (bc.GOARCH() != "amd64" && bc.GOARCH() != "386") {
+ if b.SkipTests && bc.IsContainer() && (bc.GOARCH() != "amd64" && bc.GOARCH() != "386") {
hostArch = "amd64"
}
@@ -464,7 +482,8 @@
// So far, we've run make.bash. We want to create the release archive next.
// Since the release archive hasn't been tested yet, place it in a temporary
// location. After all.bash runs successfully (or gets explicitly skipped),
- // we'll move the release archive to its final location.
+ // we'll move the release archive to its final location. For TestOnly builds,
+ // we only care whether tests passed and do not produce release artifacts.
type releaseFile struct {
Untested string // Temporary location of the file before the release has been tested.
Final string // Final location where to move the file after the release has been tested.
@@ -482,7 +501,7 @@
return filepath.Join(stagingDir, *version+"."+b.String()+ext+".untested")
}
- if b.OS == "windows" {
+ if !b.TestOnly && b.OS == "windows" {
untested := stagingFile(".msi")
if err := b.fetchFile(client, untested, "msi"); err != nil {
return err
@@ -491,6 +510,9 @@
Untested: untested,
Final: *version + "." + b.String() + ".msi",
})
+ }
+
+ if b.OS == "windows" {
cleanFiles = append(cleanFiles, "msi")
}
@@ -510,8 +532,8 @@
return fmt.Errorf("verifying file permissions: %v", err)
}
- switch b.OS {
- default:
+ switch {
+ case !b.TestOnly && b.OS != "windows":
untested := stagingFile(".tar.gz")
if err := b.fetchTarball(ctx, client, untested); err != nil {
return fmt.Errorf("fetching and writing tarball: %v", err)
@@ -520,7 +542,7 @@
Untested: untested,
Final: *version + "." + b.String() + ".tar.gz",
})
- case "windows":
+ case !b.TestOnly && b.OS == "windows":
untested := stagingFile(".zip")
if err := b.fetchZip(client, untested); err != nil {
return fmt.Errorf("fetching and writing zip: %v", err)
@@ -529,10 +551,23 @@
Untested: untested,
Final: *version + "." + b.String() + ".zip",
})
+ case b.TestOnly:
+ // Use an empty .test-only file to indicate the test outcome.
+ // This file gets moved from its initial location in the
+ // release-staging directory to the final release directory
+ // when the test-only build passes tests successfully.
+ untested := stagingFile(".test-only")
+ if err := ioutil.WriteFile(untested, nil, 0600); err != nil {
+ return fmt.Errorf("writing empty test-only file: %v", err)
+ }
+ releases = append(releases, releaseFile{
+ Untested: untested,
+ Final: *version + "." + b.String() + ".test-only",
+ })
}
// Execute build (all.bash) if running tests.
- if *skipTests || b.MakeOnly {
+ if *skipTests || b.SkipTests {
b.logf("Skipping all.bash tests.")
} else {
if u := bc.GoBootstrapURL(buildEnv); u != "" {
@@ -760,9 +795,12 @@
}
// checkRelocations runs readelf on pkg/linux_amd64/runtime/cgo.a and makes sure
-// we don't see R_X86_64_REX_GOTPCRELX. See issue 31293.
+// we don't see R_X86_64_REX_GOTPCRELX. See golang.org/issue/31293.
func (b *Build) checkRelocations(client *buildlet.Client) error {
- if b.OS != "linux" || b.Arch != "amd64" {
+ if b.OS != "linux" || b.Arch != "amd64" || b.TestOnly {
+ // This check is only applicable to linux/amd64 builds.
+ // However, skip it on test-only builds because they
+ // don't produce binaries that are shipped to users.
return nil
}
var out bytes.Buffer
diff --git a/cmd/release/release_test.go b/cmd/release/release_test.go
index 5634ee1..34609f0 100644
--- a/cmd/release/release_test.go
+++ b/cmd/release/release_test.go
@@ -19,6 +19,14 @@
}
}
+func TestTestOnlyBuildsDontSkipTests(t *testing.T) {
+ for _, b := range builds {
+ if b.TestOnly && b.SkipTests {
+ t.Errorf("build %s is configured to run tests only, but also to skip tests; is that intentional?", b)
+ }
+ }
+}
+
func TestMinSupportedMacOSVersion(t *testing.T) {
testCases := []struct {
desc string
diff --git a/cmd/releasebot/main.go b/cmd/releasebot/main.go
index f53d97d..51faa50 100644
--- a/cmd/releasebot/main.go
+++ b/cmd/releasebot/main.go
@@ -31,19 +31,32 @@
"golang.org/x/build/maintner"
)
-var releaseTargets = []string{
- "src",
- "linux-386",
- "linux-armv6l",
- "linux-amd64",
- "linux-arm64",
- "freebsd-386",
- "freebsd-amd64",
- "windows-386",
- "windows-amd64",
- "darwin-amd64",
- "linux-s390x",
- "linux-ppc64le",
+// A Target is a release target.
+type Target struct {
+ Name string // Target name as accepted by cmd/release. For example, "linux-amd64".
+ TestOnly bool // Run tests only; don't produce a release artifact.
+}
+
+var releaseTargets = []Target{
+ // Source-only target.
+ {Name: "src"},
+
+ // Binary targets.
+ {Name: "linux-386"},
+ {Name: "linux-armv6l"},
+ {Name: "linux-amd64"},
+ {Name: "linux-arm64"},
+ {Name: "freebsd-386"},
+ {Name: "freebsd-amd64"},
+ {Name: "windows-386"},
+ {Name: "windows-amd64"},
+ {Name: "darwin-amd64"},
+ {Name: "linux-s390x"},
+ {Name: "linux-ppc64le"},
+
+ // Test-only targets.
+ {Name: "linux-amd64-longtest", TestOnly: true},
+ {Name: "windows-amd64-longtest", TestOnly: true},
}
var releaseModes = map[string]bool{
@@ -53,10 +66,18 @@
func usage() {
fmt.Fprintln(os.Stderr, "usage: releasebot -mode {prepare|release} [-security] [-dry-run] {go1.8.5|go1.10beta2|go1.11rc1}")
+ flag.PrintDefaults()
os.Exit(2)
}
-var dryRun bool // only perform pre-flight checks, only log to terminal
+var (
+ skipTestFlag = flag.String("skip-test", "linux-amd64-longtest windows-amd64-longtest", "space-separated list of test-only targets to skip (only use if sufficient testing was done elsewhere)")
+)
+
+var (
+ dryRun bool // only perform pre-flight checks, only log to terminal
+ skipTest = make(map[string]bool) // test-only targets that should be skipped
+)
func main() {
modeFlag := flag.String("mode", "", "release mode (prepare, release)")
@@ -65,8 +86,19 @@
flag.Usage = usage
flag.Parse()
if *modeFlag == "" || !releaseModes[*modeFlag] || flag.NArg() != 1 {
+ fmt.Fprintln(os.Stderr, "need to provide a valid mode and a release name")
usage()
}
+ for _, target := range strings.Fields(*skipTestFlag) {
+ if t, ok := releaseTarget(target); !ok {
+ fmt.Fprintf(os.Stderr, "target %q in -skip-test=%q is not a known target\n", target, *skipTestFlag)
+ usage()
+ } else if !t.TestOnly {
+ fmt.Fprintf(os.Stderr, "%s is not a test-only target\n", target)
+ usage()
+ }
+ skipTest[target] = true
+ }
http.DefaultTransport = newLogger(http.DefaultTransport)
@@ -521,8 +553,8 @@
w.releaseMu.Lock()
defer w.releaseMu.Unlock()
for _, target := range releaseTargets {
- fmt.Fprintf(md, "%s", mdEscape(target))
- info := w.ReleaseInfo[target]
+ fmt.Fprintf(md, "%s", mdEscape(target.Name))
+ info := w.ReleaseInfo[target.Name]
if info == nil {
fmt.Fprintf(md, " not started\n")
continue
@@ -647,11 +679,17 @@
var wg sync.WaitGroup
for _, target := range releaseTargets {
- func() {
+ w.releaseMu.Lock()
+ w.ReleaseInfo[target.Name] = new(ReleaseInfo)
+ w.releaseMu.Unlock()
+
+ if target.TestOnly && skipTest[target.Name] {
+ w.log.Printf("skipping test-only target %s because of -skip-test=%q flag", target.Name, *skipTestFlag)
w.releaseMu.Lock()
- defer w.releaseMu.Unlock()
- w.ReleaseInfo[target] = new(ReleaseInfo)
- }()
+ w.ReleaseInfo[target.Name].Msg = fmt.Sprintf("skipped because of -skip-test=%q flag", *skipTestFlag)
+ w.releaseMu.Unlock()
+ continue
+ }
wg.Add(1)
target := target
@@ -662,9 +700,9 @@
stk := strings.TrimSpace(string(debug.Stack()))
msg := fmt.Sprintf("PANIC: %v\n\n %s\n", mdEscape(fmt.Sprint(err)), strings.Replace(stk, "\n", "\n ", -1))
w.logError(msg)
- w.log.Printf("\n\nBuilding %s: PANIC: %v\n\n%s", target, err, debug.Stack())
+ w.log.Printf("\n\nBuilding %s: PANIC: %v\n\n%s", target.Name, err, debug.Stack())
w.releaseMu.Lock()
- w.ReleaseInfo[target].Msg = msg
+ w.ReleaseInfo[target.Name].Msg = msg
w.releaseMu.Unlock()
}
}()
@@ -676,7 +714,7 @@
// Check for release errors and stop if any.
w.releaseMu.Lock()
for _, target := range releaseTargets {
- for _, out := range w.ReleaseInfo[target].Outputs {
+ for _, out := range w.ReleaseInfo[target.Name].Outputs {
if out.Error != "" || len(w.Errors) > 0 {
w.logError("RELEASE BUILD FAILED\n")
w.releaseMu.Unlock()
@@ -699,14 +737,16 @@
// they are reused instead of being rebuilt. In release mode, buildRelease then uploads
// the release packaging to the gs://golang-release-staging bucket, along with files
// containing the SHA256 hash of the releases, for eventual use by the download page.
-func (w *Work) buildRelease(target string) {
- log.Printf("BUILDRELEASE %s %s\n", w.Version, target)
- defer log.Printf("DONE BUILDRELEASE %s\n", target)
+func (w *Work) buildRelease(target Target) {
+ log.Printf("BUILDRELEASE %s %s\n", w.Version, target.Name)
+ defer log.Printf("DONE BUILDRELEASE %s %s\n", w.Version, target.Name)
releaseDir := filepath.Join(w.Dir, "release", w.VersionCommit)
- prefix := fmt.Sprintf("%s.%s.", w.Version, target)
+ prefix := fmt.Sprintf("%s.%s.", w.Version, target.Name)
var files []string
switch {
- case strings.HasPrefix(target, "windows-"):
+ case target.TestOnly:
+ files = []string{prefix + "test-only"}
+ case strings.HasPrefix(target.Name, "windows-"):
files = []string{prefix + "zip", prefix + "msi"}
default:
files = []string{prefix + "tar.gz"}
@@ -725,15 +765,15 @@
}
}
w.releaseMu.Lock()
- w.ReleaseInfo[target].Outputs = outs
+ w.ReleaseInfo[target.Name].Outputs = outs
w.releaseMu.Unlock()
if haveFiles {
- w.log.Printf("release %s: already have %v; not rebuilding files", target, files)
+ w.log.Printf("release -target=%q: already have %v; not rebuilding files", target.Name, files)
} else {
failures := 0
for {
- args := []string{w.ReleaseBinary, "-target", target, "-user", gomoteUser,
+ args := []string{w.ReleaseBinary, "-target", target.Name, "-user", gomoteUser,
"-version", w.Version, "-staging_dir", w.StagingDir}
if w.Security {
args = append(args, "-tarball", filepath.Join(w.Dir, w.VersionCommit+".tar.gz"))
@@ -746,7 +786,7 @@
if !w.Prepare {
args = append(args, "-skip_tests")
}
- out, err := w.runner(releaseDir, "GOPATH="+filepath.Join(w.Dir, "gopath")).runErr(args...)
+ releaseOutput, releaseError := w.runner(releaseDir, "GOPATH="+filepath.Join(w.Dir, "gopath")).runErr(args...)
// Exit code from release binary is apparently unreliable.
// Look to see if the files we expected were created instead.
failed := false
@@ -760,12 +800,12 @@
if !failed {
break
}
- w.log.Printf("release %s:\nerror from cmd/release binary = %v\noutput from cmd/release binary:\n%s", target, err, out)
+ w.log.Printf("release -target=%q did not produce expected output files %v:\nerror from cmd/release binary = %v\noutput from cmd/release binary:\n%s", target.Name, files, releaseError, releaseOutput)
if failures++; failures >= 3 {
- w.log.Printf("release %s: too many failures\n", target)
+ w.log.Printf("release -target=%q: too many failures\n", target.Name)
for _, out := range outs {
w.releaseMu.Lock()
- out.Error = fmt.Sprintf("release %s: build failed", target)
+ out.Error = fmt.Sprintf("release -target=%q: build failed", target.Name)
w.releaseMu.Unlock()
}
return
@@ -778,9 +818,14 @@
return
}
+ if target.TestOnly {
+ // This was a test-only target, nothing to upload.
+ return
+ }
+
for _, out := range outs {
if err := w.uploadStagingRelease(target, out); err != nil {
- w.log.Printf("release %s: %s", target, err)
+ w.log.Printf("error uploading release %s to staging bucket: %s", target.Name, err)
w.releaseMu.Lock()
out.Error = err.Error()
w.releaseMu.Unlock()
@@ -794,9 +839,11 @@
// named "<target>.sha256" containing the hex sha256 hash
// of the target file. This is needed for the release signing process
// and also displayed on the eventual download page.
-func (w *Work) uploadStagingRelease(target string, out *ReleaseOutput) error {
+func (w *Work) uploadStagingRelease(target Target, out *ReleaseOutput) error {
if dryRun {
return errors.New("attempted write operation in dry-run mode")
+ } else if target.TestOnly {
+ return errors.New("attempted to upload a test-only target")
}
src := filepath.Join(w.Dir, "release", w.VersionCommit, out.File)
@@ -848,3 +895,13 @@
log.Fatalf("release branch does not contain security release HEAD commit %q; aborting", sha)
}
}
+
+// releaseTarget returns a release target with the specified name.
+func releaseTarget(name string) (_ Target, ok bool) {
+ for _, t := range releaseTargets {
+ if t.Name == name {
+ return t, true
+ }
+ }
+ return Target{}, false
+}
