Fix build issues from Makefile, cleanup

[kfsone]

(Siloing unrelated changes from https://github.com/goccmack/gocc/pull/110)

• Moved goimports target into regenerate for consistency with ci requirements,
• make ci adds a text representation of the error condition to the exit code,
• make ci additionally runs git diff after regenerate for more readable early out,
• removed seeming merge-artifact, main.go.orig invocation after
• Made 'goimports' target part of 'regenerate' so that 'ci' does not fail builds because someone forgot to manually run goimports after regenerate,

Re: added additional git-diff early out from make ci

Intended to make the workflow easier to understand for PR submission, since the last line of output will clearly explain why the diff is being rejected, and does so before the make test because make test contains a lot of false-positive error messages (tests that verify errors produce errors).

[awalterschulze]

Looks good, can you also explain what -l does for goimports and why two files were deleted?

[kfsone]

editing commit/pr description.

go.sum was a force of habit; it's actually a build artifact that seems designed to accompany release artifacts. Including it is recommended: https://github.com/golang/go/wiki/Modules#should-i-commit-my-gosum-file-as-well-as-my-gomod-file

Given that, unlike say python requirements.txt, it's not a lock file, what it does is produce diffs...

In order to not have golangci-lint reject pull requests because go.sum changed, the user workflow needs to be:

write ...... test ...... fix ...... test ...... lint ...... rush{go mod tidy/go build/git commit go.sum -m "go.sum update"/git push}

and then hope for consistency between their local go cache and the github runner's cache :)

[kfsone]

... just like that :) sasssafrass

[kfsone]

Ah, the problem is the errcheck action is fetching an external package and so modifying the go.mod and go.sum. I wasn't manually running that step.

[kfsone]

@goccmack I'm confused as to how the make lint step is even invoked? It doesn't appear to be in the .github/workflows/build.yaml and it's not a dependency of anything in the Makefile?

By not running it, my commits always end up differing from the commit in the ci environment by go.mod and go.sum.

[kfsone]

Also, should the got vet invocation be go vet -methods=false ./... instead of just .?

[goccmack]

@goccmack I'm confused as to how the make lint step is even invoked? It doesn't appear to be in the .github/workflows/build.yaml and it's not a dependency of anything in the Makefile?

By not running it, my commits always end up differing from the commit in the ci environment by go.mod and go.sum.

@awalterschulze Will you please answer this?

[awalterschulze]

lows/build.yaml and it's not a dependency of anything in the M

If lint is not invoked in build.yaml then we should probably add it

[awalterschulze]

go vet -methods=false ./...

It has been a while, but I think we would like to ./..., but it is -methods=false, then it is probably there, because have unresolved issues and don't want to regress what we have fixed so far.

[awalterschulze]

So feel free to increase the strictness if possible, but if changes become too many, consider splitting over a few pull requests.

[kfsone]

lows/build.yaml and it's not a dependency of anything in the M

If lint is not invoked in build.yaml then we should probably add it

It's not explicitly invoked, but somehow it runs... That's what I've been getting caught out by.

Prior to my PRs, I run:

 $ git clean -f -d && go mod tidy && make regenerate && make goimports && make test && golangci-lint run
 $ git status | grep -q modified: || echo >&2 "ERROR: outstanding files"

But somehow the ci build is invoking make lint and causing go.mod/go.sum to change:

(long outputs redirected to gists to make this readable, links are shown)

oliver@spud:gocc$ (go mod tidy && make regenerate goimports test && golangci-lint run) |& gist-paste
https://gist.github.com/6f6d57ad7dbf37045e226d014a3681aa

oliver@spud:gocc$ git diff go.mod go.sum
diff --git a/go.mod b/go.mod
index 298bbfc..550b356 100644
--- a/go.mod
+++ b/go.mod
@@ -3,7 +3,6 @@ module github.com/goccmack/gocc
 go 1.12

 require (
-       github.com/kisielk/errcheck v1.2.0 // indirect
        golang.org/x/mod v0.3.0
        golang.org/x/tools v0.1.0 // indirect
 )
diff --git a/go.sum b/go.sum
index 4e59810..dd8cbcb 100644
--- a/go.sum
+++ b/go.sum
@@ -1,5 +1,3 @@
-github.com/kisielk/errcheck v1.2.0 h1:reN85Pxc5larApoH1keMBiu2GWtPqXQ1nc9gx+jOU+E=
-github.com/kisielk/errcheck v1.2.0/go.mod h1:/BMXB+zMLi60iA8Vv6Ksmxu/1UDYcXs4uQLJ+jE2L00=
 github.com/yuin/goldmark v1.2.1/go.mod h1:3hX8gzYuyVAZsxl0MRgGTJEmQBFcNTphYh9decYSb74=
 golang.org/x/crypto v0.0.0-20190308221718-c2843e01d9a2/go.mod h1:djNgcEr1/C05ACkg1iLfiJU5Ep61QUkGW8qpdssI0+w=
 golang.org/x/crypto v0.0.0-20191011191535-87dc89f01550/go.mod h1:yigFU9vqHzYiE8UmvKecakEJjdnWj3jj499lnFckfCI=
