golang / go

The Go programming language
https://go.dev
BSD 3-Clause "New" or "Revised" License
122.87k stars 17.52k forks source link

dl: add versioned gofmts #26397

Open mvdan opened 6 years ago

mvdan commented 6 years ago

It is well understood that users shouldn't enforce Go source code formatting with go/format. This is documented in its godoc, but also repeated in Go release notes: https://tip.golang.org/doc/go1.10#gofmt

The reason is simple - the formatting that the package produces depends on the Go version in use, so different developers may have different versions of it.

When a project wants to enforce gofmt, what they should instead do is consistently run a specific version of the gofmt binary.

However, it can be difficult for many systems and developers to run exactly the same Go version. Some people may be on an older Go version, some others may run the beta, and some environments may be stuck on older versions of Go.

Currently, it isn't easy for those people to download a specific version of gofmt if they don't have it already. With #23223 implemented they could run go get golang.org/v/go1.10, but that would install much more than just gofmt.

This proposal suggests to add a similar mechanism to install a specific version of gofmt. Ideally via a simple command like go get golang.org/v/gofmt1.10, but a download link would also be an improvement.

Then, one could write a pre-commit hook to enforce gofmt from Go 1.10 as follows:

    gofmt="gofmt"
    if [[ $(go version) != *go1.10* ]]; then
            go get golang.org/v/gofmt1.10
            gofmt="gofmt1.10"
    fi
    # use $gofmt
    $gofmt -l -w ./...

/cc @thaJeztah

mvdan commented 6 years ago

This proposal started off from a discussion in https://github.com/golang/go/issues/26228.

bradfitz commented 6 years ago

This is slightly possible already:

bradfitz@gdev:~/go/src$ go1.8.4 download
Downloaded 0.0% (23422 / 99071672 bytes) ...
Downloaded 59.3% (58753024 / 99071672 bytes) ...
Downloaded 100.0% (99071672 / 99071672 bytes)
Unpacking /home/bradfitz/sdk/go1.8.4/go1.8.4.linux-amd64.tar.gz ...
Success. You may now run 'go1.8.4'

bradfitz@gdev:~/go/src$ go1.8.4 version
go version go1.8.4 linux/amd64

bradfitz@gdev:~/go/src$ ~/sdk/go1.8.4/bin/gofmt -h
usage: gofmt [flags] [path ...]
  -cpuprofile string
        write cpu profile to this file
  -d    display diffs instead of rewriting files
  -e    report all errors (not just the first 10 on different lines)
  -l    list files whose formatting differs from gofmt's
  -r string
        rewrite rule (e.g., 'a[b:len(a)] -> a[b:]')
  -s    simplify code
  -w    write result to (source) file instead of stdout
thaJeztah commented 6 years ago

