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

doc: document changes in behavior of gofmt in 1.11 #26228

Closed
thaJeztah opened this issue Jul 5, 2018 · 11 comments
Closed

doc: document changes in behavior of gofmt in 1.11 #26228

thaJeztah opened this issue Jul 5, 2018 · 11 comments
Labels
Documentation FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@thaJeztah
Copy link
Contributor

Please answer these questions before submitting your issue. Thanks!

What version of Go are you using (go version)?

go version go1.11beta1 linux/amd64

Does this issue reproduce with the latest release?

yes, latest beta

What operating system and processor architecture are you using (go env)?

GOARCH="amd64"
GOBIN=""
GOCACHE="/root/.cache/go-build"
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/go"
GOPROXY=""
GORACE=""
GOROOT="/usr/local/go"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build926389312=/tmp/go-build -gno-record-gcc-switches"
VGOMODROOT=""

What did you do?

Running gofmt in Go 1.11beta1 causes a diff 😄

To reproduce;

  1. Create a file example.go, with the following content;
package main

func main() {
	_ = map[string]string{
		"a": "",
		"123456789012345678": "",
	}
	_ = map[string]string{
		"a": "",
		"1234567890123456789": "",
	}
}
  1. gofmt the file;
gofmt -s -d ./example.go 

What did you expect to see?

On Go 1.10.3, only the first map is re-formatted (the threshold for aligning values lies at 19 characters)

