Create a separate module for our golangci-lint version #80
Conversation
In principle, it's not a problem to have test-only deps in your go.mod, because they won't end up in your importers' builds (and in newer Go versions may not even be downloaded. (Which is why there's no annotation to do so.) In practice, that doesn't really work for golangci-lint, because it doesn't really use semver (reasonably, in that any updated linter may break lint in your codebase). And we don't really need it other than to run the binary at a particular version. So now, I put it in its own go module. This requires a bit more throat-clearing to run it (we can't just `go run`), but it's not so bad and avoids anyone getting annoyed at us because we upgraded their golangci-lint for them. We could do the same for `internal/integration`, which adds quite a lot, including gqlgen, to our dependency tree, but it's not clear there's a need. Fixes #62. Issue: #62 Test plan: make check Reviewers: marksandstrom, steve, adam, miguel
benjaminjkraft
requested review from
aberkan,
dnerdy,
MiguelCastillo and
StevenACoffman
|
(@StevenACoffman, I cc'ed you here in case you know a better way to do this, or know of reasons why I should do it for gqlgen as well.) |
internal/lint/tools.go
Outdated
| // `go mod tidy` ignores so-tagged files explicitly, so we use another build | ||
| // tag we never intend to set. | ||
| // +build tools | ||
| //go:build tools |
There was a problem hiding this comment.
So is go:build in Go 1.17? I remembered something about "finishing the transition in Go 1.18" (which maybe was just dropping // +build support)?
I also recall that some old versions of Go required // +build tools to be the very first line of the file... but that might have been before go mod so maybe not an issue? 🤷♂️ I think the current rules (until 1.18) are:
- start with the prefix +build followed by one or more spaces
- located at top of file before the package declaration
- have at least one blank line between it and package declaration to prevent it from being treated as package documentation
- The tags separated by space will be interpreted under the OR logic.
- Comma separated tags will be interpreted under the AND logic.
- Each term is an alphanumeric word and if preceded by ! it means it is negated.
There was a problem hiding this comment.
Yeah, I think 1.16-1.18 all sync the two, and 1.17 is where go:build becomes the source of truth. Since it's easy, I plan to keep both around until we have other reasons to drop 1.16, which is presumably a while yet.
With all that said, I could probably remove this build tag now; nobody should be trying to build anything in this module!
| @@ -0,0 +1,6 @@ | |||
| module github.com/Khan/genqlient/internal/lint | |||
|
|
|||
| go 1.14 | |||
There was a problem hiding this comment.
1.14 is really old. I kind of think Khan is close to the trailing edge when it comes to Go version support. Most surveys show that 80% are on the current or last version by 6 months after a release. With container based runtimes and dependabot, it's automagical.
There was a problem hiding this comment.
Yeah, I think that's accurate. But my general feeling is to wait to drop support until there's a nontrivial reason to, which is why I dropped 1.13 recently, but kept 1.14.
|
No, Craig actually just updated golangci-lint in webapp when pulling in the change that added it to genqlient, so it was not an issue in practice at Khan. But I still felt it was a potential problem! |
Summary:
In principle, it's not a problem to have test-only deps in your go.mod,
because they won't end up in your importers' builds (and in newer Go
versions may not even be downloaded. (Which is why there's no
annotation to do so.) In practice, that doesn't really work for
golangci-lint, because it doesn't really use semver (reasonably, in that
any updated linter may break lint in your codebase). And we don't
really need it other than to run the binary at a particular version.
So now, I put it in its own go module. This requires a bit more
throat-clearing to run it (we can't just
go run), but it's not so badand avoids anyone getting annoyed at us because we upgraded their
golangci-lint for them.
We could do the same for
internal/integration, which adds quite a lot,including gqlgen, to our dependency tree, but it's not clear there's a
need.
Fixes #62.
Issue: #62
Test plan:
make check