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/tools/cmd/goimports: return zero exit code if an offense was resolved #39316

Closed
jthurman42 opened this issue May 29, 2020 · 8 comments
Closed

x/tools/cmd/goimports: return zero exit code if an offense was resolved #39316

jthurman42 opened this issue May 29, 2020 · 8 comments

Comments

@jthurman42
Copy link

@jthurman42 jthurman42 commented May 29, 2020

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

$ go version
go version go1.14.3 darwin/amd64

Does this issue reproduce with the latest release?

Yes

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

go env Output
$ go env
GO111MODULE="on"
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/jthurman/Library/Caches/go-build"
GOENV="/Users/jthurman/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GOOS="darwin"
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/Cellar/go/1.14.3/libexec"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/Cellar/go/1.14.3/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
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 -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/t1/snd3btxx59sfx8f3_6c_wpk40000gn/T/go-build703369712=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

As part of our make process, we allow goimports to automatically fix import formatting issues. The behavior changed with #39032 in an unexpected way. Returning non-zero makes perfect sense except when the imports were automatically resolved.

# Makefile snippet:

GO ?= go
SRCDIR ?= .
GO_FILES ?= $(shell find $(SRCDIR) -type f -name "*.go" | grep -v -e ".git/" -e '/vendor/' -e '/example/')
PROJECT_MODULE  ?= $(shell $(GO) list -m)

goimports:
      @$(GOIMPORTS) -l -w -local $(PROJECT_MODULE) $(GO_FILES)

What did you expect to see?

List of modified files, build process continues

What did you see instead?

List of modified files, build process terminates (exit code 1 from goimports)

@gopherbot gopherbot added the Tools label May 29, 2020
@gopherbot gopherbot added this to the Unreleased milestone May 29, 2020
jthurman42 added a commit to jthurman42/tools that referenced this issue May 29, 2020
Return zero if goimports was successfully able to modify files in place
even if -l is set to list the changed files.

Fixes: golang/go#39316
@gopherbot
Copy link

@gopherbot gopherbot commented May 29, 2020

Change https://golang.org/cl/235526 mentions this issue: cmd/goimports: return exit code 0 when both -l and -w succeed

@dmitshur
Copy link
Member

@dmitshur dmitshur commented Jun 2, 2020

@dramich
Copy link

@dramich dramich commented Jun 9, 2020

Can this be put behind a new flag? This is a breaking change and not an intuitive one to figure out what happened.

@jthurman42
Copy link
Author

@jthurman42 jthurman42 commented Jun 9, 2020

This reverts a breaking change (returning non-zero on -l broke the existing workflow of -l -w)

@dramich
Copy link

@dramich dramich commented Jun 9, 2020

@jthurman42 I understand, my comment wasn't clear. The pr should be reverted OR put behind a flag as an option vs being the default behavior of -l

@jthurman42
Copy link
Author

@jthurman42 jthurman42 commented Jun 9, 2020

@dramich Ah gotcha. You'll want to comment here: https://go-review.googlesource.com/c/tools/+/235526/ as the Github issues do not appear to be monitored (and do not sync to that thread 😢 )

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Jun 10, 2020

On the contrary, GitHub issues are the canonical locations for discussion and decisions. The code review comments are for discussion of the code itself primarily.

@heschik
Copy link
Contributor

@heschik heschik commented Jun 10, 2020

Given the back and forth on this issue I'm concluding that changing the behavior in any way is too much. I'll roll back the original CL. If someone wants to do an analysis of the various use cases and propose a backwards-compatible flag, we can discuss that. But overall it seems like anyone who cares can just run goimports twice.

Sorry for the trouble and delay.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

6 participants
You can’t perform that action at this time.