Upgrade to Go 1.16+ - Githubissues

rootulp commented 3 years ago

My solution to tournament passes locally but fails on exercism.com due to:

# tournament [tournament.test]
./tournament.go:88:16: undefined: io.ReadAll
FAIL    tournament [build failed]
'/usr/local/go/bin/go test --short --json .' returned exit code 2: exit status 2

It looks like io.ReadAll was added in Go 1.16 per this issue. However, the test-runner appears to run with Go 1.15 per https://github.com/exercism/go-test-runner/blob/b6393ab75da7b6bd6d0526e55608da8009f6dc34/go.mod#L3

Should we consider upgrading the test runner to Go 1.16+?

jmrunkle commented 3 years ago

@junedev - I have no problem with this, but we should probably upgrade the go.mod files in the main repo as well. Further thought, I think there will be even more pressure to upgrade once we want to include generics and want to support a concept exercise that handles generics.

junedev commented 3 years ago

I would like to combine the Go update with establishing some proper checking/syncing for the go.mod files in all the exercises.

Tbh since we don't have a lot of excess contributor/maintainer capacity at the moment my tendency would be to wait for Go 1.18 (with Generics) and then upgrade all the things. Until then we probably have our hands full with the concept exercises, test runner fixes, analyzers for concept exercises etc

andrerfcsantos commented 3 years ago

@junedev What problems do you anticipate from this upgrade?

The way I see it, it's just an upgrade in the Dockerfile of the test runner and with a script we can upgrade all the go.mod files to target a specific version. Do you have something else in mind?

As a general strategy, I think we should keep the test runner image always running the latest major release of Go.

Since the go.mod file only specifies a minimum version, the version specified there doesn't need to exactly match the one on the test runner. For that minimum version, I suggest it to be at least one version behind from the one on the test runner. The rationale for this is that Go keeps their official support for 2 major releases, the current and the previous one. But we probably should be a more lenient than that and make the minimum version in the go.mod file lag more than one version behind. This is because people on linux might be using the go version that comes with their package manager, and sometimes those versions are a bit behind. As a concrete example, right now the latest major version of Go is 1.17, but looking at the ubuntu packages repository, their most recent LTS is focal (20.04) and it's is running go 1.14, which is 3 major versions behind. Of course there are some exercises that can be exceptions. For instance, if one exercise uses a concept introduced in a specific version, its go.mod file should be set to that version, regardless of everything else.

Apart from the upcoming 1.18 release that everyone is excited about, there is another reason to upgrade: at least 3 students I mentored were confused why when they use io.ReadAll locally everything works, but then they use the editor and their code suddenly doesn't pass the tests. This is because io.ReadAll was only introduced in 1.16 and our test runner is running 1.15. I told those students to use ioutil.ReadAll instead, but this is clearly not great, because if they are running a version that supports io.ReadAll, they should use that instead, and not ioutil.ReadAll just because of exercism.

junedev commented 3 years ago

@andrerfcsantos There is no problem at all. I just meant that if we want to do this properly it is more work that changing one number somewhere and we didn't have much maintainer capacity at that point. (Remember that was before you even joined as maintainer.)

For me, a proper update would include the following tasks (incl. the version aspect you mentioned).

[x] update the dockerfile and go.mod file of the test runner, analyzer and representer to the latest Go version
[x] pin the major.minor version of alpine used in the dockerfiles above to the latest alpine version available in combination with the latest Go version
[x] in the track repo add a new script that has two modes (changed via some command line flag):
- update mode where it changes all "go x.y" strings in the Go mod file to some new version (later we need a slug-version map with exceptions as you mentioned)
- check mode that can be used in CI that ensures all mod files adhere to the established scheme

As for the Go version in the tracks go.mod files my tendency would be to set 1.16 for now. Otherwise students could still read about io.ReadAll somewhere and it wouldn't work for them locally because of their older Go version. I think everyone who uses a Linux distro can be trusted to be able to update their Go installation. We should make sure to mention the error message you get if your Go version is too old in some of the documentation with info on how to fix it. In the end, updating your installation every year if you want to stay "in maintenance" is part of the Go experience. Not really sure what is the best solution here though.

In any case, if you want to work on this, feel free to do so.

junedev commented 3 years ago

@andrerfcsantos Another challenge we will have to solve once we have a generics (Go 1.18) exercise is that we currently run all the exercise tests on multiple Go versions in the CI pipeline. (https://github.com/exercism/go/blob/main/.github/workflows/exercise-tests.yml)

That means if a go.mod file would specify 1.18, that exercise would need to be skipped if, e.g. the test run is performed with 1.17. Here the file that runs the tests for reference: https://github.com/exercism/go/blob/main/bin/test-without-stubs

junedev commented 2 years ago

Re the issue I mentioned above (https://github.com/exercism/go/issues/1881#issuecomment-959705810), I think the matrix tests with multiple Go versions don't really add much value for Go because of the compatibility promise. So if we change the the exercise-tests.yml file to only run with the latest Go version, we can be sure all solutions can be compiled.

That said, I think this means this issue can be closed now. WDYT @andrerfcsantos?

andrerfcsantos commented 2 years ago

That's a good idea of testing just the latest version. I'd say to keep the tests on multiple platforms.

I kept this open due to the open PR in the analyzer (https://github.com/exercism/go-analyzer/pull/26), which I've updated now. Once that PR is merged we can declare the upgrade completed and close this.

junedev commented 2 years ago

Yes, multiple platforms are fine in the test of course. I just meant only having one entry for the version in the "matrix".

andrerfcsantos commented 2 years ago

I merged the analyzer PR, so I'll close this one.

Made an issue for the removal of old versions from CI: https://github.com/exercism/go/issues/2089

exercism / go

Upgrade to Go 1.16+ #1881