exercism / go-test-runner

GNU Affero General Public License v3.0
15 stars 17 forks source link

Update to Go 1.18 #72

Closed pedreviljoen closed 2 years ago

pedreviljoen commented 2 years ago

As per below issue:

68

This PR updates Go to version 1.18

coveralls commented 2 years ago

Pull Request Test Coverage Report for Build 2413133311


Totals Coverage Status
Change from base Build 2309967076: 0.0%
Covered Lines: 385
Relevant Lines: 498

💛 - Coveralls
andrerfcsantos commented 2 years ago

@pedreviljoen Thanks for this.

This PR can't be merged just yet. The reason is that we are using golangci/golangci-lint-action@v2 to run golangci-lint at v1.43 with the default checks:

https://github.com/exercism/go-test-runner/blob/5670f9ff28f47a0d69a6569dc31c8f2606853bf0/.github/workflows/test.yml#L16-L19

However, golangci-lint in v1.43 doesn't understand generics. The support was only added in v1.45. Bumping the version of golangci-lint will probably make CI pass.

However, there's another nuance to this. At version 1.45.2, golangci-lint uses the version v0.2.2 of honnef.co/go/tools which provides the staticcheck check:

https://github.com/golangci/golangci-lint/blob/8bdc4d3f8044b1a20e10a9f519b5f738e8188877/go.mod#L99

This check is used in our Go repositories, and it is enabled by default by golangci-lint. But support for generics was only added in v0.3.0 of staticcheck. Checking the repository of golangci-lint, I see they already updated to v0.3.0 of staticcheck on master:

https://github.com/golangci/golangci-lint/blob/ff93f2715709ab084d0004b197f5ba84116a65ee/go.mod#L101

However, this is only on master, it's not on any of the official releases. It is expected to be included on the next release of golangci-lint. I would wait for the next release golangci-lint to contain v0.3.0 of staticcheck and only update then.

This should also be merged together with the update to the analyzer.

pedreviljoen commented 2 years ago

@andrerfcsantos Ok cool, let's wait for those peer dependencies to be more stable. Is there a way we can keep an eye on those?

andrerfcsantos commented 2 years ago

The best way is to keep track of new releases of golangci/golangci-lint

There's also this issue keeping track of the support for 1.18 of all the linters of golangci-lint: https://github.com/golangci/golangci-lint/issues/2649

In our case, I think it's fine to wait just for the next release of golangci-lint to update staticcheck to support go 1.18, the other are not as important. Also, like that issue describes, this problem with lack of support only happens when the go.mod for the module being checked declares 1.18 - we don't have any currently in the main repo for the Go track and I won't think we'll have one for some time. Even when we do, I think having just staticcheck (and some other basic checks) is enough and we shouldn't block students to use Go 1.18 just because we are waiting for perfect support for go 1.18 in the tools for our CI (which doesn't affect students directly). We can always do a second upgrade later when those tools add more support for go 1.18.

andrerfcsantos commented 2 years ago

@pedreviljoen golangci-lint released an update today (1.46.0) that updates staticcheck to work with Go 1.18: https://github.com/golangci/golangci-lint/releases/tag/v1.46.0

The relevant changelog lines for us are:

https://github.com/golangci/golangci-lint/commit/e4945f75cccb8ae362f51650a8ead5d4e00799b4 build(deps): bump honnef.co/go/tools from 0.2.2 to 0.3.0 (https://github.com/golangci/golangci-lint/pull/2738) https://github.com/golangci/golangci-lint/commit/5c77c1b9133bc08cbd000ab277523e9f1e6ad11a build(deps): bump honnef.co/go/tools from 0.3.0 to 0.3.1 (https://github.com/golangci/golangci-lint/pull/2780) [...] https://github.com/golangci/golangci-lint/commit/f5b92e1ae287828915d2bb42ff793fb714dfd28b staticcheck: re-enable for go1.18 (https://github.com/golangci/golangci-lint/pull/2746)

So it would be great if you could update this PR to use golangci-lint v1.46.0. It's probably also a good idea to update the versions for our actions:

@pedreviljoen Let us know if you are still interested in finishing this.

pedreviljoen commented 2 years ago

@andrerfcsantos Ok cool will do so tomorrow, while doing the upgrade to 1.18 I see there are two other open requests too. Will perhaps do all 3 then 😅

pedreviljoen commented 2 years ago

@andrerfcsantos Implemented the recommended updates, let me know if you need anything else?

andrerfcsantos commented 2 years ago

@pedreviljoen We'll gladly let you take care of the upgrades for the other repos too, thanks for the help!

Those upgrades should be similar to this one, where we should also upgrade actions/setup-go, actions/checkout and golangci/golangci-lint-action there too where applicable.

The only difference to this PR that comes to mind is that in the main repo (exercism/go), we are installing golangci-lint via a script and not an action, so there the version of golangci-lint must be changed by changing the URL in the script and not by changing an action configuration. And bumping the version of go.mod files that are part of tests is ok, but we should not do it for the go.mod files of student's exercises, specially if the exercise doesn't require generics or anything else in 1.18.

andrerfcsantos commented 2 years ago

@pedreviljoen Still interested in working on this? Totally ok if you are not, just let us know and we can finish it for you. Otherwise, we are happy to let you finish.

pedreviljoen commented 2 years ago

@andrerfcsantos Thanks for pinging me, got busy with a project at work. Will look into the mentioned suggestions and feed back 👍

pedreviljoen commented 2 years ago

@andrerfcsantos All good, updated those coverage jobs failing

andrerfcsantos commented 2 years ago

That seems to have fixed it, cool!

Just waiting for more approvals and for the other tooling PRs before we can merge this one.

Having also the upgrade of the analyzer to 1.18 at least would be important for us to say that the track supports 1.18. Would you also like to work on that one?

pedreviljoen commented 2 years ago

@andrerfcsantos yeah cool, will do 👍

pedreviljoen commented 2 years ago

@andrerfcsantos I think we are in a good place now?

analyzer and representer also bumped to 1.18

andrerfcsantos commented 2 years ago

Yeah. We now just need a review from @exercism/maintainers-admin

ErikSchierboom commented 2 years ago

Is Go 1.18 backwards compatible with the currently used version (1.17 I think)?

andrerfcsantos commented 2 years ago

@ErikSchierboom Yes. New 1.x releases have the implicit promise of being compatible with all previous 1.x versions of Go. This is known as the "Go 1 compatibility promise", and it's detailed here Go 1 and the Future of Go Programs .

It is intended that programs written to the Go 1 specification will continue to compile and run correctly, unchanged, over the lifetime of that specification. At some indefinite point, a Go 2 specification may arise, but until that time, Go programs that work today should continue to work even as future "point" releases of Go 1 arise (Go 1.1, Go 1.2, etc.).

The other related PRs:

andrerfcsantos commented 2 years ago

@ErikSchierboom I'll do follow-up PRs then :)