Thanks for opening the proposal (sorry I didn't reply on your e-mail, I got caught up in other things 😓)

Let me /cc @dnephin @vdemeester here as well for input 👍

mvdan commented 6 years ago

This is slightly possible already

It is, and I believe I noted it in the proposal, but it's not as good of a solution. From my 1.10 install, gofmt seems to weigh under 4MB, while the entirety of 1.10 weighs over 300MB.

If someone needs a development environment on Go version X, I agree that they should install that instead of downloading tools like gofmt and vet separately. However, it's very common for Go projects to build on the latest two major Go versions (and tip), but they still need to be able to enforce a single version of gofmt.

mvdan commented 6 years ago

Here's a slightly different idea: instead of go get golang.org/v/gofmt1.10, piggyback on the existing import paths and do go get golang.org/v/go1.10/cmd/gofmt. That doesn't pollute golang.org/v as much, but it does mean you end up with a genereic gofmt binary which may be confusing.

thaJeztah commented 6 years ago

From my 1.10 install, gofmt seems to weigh under 4MB, while the entirety of 1.10 weighs over 300MB.

I agree that fetching 300MB is a bit much for just gofmt, and 4MB would definitely make it more acceptable.

bradfitz commented 6 years ago

@mvdan, go get golang.org/v/go1.10/cmd/gofmt implies the binary name would be gofmt, precluding having two Go versions' gofmt binaries in your path. It seems you'd want the version number as part of the binary name like we do for go1.11beta1, etc.

mvdan commented 6 years ago

Yes; that's what I meant by "you end up with a generic gofmt binary which may be confusing" :) I don't mind how the proposal ends up being implemented, as long as it's simple to use.

rsc commented 6 years ago

At proposal review we are a bit confused about the use case here. It's no good to use this to reach backward in time and grab older gofmts, since in general new syntaxes can be added to go that those gofmts won't handle (the last I remember was for range x {} as shorthand for for _ = range x{}, or maybe type aliases). So you can't really standardize on, say, Go 1.9 gofmt even though you've moved to Go 1.11. Eventually some syntax addition will come in that the old gofmt can't read.

You could potentially reach into the future and grab the latest gofmt before everyone's dev environments was using it. So maybe you'd force use of Go 1.11 gofmt even if your dev machines were not using Go 1.11. But why are the dev machines on different Go versions?

It's certainly overkill to download 300MB to get a 4MB binary. And we could with some effort maintain a special repo of versioned gofmt commands. But why? How would these get used? What problem would they solve? That's what we don't quite understand.

mvdan commented 6 years ago

I agree that using an older gofmt than the latest Go stable version makes little sense. And I agree that, in general, all developers should be using the latest stable version of Go.

However, there are still a few scenarios where being able to download the latest stable version of gofmt would be useful. I briefly mentioned a couple in my original post, but I'll expand on some that I encountered recently.

I realise that these cases are all about convenience rather than an absolute need. Then again, I imagine that go get golang.org/v/go1.10 is also about convenience, hence why I opened this issue.

Perhaps @thaJeztah or others have other use cases for this.

rsc commented 6 years ago

OK, so it sounds like "making the latest gofmt available to users of earlier Go versions" would suffice for your use case. I think that does make some sense, as long as we document the expected usage the right way.

rsc commented 6 years ago

Talked to @bradfitz and @griesemer. It sounds like we agree with adding subdirectories gofmt1.9, gofmt1.10, etc, to the dl repo, so that you can go get golang.org/x/dl/gofmt1.10, even if you are using Go 1.9. (This would mean that go/parser etc would need to wait two cycles before using new features. They tend to be very plain Go and not change often, so that shouldn't be too onerous. It's less onerous than the compiler requirement (keep working with Go 1.4).)

Each subdirectory would build against its own copies of the relevant libraries so "golang.org/x/dl/gofmt1.10/internal/go/parser", etc. A script in the dl repo would automate copying them out of the Go release, updating imports, and so on. At each release we should only have to run "./addgofmt go1.10".

A README or doc comment (perhaps in each subdirectory) should make clear that it's best to use newer gofmt on old Go source; old gofmt on new Go source is not guaranteed to work and is likely not to.

Any interest in doing this, @mvdan?

mvdan commented 6 years ago

Thanks for the update, and apologies for the late reply - been travelling a bit the past month.

Only having old gofmt binaries available on CI is something that I had to deal with at an old job, but I haven't had that issue for a while. So it isn't a pressing need for me right now - I filed this because I know that others are suffering from similar issues.

Perhaps we could label this issue as "help wanted" and see if someone is willing to put in the work. If noone pops up in a few months, I'd personally be fine with rejecting the proposal - after all, that may be a sign that the issue isn't as widespread as I thought.

bassam commented 6 years ago

is it safe to assume that formatting rules in gofmt will not change on patch releases, and only on major/minor releases if it all?

griesemer commented 6 years ago

@bassam I'd say that is a pretty safe assumption, unless the patch release is fixing a gofmt issue. But in general we only roll out gofmt changes for proper releases (1.10, 1.11, etc.).