Use go 1.22 to allow building with go1.18

[alexandear]

When go.mod uses go 1.22.0 (with .0), building with go1.18 or go1.19 fails.

Before:

$ go1.18 build ./...
go: errors parsing go.mod:
/Users/Oleksandr_Redko/src/github.com/google/go-github/go.mod:3: invalid go version '1.22.0': must match format 1.23

$ go1.19 build ./...
go: errors parsing go.mod:
/Users/Oleksandr_Redko/src/github.com/google/go-github/go.mod:3: invalid go version '1.22.0': must match format 1.23

After:

$ go1.18 build ./...

$ go1.19 build ./...

Follows #3423

[alexandear]

#3428 reverted to fix the error while running ./script/lint.sh:

linting tools
validating openapi_operations.yaml
validating generated files
go: downloading go1.22 (linux/amd64)
go: invalid toolchain: go1.22 is a language version but not a toolchain version (go1.22.x)
failed validating generated files

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.26%. Comparing base (2b8c7fa) to head (ac966a0).
Report is 221 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3432      +/-   ##
==========================================
- Coverage   97.72%   92.26%   -5.46%     
==========================================
  Files         153      174      +21     
  Lines       13390    15023    +1633     
==========================================
+ Hits        13085    13861     +776     
- Misses        215     1068     +853     
- Partials       90       94       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[gmlewis]

@alexandear - my question with this PR is... with your changes, is this simple to maintain?

We've had a lot of churn in the last few weeks with Go versioning where people coming late to the party were not involved in the original changes (I'm not talking about you) and now we seem to be back-peddling to make sure everyone is happy that has now spoken up.

So what I really want to know is... either as a developer or as a maintainer, can I run the scripts under ./script/ and make no further manual edits, and tests will pass and the go.mod and go.sum files will be properly preserved to everyone's liking? Also, if my editor runs go fmt on the go.mod file upon save, will it get messed up by the latest tooling?

I've seen some very weird behavior in the last few weeks due to the GOTOOLCHAIN stuff and latest versions of Go tweaking the go.mod files to their heart's content without properly preserving the history needed to make everyone happy.

Thoughts?

[gmlewis]

ALSO: Is the README.md still correct about versioning and toolchains after this PR?

[gmlewis]

@dnwe - please comment on this PR.

[dnwe]

@gmlewis whilst I understand the motivation behind the PR (the code can currently be compiled with older versions than 1.22) and have some sympathy for it, I don’t think we should accept the PR as maintaining support for 1.18 isn’t a goal going forward and isn’t something we’ll test or commit to. I think the current model of support N-1 and N major releases is the correct stance for the module

[gmlewis]

@gmlewis whilst I understand the motivation behind the PR (the code can currently be compiled with older versions than 1.22) and have some sympathy for it, I don’t think we should accept the PR as maintaining support for 1.18 isn’t a goal going forward and isn’t something we’ll test or commit to. I think the current model of support N-1 and N major releases is the correct stance for the module

OK, thank you for your input, @dnwe.

I've got conflicting feelings and sympathize with both viewpoints. Throughout the years, we have attempted to keep this client library usable by the largest audience possible. It is true that someone can pin their usage to a past version of this client library, but the downside to that is that the GitHub v3 API keeps evolving. And while I personally don't understand why people are not using one of latest minor versions of Go itself, I also know that sometimes big corporate projects don't have the bandwidth allocated to bring the project into the modern era by updating their Go version.

So if @willnorris, the original author of this client library, has time to comment, I would greatly appreciate any guidance Will can provide.

[willnorris]

I wasn't aware there was a meaningful difference between 1.22 and 1.22.0 in this context, and there seems to be no indication in https://go.dev/ref/mod#go-mod-file-go of what the difference is. So it's hard for me to fully wrap my head around the impact of this and why one allows older go version to work (I wouldn't have thought it would).

That said, as a general philosophical point, I'm in favor of not breaking older versions if we don't have to, even if the official support is only for N-1. Assuming the proposed change here doesn't have any other unforeseen adverse effects (and I can't imagine that it would), I'd be in favor of making this change.

[gmlewis]

Thank you, @willnorris , I appreciate your feedback!
In a nutshell, #3423 and #3428 have some very interesting notes about Go 1.22, Go toolchains, and the Go team that you might want to read.

I'll wait for @alexandear to answer my maintenance questions before proceeding.

[gmlewis]

@stevehipwell - looking at #3423, you might wish to comment on this PR as well.

[willnorris]

wow, fascinating... I completely missed all of that in golang/go#62278, though I certainly do remember noticing the change in vet warnings some time last year.

Even after reading that, I think I'm still of the same opinion. If go-github actually requires some behavior in a patch release of 1.22, then that's one thing. But assuming that it doesn't, using 1.22 (which will be interpreted as 1.22.0) seems reasonable to allow older versions of go to still function. There may come a day that breaking that compatibility is necessary for some reason, but it seems pretty low-effort to maintain in the meantime.

[dnwe]

@willnorris the problem is that changing to 1.22 like in this PR is a hack rather than a maintainable stance that sort of works because go 1.18 doesn't know about toolchains (introduced in go 1.21) so only understands the old 1.N style directives, and go 1.21 and newer expect 1.N.P. But a go 1.22 directive is actual invalid (see explanation later).

Before Go 1.21, Go treated the go line as an advisory requirement: if builds succeeded the toolchain assumed everything worked, and if not it printed a note about the potential version mismatch. That's why changing to 1.22 happens to allow 1.18 to compile again. Go 1.21 changed the go line to be a mandatory requirement instead. This behaviour is partly backported to earlier language versions: Go 1.19 releases starting at Go 1.19.13 and Go 1.20 releases starting at Go 1.20.8 will refuse to load workspaces or modules declaring version Go 1.22 or later in the same way as newer releases.

