Improve GitHub Actions performance and also local formatting/testing/linting

[gmlewis]

I'm really kinda getting tired of waiting for golintci to finish in this repo.
Yes, we have a huge number of linters in addition to custom ones.
There's a new kid on the block that claims 2x performance boost: https://revive.run/
It might be nice if someone wants to investigate switching this repo over from golintci to revive and perform benchmarks on "Before vs After" so we can see if this would be a beneficial switch.
Obviously, we would want the same coverage, as all of our linters are in place for good reason.

[stevehipwell]

@gmlewis do you mean golangci-lint? If so I'm not sure they're comparable as golangci-lint is an aggregator not an actual linter. golangci-lint does have support for revivie so if you could replace some of the other linters called by golangci-lint with revive that may improve performance.

FYI we currently have two unnecessary formatters configured, gofmt (unnecessary as gofumpt is a superset) & gci (unnecessary as we're using goimports and don't have a complicated import policy that would need this mainly by virtue of having very few dependencies). Removing these two formatters speeds up formatting the whole repo by just over 15%.

[gmlewis]

Thank you, @stevehipwell! Yes, any speedups in our CI/CD pipeline, both on GitHub Actions and when running locally would be really nice, so feel free to make PRs for performance gains in formatting/testing/linting.

[stevehipwell]

Just looking at the revive repo and it looks like this is @alexandear area of expertise so I'd be really interested in his view. Off the top of my head I'd think that we'd still want golangci-lint but that we might be able to replace some of the existing formatters with revive.

[stevehipwell]

On the subject of linters, shouldn't we have linters.default set to none so only the explicitly enabled ones are used? Also I think copyloopvar may be unnecessary as we're > 1.22?

[alexandear]

On the subject of linters, shouldn't we have linters.default set to none so only the explicitly enabled ones are used?

A missing linters.default setting means we get the default value linters.default: standard, which includes linters errcheck, govet, ineffassign, staticcheck, unused. We can explicitly enable these linters and then add linters.default: none to the configuration.

Also I think copyloopvar may be unnecessary as we're > 1.22?

copyloopvar ensures that we don't introduce any loop variables copies like i := i. See #3537 for details.

[alexandear]

Just looking at the revive repo and it looks like this is @alexandear area of expertise so I'd be really interested in his view. Off the top of my head I'd think that we'd still want golangci-lint but that we might be able to replace some of the existing formatters with revive.

I'm one of three Revive's maintainers.
We can't replace the currently enabled golangci-lint linters with Revive. While Revive provides many rules, it doesn't cover all linters available in golangci-lint. Additionally, Revive performs poorly on large codebases as it lacks caching. Also, Revive doesn't have go/analysis support.

[alexandear]

Since building custom-gcl takes about 40–50 seconds on GitHub Actions, we can cache it to achieve a significant speedup.

From the screenshot of a running "lint" job:

We can see

11:53:13 - start downloading golangci-lint and building .bin/custom-gcl
11:53:58 - end with custom-gcl and start linting

Caching can be done using either https://github.com/actions/setup-go or https://github.com/actions/cache.

[stevehipwell]

A missing linters.default setting means we get the default value linters.default: standard, which includes linters errcheck, govet, ineffassign, staticcheck, unused. We can explicitly enable these linters and then add linters.default: none to the configuration.

@alexandear that would be clearer and deterministic.

copyloopvar ensures that we don't introduce any loop variables copies like i := i. See #3537 for details.

This is captured by modernize, it's only codebases < 1.22 that need to keep copyloopvar as obviously modernize isn't doing anything there.