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

.github: update to Go 1.18.1 #76

Merged
merged 2 commits into from Apr 28, 2022
Merged

.github: update to Go 1.18.1 #76

merged 2 commits into from Apr 28, 2022

Conversation

kevinburkesegment
Copy link
Contributor

Also regenerate all of the stubs, we don't need "+build" anymore
because both Go 1.17 and 1.18 use "go:build" for build tags.

Copy link
Contributor

@achille-roussel achille-roussel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we dropping compatibility with 1.16 if we remove +build?

@kevinburke
Copy link

Are we dropping compatibility with 1.16 if we remove +build?

Yes, I think so, but note the go formatter is doing this automatically. So we either need to use 1.16/1.17 to generate these files always, or upgrade.

IMO if you care enough about perf to use this library you should be given carrots to upgrade, or interested in upgrading.

@achille-roussel
Copy link
Contributor

The segmentio/encoding package depends on this one, users may have indirect dependencies on segmentio/asm, that would break compilation of their programs then.

I'm not against making the change but I think we need to carefully vet the impact, and take proactive steps to mitigate it, for example:

  • document that we are requiring 1.17+ in the README
  • tag a release prior to merging this change so there is a recent stable version to rely on if upgrading to 1.17+ isn't yet possible for some users
  • make an internal announcement that we expect systems to build with 1.17+ moving forward (not useful for users outside our org tho)

@mmcloughlin
Copy link
Contributor

For what it's worth, I think supporting the last two Go versions is a common practice since it matches the support policy of Go itself.

run: make --always-make build

- name: Git Status
- name: Ensure stubs are up to date
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This name isn't correct: more than just stub files are generated.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

run: |
git diff
test -z "$(git status --porcelain)"
go install github.com/kevinburke/differ@latest
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid @latest in CI? Pin a version instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Also regenerate all of the stubs, we don't need "+build" anymore
because both Go 1.17 and 1.18 use "go:build" for build tags.
@kevinburkesegment
Copy link
Contributor Author

Ok - I've done everything on the list here: #76 (comment)

@kevinburkesegment kevinburkesegment merged commit 52fc573 into main Apr 28, 2022
@kevinburkesegment kevinburkesegment deleted the go-1.18 branch April 28, 2022 04:09
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.

None yet

4 participants