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
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone

Comments

@jthurman42
Copy link

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 This label describes issues relating to any tools in the x/tools repository. label May 29, 2020
@gopherbot gopherbot added this to the Unreleased milestone May 29, 2020
jthurman42 pushed 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

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

@dmitshur dmitshur added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jun 2, 2020
@dmitshur
Copy link
Contributor

dmitshur commented Jun 2, 2020

/cc @heschik @bradfitz @khos2ow

@dramich
Copy link

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

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

@dramich
Copy link

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 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

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.

@heschi
Copy link
Contributor

heschi 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 subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants