Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Go 1.17 is the minimum version in all cases #1337

Merged
merged 1 commit into from
Mar 28, 2022

Conversation

TBBle
Copy link
Contributor

@TBBle TBBle commented Mar 26, 2022

Closes: #950
per #950 (comment)

Are we all good to close this one out now that we're on 1.17?

This ensures:

  • CI explicitly requires Go 1.17
  • README specifies Go 1.17
  • Tests also depend on Go 1.17

The go.mod already specifies 1.17 as the minimum Go version, so this shouldn't be a difference in practice.

GitHub Actions setup-go was installing Go 1.17 already, presumably honouring the value in go.mod.

One interesting change is that in 1.17, go.mod lists indirect dependencies, and vendoring does not capture go.mod/go.sum, so the test/vendor directory got slightly less noisy.

This ensures:
- CI explicitly requires Go 1.17
- README specifies Go 1.17
- Tests also depend on Go 1.17

The go.mod already specifies 1.17 as the minimum Go version, so this
shouldn't be a difference in practice.

GitHub Actions setup-go was installing Go 1.17 already, presumably
honouring the value in go.mod.

One interesting change is that in 1.17, go.mod lists indirect
dependencies, and vendoring does not capture go.mod/go.sum, so the
test/vendor directory got slightly less noisy.

Signed-off-by: Paul "TBBle" Hampson <Paul.Hampson@Pobox.com>
@TBBle TBBle requested a review from a team as a code owner March 26, 2022 07:46
@dcantah
Copy link
Contributor

dcantah commented Mar 26, 2022

Embarrassing that I forgot to update the README after the 1.17 bump.. and I was the one who made it 😆. I'm curious, given I don't think we actually use any 1.17 features (although could be forgetting some), could we get away with building this on a version of go prior to the minimum supported version in our go.mod? We use some 1.16 features so if we wanted to be able to build everything this would be minimum 1.16. I'm always confused on reading Go's description of how this value is used and what it represents in terms of compilation, and looks like I'm not alone..

@TBBle
Copy link
Contributor Author

TBBle commented Mar 26, 2022

Yeah, I did go and look up specifically in the specs that the go.mod value is described as go _minimum-go-version_

The minimum version of Go required to compile packages in this module.

but the docs of the directive itself indicate that this doesn't block older code from using the module, just that if such a compile fails, it'll flag that the dependency is marked as requiring a newer version of Go.

And it doesn't let you use newer syntax than the go.mod-claimed version, thankfully.

So it really only applies to code covered by the go.mod, and other code is best-effort. We didn't have any issues with the tests building while marked for 1.16, so a 1.16 minimum version would probably work correctly too.

That said, the new-in 1.17 unsafe language features might be useful if they make the Win32-API-structure-manipulation code cleaner. More of a problem in go-winio though, which from memory has a lot of such structure activities.

@dcantah
Copy link
Contributor

dcantah commented Mar 26, 2022

I'm fine with just saying 1.17 minimum, 1.16 is out of support now anyways 😶

That said, the new-in 1.17 unsafe language features might be useful if they make the Win32-API-structure-manipulation code cleaner. More of a problem in go-winio though, which from memory has a lot of such structure activities.

Right, we were looking to use unsafe.Slice in winio for some new code as well iirc

@dcantah dcantah merged commit 6dd7225 into microsoft:master Mar 28, 2022
@TBBle TBBle deleted the go-1_17-everywhere branch March 28, 2022 23:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Minimum Go version is now at least 1.13
3 participants