@@ -19,7 +17,6 @@ golang.org/x/sys v0.0.0-20210119212857-b64e53b001e4/go.mod h1:h1NjWce9XRLGQEsW7w
 golang.org/x/text v0.3.0/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ=
 golang.org/x/text v0.3.3/go.mod h1:5Zoc/QRtKVWzQhOtBMvqHzDpF6irO9z98xDceosuGiQ=
 golang.org/x/tools v0.0.0-20180917221912-90fa682c2a6e/go.mod h1:n7NCudcB/nEzxVGmLbDWY5pfWTLqBcC2KZ6jyYvM4mQ=
-golang.org/x/tools v0.0.0-20181030221726-6c7e314b6563/go.mod h1:n7NCudcB/nEzxVGmLbDWY5pfWTLqBcC2KZ6jyYvM4mQ=
 golang.org/x/tools v0.0.0-20191119224855-298f0cb1881e h1:aZzprAO9/8oim3qStq3wc1Xuxx4QmAGriC4VU4ojemQ=
 golang.org/x/tools v0.0.0-20191119224855-298f0cb1881e/go.mod h1:b+2E5dAYhXwXZwtnZ6UAqBI28+e2cm9otk0dWdXHAEo=
 golang.org/x/tools v0.1.0 h1:po9/4sTYwZU9lPhi1tOrb4hCv3qrhiQ77LZfGa2OjwY=

oliver@spud:gocc$ make lint |& gist-paste
https://gist.github.com/10320268448f70e0a0391d7bb7e09480

oliver@spud:gocc$ git diff go.mod go.sum

oliver@spud:gocc$

[kfsone]

• go vet ./... seems to work fine,
• I'm adding a "make check" because during my iteration cycle, I like to know what I did wrong rather than what I did right (humor: cause I assume that's everything? /humor)

[awalterschulze]

That is pretty weird. Sorry for the inconvenience. I am a bit at a loss for how this is happening. Ideally we want make ci to work the same locally as on the github action, to make it easy for people to contribute. Whatever you can do to make it that easy, I will support. Sorry to not be of more help.

[kfsone]

[edit: not goimports, but lint] Ok, that explains why goimports is not part of make regenerate - it doesn't work under go 1.12. It doesn't look like 'go fmt' will mess with goimports formatted code, so ... maybe we just tell make goimports do nothing if go version is 1.12?

[kfsone]

/work/gocc/gocc/go/src/github.com/goccmack/gocc'
go vet ./...
go get github.com/kisielk/errcheck@v1.2.0
go get: warning: modules disabled by GO111MODULE=auto in GOPATH/src;
    ignoring go.mod;
    see 'go help modules'
go: cannot use path@version syntax in GOPATH mode

[kfsone]

option 1: I can exempt errcheck from go 1.12:

errcheck:
    @# can't use path@version with go 1.12, so skip errcheck entirely in go 1.12
    @if [ -n "$(go version | grep 'go1.12')" ]; then    \
        go get github.com/kisielk/errcheck@v1.2.0; \
        errcheck -exclude .errcheck-ignore ./...; \
    else \
        echo "-- skipping errcheck on go1.12"; \
    fi

problem: this will cause the go.mod and go.sum to be different,

Option 2:

Rely on the err checking in golangci-lint run?

[kfsone]

(base) oliver@Spud:/mnt/c/Users/oliver/go/src/github.com/kfsone/gocc$ golangci-lint run main.go
main.go:6:9: Error return value of `os.Open` is not checked (errcheck)
        os.Open("foo")
               ^

from

package main

import "os"

func main() {
        os.Open("foo")
}

but it doesn't complain about

package main

import "os"

func main() {
        file, _ := os.Open("foo")
        file.Close()
}

while standalone-errcheck says

(base) oliver@Spud:/mnt/c/Users/oliver/go/src/github.com/kfsone/gocc$ errcheck main.go
main.go:7:12:   file.Close()

[kfsone]

Found it ... .golangci.yml -> linters-settings (and not 'linter-settings' as one of their docs pages said, which I fixed :)

[kfsone]

@awalterschulze if it looks good, lmk if you'd like me to squash. I have one set of colleagues who will only review after squash, another who won't review squashed, and another who will stab you if you squash, so my solution is: HEY LOOK A SQUIRREL.

[awalterschulze]

I think I pressed squash and merge, so should be fine. I am historically your colleagues' worst fear, I have improved, but I guess they still wouldn't like me, even though I have at least started trying.