Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

x/dl: add versioned gofmts #26397

Open
mvdan opened this issue Jul 16, 2018 · 15 comments

Comments

@mvdan
Copy link
Member

commented Jul 16, 2018

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

@gopherbot gopherbot added this to the Proposal milestone Jul 16, 2018

@gopherbot gopherbot added the Proposal label Jul 16, 2018

@mvdan

This comment has been minimized.

Copy link
Member Author

commented Jul 16, 2018

This proposal started off from a discussion in #26228.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Jul 16, 2018

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

This comment has been minimized.

Copy link
Contributor

commented Jul 17, 2018

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

This comment has been minimized.

Copy link
Member Author

commented Jul 17, 2018

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

This comment has been minimized.

Copy link
Member Author

commented Jul 18, 2018

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

This comment has been minimized.

Copy link
Contributor

commented Jul 18, 2018

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

This comment has been minimized.

Copy link
Member

commented Jul 18, 2018

@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

This comment has been minimized.

Copy link
Member Author

commented Jul 18, 2018

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

This comment has been minimized.

Copy link
Contributor

commented Aug 6, 2018

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.

@rsc rsc changed the title Proposal: allow go-getting specific gofmt versions proposal: cmd/gofmt: allow go get access to versioned gofmts Aug 6, 2018

@mvdan

This comment has been minimized.

Copy link
Member Author

commented Aug 7, 2018

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.

  • A project supports Go 1.9.x and 1.10.x, and some CI system does not support 1.10.x yet. This tends to happen if 1.10 came out recently, as it may take a few weeks for it to be commonplace. The only current option is to not enforce gofmt on that 1.9.x environment, or to enforce 1.9's gofmt, which is worse.

  • A project supports Go 1.9.x and 1.10.x, and the user prefers sticking to 1.9.x as packaged by their distribution or operating system. These can lag a few weeks or even months behind. Developing on 1.9 should still be possible, but it's not feasible without the correct gofmt.

  • A developer has go pointing at the master version. I myself have this setup, with go1 being an alias to the latest stable. This way, gofmt is from master, and thus may differ from its stable version. I still have the stable version installed, but a script wanting to enforce gofmt wouldn't be able to find it automatically. The script being able to download gofmt on its own could help.

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

This comment has been minimized.

Copy link
Contributor

commented Aug 14, 2018

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

This comment has been minimized.

Copy link
Contributor

commented Aug 20, 2018

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?

@rsc rsc modified the milestones: Proposal, Unreleased Aug 20, 2018

@rsc rsc changed the title proposal: cmd/gofmt: allow go get access to versioned gofmts x/dl: add versioned gofmts Aug 20, 2018

@mvdan

This comment has been minimized.

Copy link
Member Author

commented Sep 9, 2018

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 bassam referenced this issue Sep 10, 2018
2 of 6 tasks complete
@bassam

This comment has been minimized.

Copy link

commented Sep 10, 2018

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

This comment has been minimized.

Copy link
Contributor

commented Sep 10, 2018

@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.).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.