From af3b81dc88f863aeed3200c89afa50be6e85bee9 Mon Sep 17 00:00:00 2001 From: "Bryan C. Mills" Date: Wed, 24 Aug 2022 09:45:18 -0400 Subject: [PATCH] [release-branch.go1.19] cmd/go: avoid registering AtExit handlers in tests Ever since 'go build' was added (in CL 5483069), it has used an atexit handler to clean up working directories. CL 154109 introduced 'cc' command to the script test framework that called Init on a builder once per invocation. Unfortunately, since base.AtExit is unsynchronized, the Init added there caused any script that invokes that command to be unsafe for concurrent use. This change fixes the race by having the 'cc' command pass in its working directory instead of allowing the Builder to allocate one. Following modern Go best practices, it also replaces the in-place Init method (which is prone to typestate and aliasing bugs) with a NewBuilder constructor function. Updates #54423. Fixes #54637. Change-Id: I8fc2127a7d877bb39a1174e398736bb51d03d4d2 Reviewed-on: https://go-review.googlesource.com/c/go/+/425205 TryBot-Result: Gopher Robot Run-TryBot: Bryan Mills Auto-Submit: Bryan Mills Reviewed-by: Than McIntosh (cherry picked from commit d5aa088d822bc8ef3ceb80c20184f40fcb9b8d2e) Reviewed-on: https://go-review.googlesource.com/c/go/+/425207 --- src/cmd/go/internal/envcmd/env.go | 4 ++-- src/cmd/go/internal/list/list.go | 3 +-- src/cmd/go/internal/run/run.go | 3 +-- src/cmd/go/internal/test/test.go | 16 ++++++++++++---- src/cmd/go/internal/vet/vet.go | 3 +-- src/cmd/go/internal/work/action.go | 14 ++++++++++++-- src/cmd/go/internal/work/build.go | 7 +++---- src/cmd/go/script_test.go | 4 +--- 8 files changed, 33 insertions(+), 21 deletions(-) diff --git a/src/cmd/go/internal/envcmd/env.go b/src/cmd/go/internal/envcmd/env.go index 529351dfbd8a78..81ee859e66f3e5 100644 --- a/src/cmd/go/internal/envcmd/env.go +++ b/src/cmd/go/internal/envcmd/env.go @@ -174,8 +174,8 @@ func ExtraEnvVars() []cfg.EnvVar { // ExtraEnvVarsCostly returns environment variables that should not leak into child processes // but are costly to evaluate. func ExtraEnvVarsCostly() []cfg.EnvVar { - var b work.Builder - b.Init() + b := work.NewBuilder("") + cppflags, cflags, cxxflags, fflags, ldflags, err := b.CFlags(&load.Package{}) if err != nil { // Should not happen - b.CFlags was given an empty package. diff --git a/src/cmd/go/internal/list/list.go b/src/cmd/go/internal/list/list.go index 5f8be6e3c9d9ee..0be3d987075290 100644 --- a/src/cmd/go/internal/list/list.go +++ b/src/cmd/go/internal/list/list.go @@ -689,8 +689,7 @@ func runList(ctx context.Context, cmd *base.Command, args []string) { // Do we need to run a build to gather information? needStale := (listJson && listJsonFields.needAny("Stale", "StaleReason")) || strings.Contains(*listFmt, ".Stale") if needStale || *listExport || *listCompiled { - var b work.Builder - b.Init() + b := work.NewBuilder("") b.IsCmdList = true b.NeedExport = *listExport b.NeedCompiledGoFiles = *listCompiled diff --git a/src/cmd/go/internal/run/run.go b/src/cmd/go/internal/run/run.go index ebe161181901bb..6b253a2c9e52e5 100644 --- a/src/cmd/go/internal/run/run.go +++ b/src/cmd/go/internal/run/run.go @@ -91,8 +91,7 @@ func runRun(ctx context.Context, cmd *base.Command, args []string) { } work.BuildInit() - var b work.Builder - b.Init() + b := work.NewBuilder("") b.Print = printStderr i := 0 diff --git a/src/cmd/go/internal/test/test.go b/src/cmd/go/internal/test/test.go index aa5e41e0043801..028db84cc462bc 100644 --- a/src/cmd/go/internal/test/test.go +++ b/src/cmd/go/internal/test/test.go @@ -744,8 +744,7 @@ func runTest(ctx context.Context, cmd *base.Command, args []string) { } } - var b work.Builder - b.Init() + b := work.NewBuilder("") if cfg.BuildI { fmt.Fprint(os.Stderr, "go: -i flag is deprecated\n") @@ -800,7 +799,16 @@ func runTest(ctx context.Context, cmd *base.Command, args []string) { if !testC || a.Failed { return } - b.Init() + + // TODO(bcmills): I have no idea why the Builder must be reset here, but + // without this reset dance, TestGoTestDashIDashOWritesBinary fails with + // lots of "vet config not found" errors. This was added in CL 5699088, + // which had almost no public discussion, a very short commit description, + // and left no comment in the code to explain what is going on here. 🤯 + // + // Maybe this has the effect of removing actions that were registered by the + // call to CompileAction above? + b = work.NewBuilder("") } var builds, runs, prints []*work.Action @@ -916,7 +924,7 @@ func runTest(ctx context.Context, cmd *base.Command, args []string) { ensureImport(p, "sync/atomic") } - buildTest, runTest, printTest, err := builderTest(&b, ctx, pkgOpts, p, allImports[p]) + buildTest, runTest, printTest, err := builderTest(b, ctx, pkgOpts, p, allImports[p]) if err != nil { str := err.Error() str = strings.TrimPrefix(str, "\n") diff --git a/src/cmd/go/internal/vet/vet.go b/src/cmd/go/internal/vet/vet.go index a0b11fdd3dc5f1..085c7d23b24699 100644 --- a/src/cmd/go/internal/vet/vet.go +++ b/src/cmd/go/internal/vet/vet.go @@ -94,8 +94,7 @@ func runVet(ctx context.Context, cmd *base.Command, args []string) { base.Fatalf("no packages to vet") } - var b work.Builder - b.Init() + b := work.NewBuilder("") root := &work.Action{Mode: "go vet"} for _, p := range pkgs { diff --git a/src/cmd/go/internal/work/action.go b/src/cmd/go/internal/work/action.go index c0862c5efe503a..4bbd23ab8e4321 100644 --- a/src/cmd/go/internal/work/action.go +++ b/src/cmd/go/internal/work/action.go @@ -240,7 +240,13 @@ const ( ModeVetOnly = 1 << 8 ) -func (b *Builder) Init() { +// NewBuilder returns a new Builder ready for use. +// +// If workDir is the empty string, NewBuilder creates a WorkDir if needed +// and arranges for it to be removed in case of an unclean exit. +func NewBuilder(workDir string) *Builder { + b := new(Builder) + b.Print = func(a ...any) (int, error) { return fmt.Fprint(os.Stderr, a...) } @@ -249,7 +255,9 @@ func (b *Builder) Init() { b.toolIDCache = make(map[string]string) b.buildIDCache = make(map[string]string) - if cfg.BuildN { + if workDir != "" { + b.WorkDir = workDir + } else if cfg.BuildN { b.WorkDir = "$WORK" } else { tmp, err := os.MkdirTemp(cfg.Getenv("GOTMPDIR"), "go-build") @@ -306,6 +314,8 @@ func (b *Builder) Init() { base.Exit() } } + + return b } func CheckGOOSARCHPair(goos, goarch string) error { diff --git a/src/cmd/go/internal/work/build.go b/src/cmd/go/internal/work/build.go index 42745d992814c9..5f11cdabaf1bc9 100644 --- a/src/cmd/go/internal/work/build.go +++ b/src/cmd/go/internal/work/build.go @@ -403,8 +403,7 @@ var RuntimeVersion = runtime.Version() func runBuild(ctx context.Context, cmd *base.Command, args []string) { modload.InitWorkfile() BuildInit() - var b Builder - b.Init() + b := NewBuilder("") pkgs := load.PackagesAndErrors(ctx, load.PackageOpts{LoadVCS: true}, args) load.CheckPackageErrors(pkgs) @@ -728,8 +727,8 @@ func InstallPackages(ctx context.Context, patterns []string, pkgs []*load.Packag } base.ExitIfErrors() - var b Builder - b.Init() + b := NewBuilder("") + depMode := ModeBuild if cfg.BuildI { depMode = ModeInstall diff --git a/src/cmd/go/script_test.go b/src/cmd/go/script_test.go index 809dfb452f3668..ffdfe12bafc795 100644 --- a/src/cmd/go/script_test.go +++ b/src/cmd/go/script_test.go @@ -556,10 +556,8 @@ func (ts *testScript) cmdCc(want simpleStatus, args []string) { ts.fatalf("usage: cc args... [&]") } - var b work.Builder - b.Init() + b := work.NewBuilder(ts.workdir) ts.cmdExec(want, append(b.GccCmd(".", ""), args...)) - robustio.RemoveAll(b.WorkDir) } // cd changes to a different directory.