Previously, the cmd/fix tool, which is analogous to a compiler
in a "go fix" or "go vet -fix" build, applied its fixes directly
to source files during the build. However, this led to races
since the edits may in some cases occur concurrently with other
build steps that are still reading those source file.
This change separates the computation of the fixes, which
happens during the build, and applying the fixes, which happens
in a phase after the build.
The unitchecker now accepts a FixArchive file name (see CL 726940).
If it is non-empty, the unitchecker will write the fixed files
into an archive instead of updating them directly.
The vet build sets this option, then reads the produced zip
files in the second phase. The files are saved in the cache;
some care is required to sequence the various cache operations
so that a cache hit has all-or-nothing semantics.
The tweak to vet_basic.txt is a sign that there was a latent
bug, inadvertently fixed by this change: because the old approach
relied on side effects of cmd/fix to mutate files, rather than
the current pure-functional approach of computing fixes which
are then applied as a second pass, a cache hit would cause some
edits not to be applied. Now they are applied.
Fixes golang/go#71859
Change-Id: Ib8e70644ec246dcdb20a90794c11ea6fd420247d
Reviewed-on: https://go-review.googlesource.com/c/go/+/727000
Auto-Submit: Alan Donovan <adonovan@google.com>
Reviewed-by: Michael Matloob <matloob@golang.org>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Michael Matloob <matloob@google.com>
return
}
- vet := b.VetAction(loaderstate, work.ModeBuild, work.ModeBuild, p)
+ vet := b.VetAction(loaderstate, work.ModeBuild, work.ModeBuild, false, p)
runAction.Deps = append(runAction.Deps, vet)
// Install will clean the build directory.
// Make sure vet runs first.
package vet
import (
+ "archive/zip"
+ "bytes"
"context"
"encoding/json"
"errors"
work.VetExplicit = len(toolFlags) > 0
+ applyFixes := false
if cmd.Name() == "fix" || *vetFixFlag {
// fix mode: 'go fix' or 'go vet -fix'
if jsonFlag {
toolFlags = append(toolFlags, "-fix")
if diffFlag {
toolFlags = append(toolFlags, "-diff")
+ } else {
+ applyFixes = true
}
}
if contextFlag != -1 {
base.Fatalf("no packages to %s", cmd.Name())
}
+ // Build action graph.
b := work.NewBuilder("", moduleLoaderState.VendorDirOrEmpty)
defer func() {
if err := b.Close(); err != nil {
}
}()
+ root := &work.Action{Mode: "go " + cmd.Name()}
+
+ addVetAction := func(p *load.Package) {
+ act := b.VetAction(moduleLoaderState, work.ModeBuild, work.ModeBuild, applyFixes, p)
+ root.Deps = append(root.Deps, act)
+ }
+
// To avoid file corruption from duplicate application of
// fixes (in fix mode), and duplicate reporting of diagnostics
// (in vet mode), we must run the tool only once for each
// We needn't worry about intermediate test variants, as they
// will only be executed in VetxOnly mode, for facts but not
// diagnostics.
-
- root := &work.Action{Mode: "go " + cmd.Name()}
for _, p := range pkgs {
_, ptest, pxtest, perr := load.TestPackagesFor(moduleLoaderState, ctx, pkgOpts, p, nil)
if perr != nil {
}
if len(ptest.GoFiles) > 0 || len(ptest.CgoFiles) > 0 {
// The test package includes all the files of primary package.
- root.Deps = append(root.Deps, b.VetAction(moduleLoaderState, work.ModeBuild, work.ModeBuild, ptest))
+ addVetAction(ptest)
}
if pxtest != nil {
- root.Deps = append(root.Deps, b.VetAction(moduleLoaderState, work.ModeBuild, work.ModeBuild, pxtest))
+ addVetAction(pxtest)
}
}
b.Do(ctx, root)
+
+ // Apply fixes.
+ //
+ // We do this as a separate phase after the build to avoid
+ // races between source file updates and reads of those same
+ // files by concurrent actions of the ongoing build.
+ //
+ // If a file is fixed by multiple actions, they must be consistent.
+ if applyFixes {
+ contents := make(map[string][]byte)
+ // Gather the fixes.
+ for _, act := range root.Deps {
+ if act.FixArchive != "" {
+ if err := readZip(act.FixArchive, contents); err != nil {
+ base.Errorf("reading archive of fixes: %v", err)
+ return
+ }
+ }
+ }
+ // Apply them.
+ for filename, content := range contents {
+ if err := os.WriteFile(filename, content, 0644); err != nil {
+ base.Errorf("applying fix: %v", err)
+ }
+ }
+ }
+}
+
+// readZip reads the zipfile entries into the provided map.
+// It reports an error if updating the map would change an existing entry.
+func readZip(zipfile string, out map[string][]byte) error {
+ r, err := zip.OpenReader(zipfile)
+ if err != nil {
+ return err
+ }
+ defer r.Close() // ignore error
+ for _, f := range r.File {
+ rc, err := f.Open()
+ if err != nil {
+ return err
+ }
+ content, err := io.ReadAll(rc)
+ rc.Close() // ignore error
+ if err != nil {
+ return err
+ }
+ if prev, ok := out[f.Name]; ok && !bytes.Equal(prev, content) {
+ return fmt.Errorf("inconsistent fixes to file %v", f.Name)
+ }
+ out[f.Name] = content
+ }
+ return nil
}
// printJSONDiagnostics parses JSON (from the tool's stdout) and
actionID cache.ActionID // cache ID of action input
buildID string // build ID of action output
- VetxOnly bool // Mode=="vet": only being called to supply info about dependencies
- needVet bool // Mode=="build": need to fill in vet config
- needBuild bool // Mode=="build": need to do actual build (can be false if needVet is true)
- vetCfg *vetConfig // vet config
- output []byte // output redirect buffer (nil means use b.Print)
+ VetxOnly bool // Mode=="vet": only being called to supply info about dependencies
+ needVet bool // Mode=="build": need to fill in vet config
+ needBuild bool // Mode=="build": need to do actual build (can be false if needVet is true)
+ needFix bool // Mode=="vet": need secondary target, a .zip file containing fixes
+ vetCfg *vetConfig // vet config
+ FixArchive string // the created .zip file containing fixes (if needFix)
+ output []byte // output redirect buffer (nil means use b.Print)
sh *Shell // lazily created per-Action shell; see Builder.Shell
// It depends on the action for compiling p.
// If the caller may be causing p to be installed, it is up to the caller
// to make sure that the install depends on (runs after) vet.
-func (b *Builder) VetAction(s *modload.State, mode, depMode BuildMode, p *load.Package) *Action {
+func (b *Builder) VetAction(s *modload.State, mode, depMode BuildMode, needFix bool, p *load.Package) *Action {
a := b.vetAction(s, mode, depMode, p)
a.VetxOnly = false
+ a.needFix = needFix
return a
}
VetxOutput string // write vetx data to this output file
Stdout string // write stdout (JSON, unified diff) to this output file
GoVersion string // Go version for package
+ FixArchive string // write fixed files to this zip archive, if non-empty
SucceedOnTypecheckFailure bool // awful hack; see #18395 and below
}
vcfg.VetxOnly = a.VetxOnly
vcfg.VetxOutput = a.Objdir + "vet.out"
vcfg.Stdout = a.Objdir + "vet.stdout"
+ if a.needFix {
+ vcfg.FixArchive = a.Objdir + "vet.fix.zip"
+ }
vcfg.PackageVetx = make(map[string]string)
h := cache.NewHash("vet " + a.Package.ImportPath)
vcfg.PackageVetx[a1.Package.ImportPath] = a1.built
}
}
- vetxKey := cache.ActionID(h.Sum()) // for .vetx file
-
- fmt.Fprintf(h, "stdout\n")
- stdoutKey := cache.ActionID(h.Sum()) // for .stdout file
+ var (
+ id = cache.ActionID(h.Sum()) // for .vetx file
+ stdoutKey = cache.Subkey(id, "stdout") // for .stdout file
+ fixArchiveKey = cache.Subkey(id, "fix.zip") // for .fix.zip file
+ )
// Check the cache; -a forces a rebuild.
if !cfg.BuildA {
c := cache.Default()
- if vcfg.VetxOnly {
- if file, _, err := cache.GetFile(c, vetxKey); err == nil {
- a.built = file
- return nil
+
+ // There may be multiple artifacts in the cache.
+ // We need to retrieve them all, or none:
+ // the effect must be transactional.
+ var (
+ vetxFile string // name of cached .vetx file
+ fixArchive string // name of cached .fix.zip file
+ stdout io.Reader = bytes.NewReader(nil) // cached stdout stream
+ )
+
+ // Obtain location of cached .vetx file.
+ vetxFile, _, err := cache.GetFile(c, id)
+ if err != nil {
+ goto cachemiss
+ }
+
+ // Obtain location of cached .fix.zip file (if needed).
+ if a.needFix {
+ file, _, err := cache.GetFile(c, fixArchiveKey)
+ if err != nil {
+ goto cachemiss
}
- } else {
- // Copy cached vet.std files to stdout.
- if file, _, err := cache.GetFile(c, stdoutKey); err == nil {
- f, err := os.Open(file)
- if err != nil {
- return err
- }
- defer f.Close() // ignore error (can't fail)
- return VetHandleStdout(f)
+ fixArchive = file
+ }
+
+ // Copy cached .stdout file to stdout.
+ if file, _, err := cache.GetFile(c, stdoutKey); err == nil {
+ f, err := os.Open(file)
+ if err != nil {
+ goto cachemiss
}
+ defer f.Close() // ignore error (can't fail)
+ stdout = f
+ }
+
+ // Cache hit: commit transaction.
+ a.built = vetxFile
+ a.FixArchive = fixArchive
+ if err := VetHandleStdout(stdout); err != nil {
+ return err // internal error (don't fall through to cachemiss)
}
+
+ return nil
}
+cachemiss:
js, err := json.MarshalIndent(vcfg, "", "\t")
if err != nil {
return err
}
- // Vet tool succeeded, possibly with facts and JSON stdout. Save both in cache.
+ // Vet tool succeeded, possibly with facts, fixes, or JSON stdout.
+ // Save all in cache.
- // Save facts
+ // Save facts.
if f, err := os.Open(vcfg.VetxOutput); err == nil {
defer f.Close() // ignore error
a.built = vcfg.VetxOutput
- cache.Default().Put(vetxKey, f) // ignore error
+ cache.Default().Put(id, f) // ignore error
+ }
+
+ // Save fix archive (if any).
+ if a.needFix {
+ if f, err := os.Open(vcfg.FixArchive); err == nil {
+ defer f.Close() // ignore error
+ a.FixArchive = vcfg.FixArchive
+ cache.Default().Put(fixArchiveKey, f) // ignore error
+ }
}
// Save stdout.
# Legacy way of selecting fixers is a no-op.
go fix -fix=old1,old2 example.com/x
stderr 'go fix: the -fix=old1,old2 flag is obsolete and has no effect'
+cp x.go.bak x.go
# -c=n flag shows n lines of context.
! go vet -c=2 -printf example.com/x