--- example.go.orig	2018-07-05 10:23:56.358365143 +0000
+++ example.go	2018-07-05 10:23:56.358365143 +0000
@@ -2,7 +2,7 @@
 
 func main() {
 	_ = map[string]string{
-		"a": "",
+		"a":                  "",
 		"123456789012345678": "",
 	}
 	_ = map[string]string{

What did you see instead?

On Go 1.11beta1, both maps are re-formatted (the threshold for aligning values lies at 18 characters):

--- example.go.orig	2018-07-05 10:24:32.709844185 +0000
+++ example.go	2018-07-05 10:24:32.709844185 +0000
@@ -2,11 +2,11 @@
 
 func main() {
 	_ = map[string]string{
-		"a": "",
+		"a":                  "",
 		"123456789012345678": "",
 	}
 	_ = map[string]string{
-		"a": "",
+		"a":                   "",
 		"1234567890123456789": "",
 	}
 }

This change complicates projects that test against multiple versions of Go (see related issues moby/moby#37358, containerd/containerd#2436)

If this was a deliberate change, please add a mention of this in the changelog 🤗

@mvdan
Copy link
Member

mvdan commented Jul 5, 2018

This change was done on purpose in 542ea5a.

As to why gofmt changes every release - see @griesemer's comment in #25161 (comment).

However, I agree that this change should be documented in the release notes. I can't see it in https://tip.golang.org/doc/go1.11.

@mvdan mvdan changed the title gofmt: threshold for aligning values changed in Go 1.11beta1 doc: document changes in behavior of gofmt in 1.11 Jul 5, 2018
@mvdan mvdan added this to the Go1.11 milestone Jul 5, 2018
@mvdan mvdan added the NeedsFix The path to resolution is known, but the work has not been done. label Jul 5, 2018
@thaJeztah
Copy link
Contributor Author

Oh! actually found that commit, but see I forgot to include it in my description (thanks!)

hm, alright, so I understand Go should not stop progression; just thinking what the best approach is for projects that test against, and support multiple go versions;

  • for CI; exclude gofmt check on version X
  • more problematic; accepting contributions from users running different versions of Go 😕 😅

@mvdan
Copy link
Member

mvdan commented Jul 5, 2018

Yes - if a project wants to enforce gofmt via CI, it should stick to exactly one version of gofmt. That tends to be the latest stable version, such as 1.10 now.

The other option is to not enforce it via CI, and instead enforce it via pre-commit checks and by humans, like Go itself does.

@thaJeztah
Copy link
Contributor Author

True; just that, pre-commit checks won't resolve accepting contributions from users running different versions of go. In an ideal world, every project would switch to (and be compatible with) latest stable Go the moment it's released. Although upgrading Go versions has become less painful over the years, but we're not there yet, so unfortunately this isn't always possible (hence, contributors pinning to different versions of Go).

Thanks for your suggestions; it's appreciated 👍

@mvdan
Copy link
Member

mvdan commented Jul 5, 2018

I agree that enforcing this server-side instead of client-side is better. Perhaps it should be made easier for developers to download specific versions of gofmt, so that they could use gofmt1.10 for work projects even if they were using 1.9 or 1.11beta1 (master) on their laptop.

gofmt could even include a version flag, so that scripts could check if the right version is being used. Although it might be a bad idea for the gofmt that ships with Go itself to report a version, equal but separate from go version.

#22695 didn't seem to reach any conclusion that would help with this. I might open a proposal issue along the lines of what I described above, once this documentation issue is fixed.

@thaJeztah
Copy link
Contributor Author

thaJeztah commented Jul 5, 2018

gofmt could even include a version flag, so that scripts could check if the right version is being used. Although it might be a bad idea for the gofmt that ships with Go itself to report a version, equal but separate from go version.

Yes, while writing my previous comment, I wanted to suggest a "version" option, so that a different gofmt could be used to ease a project's transition between Go versions. I decided not to, because doing so may defeat the whole purpose of gofmt (end all discussions around code-style/formatting).

Having said the above, perhaps there's still merit in that option; introduce an option to select a "slow" version of gofmt, limited to versions of Go that are actively maintained;

For example, currently, Go 1.11 (next stable), 1.10.x (current stable), and 1.9.x (previous stable) are actively maintained; I think it's ok to assume (most) projects that support multiple Go versions won't Go beyond those versions

Could look something like this; (gofmt [--version=<option>]);

Option Description
(not set) default; use gofmt of the installed version
stable use current stable; same as default under normal circumstances, but can be useful when running go tip or a beta
previous-stable use previous stable; useful during migration between versions, and if a project supports both "current" and "previous" stable

edit: removed --version=current option from the table, as it didn't make much sense

@mvdan
Copy link
Member

mvdan commented Jul 5, 2018

When I mentioned a version flag, I meant just a flag to print its version :) Then, scripts could do something like:

gofmt="gofmt"
if [[ $(gofmt -version) != go1.10.* ]]; then
    # download gofmt1.10
    gofmt="gofmt1.10"
fi
# use $gofmt

I'll send a proposal about this in a few days. @thaJeztah if you want, I can email you a draft before publishing.

@thaJeztah
Copy link
Contributor Author

Looks like that would indeed help 👍 You can /cc me at sebastiaan <swirly-thingy> docker.com

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/122295 mentions this issue: doc: explain minor change to gofmt in go1.11

@kortschak
Copy link
Contributor

@mvdan

The other option is to not enforce it via CI, and instead enforce it via pre-commit checks and by humans, like Go itself does.

Yes, Russ has made this comment too in the past. Pre-commit checks don't work well in practice unless there is either a requirement that contributors run a Makefile to install the hooks or there is a bespoke codereview tool as exists for Go contributions. These are not necessarily feasible for small open source projects (a good example is Gonum where we already have difficulty managing new and drive-by contributions).

I'll just go and eat my brioche.

@mvdan
Copy link
Member

mvdan commented Sep 9, 2018

@kortschak please note that this issue is closed, and that I filed a proposal precisely to make enforcing gofmt easier - #26397.

rodrigc pushed a commit to libopenstorage/openstorage that referenced this issue Mar 28, 2019
go 1.11's "go fmt" has a backwards incompatible formatting
change due to golang/go#26228
which only affects the csi/csi_test.go file.

This is a temporary workaround for difference in go fmt between
1.9/1.10 and 1.11+, to fix travis.  We can remove this when we drop
go 1.9 and go 1.10 from travis.

Signed-off-by: Craig Rodrigues <[email protected]>
rodrigc pushed a commit to libopenstorage/openstorage that referenced this issue Apr 4, 2019
go 1.11's "go fmt" has a backwards incompatible formatting
change due to golang/go#26228
which only affects the csi/csi_test.go file.

This is a temporary workaround for difference in go fmt between
1.9/1.10 and 1.11+, to fix travis.  We can remove this when we drop
go 1.9 and go 1.10 from travis.

Signed-off-by: Craig Rodrigues <[email protected]>
rodrigc pushed a commit to libopenstorage/openstorage that referenced this issue Apr 15, 2019
go 1.11's "go fmt" has a backwards incompatible formatting
change due to golang/go#26228
which only affects the csi/csi_test.go file.

This is a temporary workaround for difference in go fmt between
1.9/1.10 and 1.11+, to fix travis.  We can remove this when we drop
go 1.9 and go 1.10 from travis.

Signed-off-by: Craig Rodrigues <[email protected]>
@golang golang locked and limited conversation to collaborators Sep 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Documentation FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

4 participants