From 70d7433507d8d92bfa78a923e1f48de9b9e17203 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Mart=C3=AD?= Date: Wed, 27 Jul 2022 16:56:24 +0100 Subject: [PATCH] revamp //gofumpt:diagnose for real use cases Right now, it simply reports "(devel)" as the gofumpt version when gofumpt is used as a library via the format package, which is the case for many users like golangci-lint. To properly test this behavior, we add a gofumpt-external program which simply formats stdin with one option and prints to stdout. We `go install` this tool, which depends on a released gofumpt version. It also does not report the Go version, which is important as we rely on packages like go/printer, which can influence our behavior. Similar to the case above, we `go install` a released gofumpt version. It also does not use VCS information when it is the main module; add that as well, along with a test, borrowed from mvdan.cc/garble. While here, make the output easier to read by adding "version" and "flags" words as descriptions. Finally, we remove version.Print, which was an unnecessary wrapper. The following PR will update the released version of gofumpt being used, meaning that the "released" and "external" tests will pick up the improvement. --- format/format.go | 2 + gofmt.go | 2 +- internal/version/version.go | 97 ++++++++++++++++++++++++++----- main_test.go | 24 +++++++- testdata/gofumpt-external/go.mod | 11 ++++ testdata/gofumpt-external/go.sum | 13 +++++ testdata/gofumpt-external/main.go | 22 +++++++ testdata/script/diagnose.txtar | 78 ++++++++++++++++++++----- 8 files changed, 219 insertions(+), 30 deletions(-) create mode 100644 testdata/gofumpt-external/go.mod create mode 100644 testdata/gofumpt-external/go.sum create mode 100644 testdata/gofumpt-external/main.go diff --git a/format/format.go b/format/format.go index 00c8e2d..fa4f2fc 100644 --- a/format/format.go +++ b/format/format.go @@ -394,7 +394,9 @@ func (f *fumpter) applyPre(c *astutil.Cursor) { if comment.Text == "//gofumpt:diagnose" || strings.HasPrefix(comment.Text, "//gofumpt:diagnose ") { slc := []string{ "//gofumpt:diagnose", + "version:", version.String(), + "flags:", "-lang=" + f.LangVersion, "-modpath=" + f.ModulePath, } diff --git a/gofmt.go b/gofmt.go index e0210a8..d2a6290 100644 --- a/gofmt.go +++ b/gofmt.go @@ -463,7 +463,7 @@ func gofmtMain(s *sequencer) { // Print the gofumpt version if the user asks for it. if *showVersion { - version.Print() + fmt.Println(version.String()) return } diff --git a/internal/version/version.go b/internal/version/version.go index 3ec8830..9929304 100644 --- a/internal/version/version.go +++ b/internal/version/version.go @@ -4,28 +4,99 @@ package version import ( + "encoding/json" "fmt" "os" + "runtime" "runtime/debug" + "time" + + "golang.org/x/mod/module" ) -var version = "(devel)" // to match the default from runtime/debug +// Note that this is not a main package, so a "var version" will not work with +// our go-cross script which uses -ldflags=main.version=xxx. -func String() string { - if testVersion := os.Getenv("GOFUMPT_VERSION_TEST"); testVersion != "" { - return testVersion +const ourModulePath = "mvdan.cc/gofumpt" + +const fallbackVersion = "(devel)" // to match the default from runtime/debug + +func findModule(info *debug.BuildInfo, modulePath string) *debug.Module { + if info.Main.Path == modulePath { + return &info.Main } - // don't overwrite the version if it was set by -ldflags=-X - if info, ok := debug.ReadBuildInfo(); ok && version == "(devel)" { - mod := &info.Main - if mod.Replace != nil { - mod = mod.Replace + for _, dep := range info.Deps { + if dep.Path == modulePath { + return dep } - version = mod.Version } - return version + return nil } -func Print() { - fmt.Println(String()) +func gofumptVersion() string { + info, ok := debug.ReadBuildInfo() + if !ok { + return fallbackVersion // no build info available + } + // Note that gofumpt may be used as a library via the format package, + // so we cannot assume it is the main module in the build. + mod := findModule(info, ourModulePath) + if mod == nil { + return fallbackVersion // not found? + } + if mod.Replace != nil { + mod = mod.Replace + } + + // If we found a meaningful version, we are done. + // If gofumpt is not the main module, stop as well, + // as VCS info is only for the main module. + if mod.Version != "(devel)" || mod != &info.Main { + return mod.Version + } + + // Fall back to trying to use VCS information. + // Until https://github.com/golang/go/issues/50603 is implemented, + // manually construct something like a pseudo-version. + // TODO: remove when this code is dead, hopefully in Go 1.20. + + // For the tests, as we don't want the VCS information to change over time. + if v := os.Getenv("GARBLE_TEST_BUILDSETTINGS"); v != "" { + var extra []debug.BuildSetting + if err := json.Unmarshal([]byte(v), &extra); err != nil { + panic(err) + } + info.Settings = append(info.Settings, extra...) + } + + var vcsTime time.Time + var vcsRevision string + for _, setting := range info.Settings { + switch setting.Key { + case "vcs.time": + // If the format is invalid, we'll print a zero timestamp. + vcsTime, _ = time.Parse(time.RFC3339Nano, setting.Value) + case "vcs.revision": + vcsRevision = setting.Value + if len(vcsRevision) > 12 { + vcsRevision = vcsRevision[:12] + } + } + } + if vcsRevision != "" { + return module.PseudoVersion("", "", vcsTime, vcsRevision) + } + return fallbackVersion +} + +func goVersion() string { + // For the tests, as we don't want the Go version to change over time. + if testVersion := os.Getenv("GO_VERSION_TEST"); testVersion != "" { + return testVersion + } + return runtime.Version() +} + +func String() string { + return fmt.Sprintf("%s (%s)", gofumptVersion(), goVersion()) } diff --git a/main_test.go b/main_test.go index cae92eb..06cea00 100644 --- a/main_test.go +++ b/main_test.go @@ -4,12 +4,14 @@ package main import ( + "encoding/json" "flag" "os" "path/filepath" "testing" qt "github.com/frankban/quicktest" + exec "golang.org/x/sys/execabs" "github.com/rogpeppe/go-internal/gotooltest" "github.com/rogpeppe/go-internal/testscript" @@ -25,11 +27,31 @@ var update = flag.Bool("u", false, "update testscript output files") func TestScripts(t *testing.T) { t.Parallel() + + var goEnv struct { + GOCACHE string + GOMODCACHE string + GOMOD string + } + out, err := exec.Command("go", "env", "-json").CombinedOutput() + if err != nil { + t.Fatal(err) + } + if err := json.Unmarshal(out, &goEnv); err != nil { + t.Fatal(err) + } + p := testscript.Params{ Dir: filepath.Join("testdata", "script"), UpdateScripts: *update, + Setup: func(env *testscript.Env) error { + env.Setenv("GOCACHE", goEnv.GOCACHE) + env.Setenv("GOMODCACHE", goEnv.GOMODCACHE) + env.Setenv("GOMOD_DIR", filepath.Dir(goEnv.GOMOD)) + return nil + }, } - err := gotooltest.Setup(&p) + err = gotooltest.Setup(&p) qt.Assert(t, err, qt.IsNil) testscript.Run(t, p) } diff --git a/testdata/gofumpt-external/go.mod b/testdata/gofumpt-external/go.mod new file mode 100644 index 0000000..5d0976a --- /dev/null +++ b/testdata/gofumpt-external/go.mod @@ -0,0 +1,11 @@ +module test/gofumpt-external + +go 1.18 + +require mvdan.cc/gofumpt v0.3.2-0.20220627183521-8dda8068d9f3 + +require ( + github.com/google/go-cmp v0.5.8 // indirect + golang.org/x/mod v0.6.0-dev.0.20220419223038-86c51ed26bb4 // indirect + golang.org/x/tools v0.1.11 // indirect +) diff --git a/testdata/gofumpt-external/go.sum b/testdata/gofumpt-external/go.sum new file mode 100644 index 0000000..da5c2a6 --- /dev/null +++ b/testdata/gofumpt-external/go.sum @@ -0,0 +1,13 @@ +github.com/frankban/quicktest v1.14.3 h1:FJKSZTDHjyhriyC81FLQ0LY93eSai0ZyR/ZIkd3ZUKE= +github.com/google/go-cmp v0.5.8 h1:e6P7q2lk1O+qJJb4BtCQXlK8vWEO8V1ZeuEdJNOqZyg= +github.com/google/go-cmp v0.5.8/go.mod h1:17dUlkBOakJ0+DkrSSNjCkIjxS6bF9zb3elmeNGIjoY= +github.com/kr/pretty v0.3.0 h1:WgNl7dwNpEZ6jJ9k1snq4pZsg7DOEN8hP9Xw0Tsjwk0= +github.com/kr/text v0.2.0 h1:5Nx0Ya0ZqY2ygV366QzturHI13Jq95ApcVaJBhpS+AY= +github.com/rogpeppe/go-internal v1.8.2-0.20220624104257-af73bbc5c731 h1:LkP6LNQyXrQoVVXMpb+sbb0iibcaOZH97feeh778heA= +golang.org/x/mod v0.6.0-dev.0.20220419223038-86c51ed26bb4 h1:6zppjxzCulZykYSLyVDYbneBfbaBIQPYMevg0bEwv2s= +golang.org/x/mod v0.6.0-dev.0.20220419223038-86c51ed26bb4/go.mod h1:jJ57K6gSWd91VN4djpZkiMVwK6gcyfeH4XE8wZrZaV4= +golang.org/x/sys v0.0.0-20220624220833-87e55d714810 h1:rHZQSjJdAI4Xf5Qzeh2bBc5YJIkPFVM6oDtMFYmgws0= +golang.org/x/tools v0.1.11 h1:loJ25fNOEhSXfHrpoGj91eCUThwdNX6u24rO1xnNteY= +golang.org/x/tools v0.1.11/go.mod h1:SgwaegtQh8clINPpECJMqnxLv9I09HLqnW3RMqW0CA4= +mvdan.cc/gofumpt v0.3.2-0.20220627183521-8dda8068d9f3 h1:Kk6yeaipDobigAr+NZqq37LjLN1q3LHSOpcaDLDwyoI= +mvdan.cc/gofumpt v0.3.2-0.20220627183521-8dda8068d9f3/go.mod h1:TSc7K1qXnyCCK7LUmDAWp4UMntOys3CzN8ksMKaxFrE= diff --git a/testdata/gofumpt-external/main.go b/testdata/gofumpt-external/main.go new file mode 100644 index 0000000..22058e6 --- /dev/null +++ b/testdata/gofumpt-external/main.go @@ -0,0 +1,22 @@ +package main + +import ( + "io" + "os" + + "mvdan.cc/gofumpt/format" +) + +func main() { + orig, err := io.ReadAll(os.Stdin) + if err != nil { + panic(err) + } + formatted, err := format.Source(orig, format.Options{ + LangVersion: "v1.16", + }) + if err != nil { + panic(err) + } + os.Stdout.Write(formatted) +} diff --git a/testdata/script/diagnose.txtar b/testdata/script/diagnose.txtar index 52f31c1..e2d6549 100644 --- a/testdata/script/diagnose.txtar +++ b/testdata/script/diagnose.txtar @@ -1,19 +1,21 @@ -cp foo.go foo.go.orig -env GOFUMPT_VERSION_TEST=v0.1.1-0.20210524140936-ba6406b58f36 +env GO_VERSION_TEST=go1.18.29 -gofumpt -w foo.go -cmp foo.go foo.go.golden +# First, test a local build of gofumpt resulting from 'git clone'. +# Its version will be inferred from VCS, but since we want a stable test, +# we mock the VCS information. Note that test binaries do not have VCS info. +# Data obtained from a real build while developing. +env GARBLE_TEST_BUILDSETTINGS='[{"Key":"vcs","Value":"git"},{"Key":"vcs.revision","Value":"8dda8068d9f339047fc1777b688afb66a0a0db17"},{"Key":"vcs.time","Value":"2022-07-27T15:58:40Z"},{"Key":"vcs.modified","Value":"true"}]' +gofumpt foo.go +cmp stdout foo.go.golden -gofumpt -w outdated.go -cmp outdated.go foo.go.golden +gofumpt outdated.go +cmp stdout foo.go.golden -cp foo.go.orig foo.go -gofumpt -w -extra foo.go -cmp foo.go foo.go.golden-extra +gofumpt -extra foo.go +cmp stdout foo.go.golden-extra -cp foo.go.orig foo.go -gofumpt -w -lang=v1 foo.go -cmp foo.go foo.go.golden-lang +gofumpt -lang=v1 foo.go +cmp stdout foo.go.golden-lang gofumpt -d nochange.go ! stdout . @@ -24,6 +26,40 @@ gofumpt -d foo.go.golden gofumpt -d -extra foo.go.golden-extra ! stdout . +# A local build without VCS information will result in a missing version. +env GARBLE_TEST_BUILDSETTINGS='[]' +gofumpt foo.go +cmp stdout foo.go.golden-devel + +[short] stop 'the rest of this test builds gofumpt binaries' + +# We want a published version of gofumpt on the public module proxies, +# because that's the only way that its module version will be included. +# Using a directory replace directive will not work. +# This means that any change in how gofumpt reports its own version +# will require two pull requests, the second one updating the test script. +# We could consider using go-internal/goproxytest, but then we would need to +# manually run something like go-internal/cmd/txtar-addmod reguarly. +# Or teach goproxytest to serve a mock version of gofumpt from its local checkout. +# Either way, both are relatively overkill for now. +env GOBIN=${WORK}/bin +env GOFUMPT_PUBLISHED_VERSION=v0.3.2-0.20220627183521-8dda8068d9f3 + +# TODO: update these once the library fix hits master + +# gofumpt as the main binary with a real module version. +go install mvdan.cc/gofumpt@${GOFUMPT_PUBLISHED_VERSION} +exec ${GOBIN}/gofumpt foo.go +cmp stdout foo.go.golden-released + +# gofumpt as a library with a real module version. +cd ${GOMOD_DIR}/testdata/gofumpt-external +go install . +cd ${WORK} +stdin foo.go +exec ${GOBIN}/gofumpt-external +cmp stdout foo.go.golden-external + -- go.mod -- module test @@ -43,12 +79,24 @@ package p -- foo.go.golden -- package p -//gofumpt:diagnose v0.1.1-0.20210524140936-ba6406b58f36 -lang=v1.16 -modpath=test +//gofumpt:diagnose version: v0.0.0-20220727155840-8dda8068d9f3 (go1.18.29) flags: -lang=v1.16 -modpath=test +-- foo.go.golden-devel -- +package p + +//gofumpt:diagnose version: (devel) (go1.18.29) flags: -lang=v1.16 -modpath=test -- foo.go.golden-extra -- package p -//gofumpt:diagnose v0.1.1-0.20210524140936-ba6406b58f36 -lang=v1.16 -modpath=test -extra +//gofumpt:diagnose version: v0.0.0-20220727155840-8dda8068d9f3 (go1.18.29) flags: -lang=v1.16 -modpath=test -extra -- foo.go.golden-lang -- package p -//gofumpt:diagnose v0.1.1-0.20210524140936-ba6406b58f36 -lang=v1 -modpath=test +//gofumpt:diagnose version: v0.0.0-20220727155840-8dda8068d9f3 (go1.18.29) flags: -lang=v1 -modpath=test +-- foo.go.golden-released -- +package p + +//gofumpt:diagnose v0.3.2-0.20220627183521-8dda8068d9f3 -lang=v1.16 -modpath=test +-- foo.go.golden-external -- +package p + +//gofumpt:diagnose (devel) -lang=v1.16 -modpath=