add initial Makefile and CI config #11
Conversation
Add some initial files for building with a Makefile and GitHub Actions. Format source code with go 1.19 (which rewrites comments). Disable linter from failing the build initially. It would be good to review the warnings and fix as needed or ignore lint warnings which aren't critical.
| @@ -0,0 +1,133 @@ | |||
| # Contributor Covenant Code of Conduct | |||
There was a problem hiding this comment.
Copied from connect-go.
| # For the same reason, only check generated code on the most recent | ||
| # supported version. | ||
| if: matrix.go-version == '1.19.x' | ||
| run: make checkgenerate && make lint |
There was a problem hiding this comment.
Runs lint on go 1.19 (since they format comments now too).
| @@ -0,0 +1,51 @@ | |||
| run: | |||
There was a problem hiding this comment.
Config without exclusions from connect-go.
| MAKEFLAGS += --no-builtin-rules | ||
| MAKEFLAGS += --no-print-directory | ||
| BIN := .tmp/bin | ||
| COPYRIGHT_YEARS := 2020-2022 |
There was a problem hiding this comment.
2020 is first commit to this repo's history.
There was a problem hiding this comment.
Do we have to manually bump this every year? Maybe it could be an expression that can always carry through to current year? (I'm not really sure where it's used, so maybe it doesn't matter...)
There was a problem hiding this comment.
OIC. This is to generate the pre-amble in every file, isn't it?
| MAKEFLAGS += --no-print-directory | ||
| BIN := .tmp/bin | ||
| COPYRIGHT_YEARS := 2020-2022 | ||
| LICENSE_IGNORE := -e /testdata/ -e /testprotos/ |
There was a problem hiding this comment.
Ignored testprotos dir but I think we should relocate this under testdata.
| .PHONY: lint | ||
| lint: $(BIN)/golangci-lint ## Lint Go | ||
| $(GO) vet ./... | ||
| $(BIN)/golangci-lint run || : # Don't fail on lint errors initially |
There was a problem hiding this comment.
Turned off failing on lint errors so we can review the lint failures first and fix them one by one.
| @@ -11,7 +25,7 @@ import ( | |||
| "github.com/bufbuild/protocompile/reporter" | |||
| ) | |||
|
|
|||
| //go:generate goyacc -o proto.y.go -p proto proto.y | |||
| //go:generate goyacc -o proto.y.go -l -p proto proto.y | |||
There was a problem hiding this comment.
Added -l to inhibit line numbers from generated code to work around golangci/golangci-lint#1559.
| @@ -31,7 +45,7 @@ func init() { | |||
| func setTokenName(token int, text string) { | |||
| // NB: this is based on logic in generated parse code that translates the | |||
| // int returned from the lexer into an internal token number. | |||
| var intern int | |||
| var intern int8 | |||
There was a problem hiding this comment.
Latest generated code appears to use smallest possible type to represent values.
There was a problem hiding this comment.
How is the version of goyacc pinned? It's not in go.mod since it's just a tool, not a dependency. I am used to pinning tool versions in the Makefile, like this. Will this just now use whatever version is already installed or always fetch latest?
There was a problem hiding this comment.
I went with a pattern I've seen in other projects and created internal/tools/go.mod to pull in Go tools and be able to keep things up to date with dependabot and use go build without hardcoded versions in Makefile. Let me know what you think.
| @@ -22,7 +36,7 @@ | |||
| // of multiple CPU cores, so a compilation involving thousands of files can be | |||
| // done very quickly by compiling things in parallel. | |||
| // | |||
| // Resolvers | |||
| // # Resolvers | |||
There was a problem hiding this comment.
Wha? This must be new in Go 1.19. This markdown-like syntax was never previously supported. The way to get a heading in Go docs before was to have a single line with no punctuation starting with a capital letter.
There was a problem hiding this comment.
Yeah, this is nice:
https://tip.golang.org/doc/go1.19#go-doc
They finally added support for lists and links!
| @@ -31,7 +45,7 @@ func init() { | |||
| func setTokenName(token int, text string) { | |||
| // NB: this is based on logic in generated parse code that translates the | |||
| // int returned from the lexer into an internal token number. | |||
| var intern int | |||
| var intern int8 | |||
There was a problem hiding this comment.
How is the version of goyacc pinned? It's not in go.mod since it's just a tool, not a dependency. I am used to pinning tool versions in the Makefile, like this. Will this just now use whatever version is already installed or always fetch latest?
| sections: | ||
| - standard # Standard section: captures all standard packages. | ||
| - default # Default section: contains all imports that could not be matched to another section type. | ||
| - prefix(github.com/bufbuild/protocompile) # Custom section: groups all imports with the specified Prefix. |
There was a problem hiding this comment.
Cool! I am not familiar with gci, but this seems strictly better than goimports.
In the past, I've used goimports with -local github.com/bufbuild/protocompile to do this. But the standard behavior of goimports seems just wrong... So I actually have a fork that fixes the behavior (and is what is actually in use at FullStory for imports sorting/checking in CI).
Add some initial files for building with a Makefile and GitHub Actions.
Format source code with go 1.19 (which rewrites comments).
Disable linter from failing the build initially. It would be good to
review the warnings and fix as needed or ignore lint warnings which
aren't critical.