I wasn't aware there was a meaningful difference between 1.22 and 1.22.0 in this context, and there seems to be no indication in https://go.dev/ref/mod#go-mod-file-go of what the difference is. So it's hard for me to fully wrap my head around the impact of this and why one allows older go version to work (I wouldn't have thought it would)

It's complicated, but 1.22 is a language version and 1.22.x is a toolchain version (see https://go.dev/doc/toolchain#version for more explanation). From Go version 1.21 the 'go directive' is meant to be a minimum toolchain version. CodeQL will actually flag go 1.22 as erroneous in a go.mod file like this:

Invalid Go toolchain version

As of Go 1.21, toolchain versions must use the 1.N.P syntax.

So I don't think it would really be a desirable solution here either.

That said, as a general philosophical point, I'm in favor of not breaking older versions if we don't have to, even if the official support is only for N-1. Assuming the proposed change here doesn't have any other unforeseen adverse effects (and I can't imagine that it would), I'd be in favor of making this change.

If you really wanted to maintain support for 1.18 then the correct solution would be to change the go.mod to have a go directive of 1.18 and to have GitHub actions' build matrix test using that to maintain the compatibility

[stevehipwell]

@gmlewis I'll have a refresher on the finer details around the Go tooling tomorrow before I comment on specifics. But as a general rule I'd advocate for the principal of least astonishment.

[willnorris]

If you really wanted to maintain support for 1.18 then the correct solution would be to change the go.mod to have a go directive of 1.18 and to have GitHub actions' build matrix test using that to maintain the compatibility

I actually thought we might have had that, but don't see it now. I actually do that in other projects I maintain... keeping a matix test for the lower working version, even if it's not actually a supported version.

For example: https://github.com/willnorris/imageproxy/blob/main/.github/workflows/tests.yml#L22-L26 (although somewhat ironically, that is also using a 1.N style version, so isn't actually indicating the lowest working version... it should include whatever patch version it works on, presumably .0)

[willnorris]

Do we know if tools like CodeQL and others updated to reflect the change of heart from the Go team in how the go directive is interpreted? From reading through the go issue, it sounds like the desire and intention is for the 1.N style declaration to still be treated as valid, albeit with certain caveats. (or am I misinterpreting the state of things, having spent far less time thinking about this than you all?)

[dnwe]

Do we know if tools like CodeQL and others updated to reflect the change of heart from the Go team in how the go directive is interpreted? From reading through the go issue, it sounds like the desire and intention is for the 1.N style declaration to still be treated as valid, albeit with certain caveats. (or am I misinterpreting the state of things, having spent far less time thinking about this than you all?)

Yes from Go 1.23 onward they aliased go 1.23 to be equivalent to go 1.23.0, though it is still not really recommended as Go 1.21 and Go 1.22 vet will still consider it to be invalid (though 1.22.x might get a backport I suppose) 😅

I did raise it on the CodeQL issue tracker over here though

[dnwe]

Note: they didn't have a change of heart about treating the go directive as a hard minimum though, they just permitted the shorter syntax to hopefully avoid people bumping their go directive for every minor release

[willnorris]

Note: they didn't have a change of heart about treating the go directive as a hard minimum though, they just permitted the shorter syntax to hopefully avoid people bumping their go directive for every minor release

Right, I think I get that. But that other key difference is that older go version will attempt to download go1.N.0, which is what allows go1.18 and go1.19 to still function. It's still using go1.22.0 as the hard minimum version, but the behavior to automatically download and use a newer toolchain is only triggered when using the 1.N syntax, right?

[dnwe]

Note: they didn't have a change of heart about treating the go directive as a hard minimum though, they just permitted the shorter syntax to hopefully avoid people bumping their go directive for every minor release

Right, I think I get that. But that other key difference is that older go version will attempt to download go1.N.0, which is what allows go1.18 and go1.19 to still function. It's still using go1.22.0 as the hard minimum version, but the behavior to automatically download and use a newer toolchain is only triggered when using the 1.N syntax, right?

No the older Go versions don’t download anything, they just parse the directive and then ignore it and build with the local toolchain. However, when you get to the minor version Go 1.19.13 suddenly it will start parsing it and also refusing to compile because it’s > 1.19

Only Go versions 1.21 and newer will download a compatible toolchain

[alexandear]

@alexandear - my question with this PR is: with your changes, is this simple to maintain?

Yes, it should be simple. As an example, I can provide links to the Go repository itself, which defines go 1.X in their go.mod:

• go 1.22: https://github.com/golang/go/blob/go1.22.10/src/go.mod#L3
• go 1.23: https://github.com/golang/go/blob/go1.23.4/src/go.mod#L3
• go 1.24: https://github.com/golang/go/blob/368a9ec99834652ca3f7d8fe24862a7581e12358/src/go.mod#L3

Also, google/cadvisor uses 1.22 https://github.com/google/cadvisor/blob/be773c0a2b04536a818f79339bf89c82a74abb02/go.mod#L3

On the other hand, the x/tools library repository uses 1.X.0:

• go 1.22.0: https://github.com/golang/tools/blob/3f87563a23a2cd14d853519141bd10a0a4718a2d/go.mod#L3

See golang/go#69095 for the reasons behind that.

In conclusion, after reading all comments, I'm sticking to 1.22.0 (or 1.23.0 when 1.24 comes out) in the go.mod. I can close the PR.

[stevehipwell]

Given that this package isn't compatible with Go 1.18 & 1.19 (go 1.22.0 is set in go.mod) what is the specific issue here?