-
-
Notifications
You must be signed in to change notification settings - Fork 664
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
Switch to Go 1.19 as minimum (attempt 2) #2714
Conversation
No need for `go.uber.org/atomic` anymore since 1.19 adds new atomic types
IMO wait until 1.19 is in debian stable backports. https://packages.debian.org/bullseye-backports/golang Maybe there's another popular "stable server" distro with a backports equivalent that would be worth considering also. |
Actually, is it not already? https://packages.debian.org/bullseye-backports/golang-1.19-go By the looks of it, the golang metapackage has not been updated to 1.19 but Debian users can still explicitly install 1.19 from the official repos. Funnily enough, that's the case on Tumbleweed too which is a rolling distro :P |
hmm you are correct |
Debian backports' |
Thanks @bones-was-here. Given that and the lack of objections, I think this is probably safe to merge sometime soon. Note that sytests are broken until matrix-org/sytest#1292 is merged, but I've fixed the other tests. |
Ah, yes, that was the idea. The comment can be removed. Added that comment before having actually used generics in Go. |
Thanks for the PR, but for now we've decided against merging this. The only changes that require actual code modifications are the atomics — the other benefits listed in the comments can all be obtained by just compiling Dendrite today using Go 1.19. I'll commit an update to the dockerfiles to build those using Go 1.19 too. Go 1.19 is still relatively recent though and may not have tricked down into the distributions that our users care about without extra work, so in the interests of not making life harder for people to run Dendrite, we'll stick with supporting Go 1.18 for the short term. |
My server uses an architecture unsupported by gc (the go compiler you all know and love), so I have no other option but to use gccgo. Gccgo will always lack behind gc in terms of what version of go it supports. I know building with gccgo is probably not a "supported" usecase, and likely won't ever be for dendrite, but I thought it was worth commenting showing there is at least a single user out there hoping dendrite supports the lowest version of go possible :) |
That, admittedly, is not something I've considered. Fair enough! |
Given that #2714 wasn't merged but we are now at a minimum supported Go version of 1.20 (soon to be 1.21), I wanted to carry over some of the changes. Namely: - Fix the log typo - Simplify build constraints for unix - Use stdlib atomic package ### Pull Request Checklist <!-- Please read https://matrix-org.github.io/dendrite/development/contributing before submitting your pull request --> * [x] I have added Go unit tests or [Complement integration tests](https://github.com/matrix-org/complement) for this PR _or_ I have justified why this PR doesn't need tests * [x] Pull request includes a [sign off below using a legally identifiable name](https://matrix-org.github.io/dendrite/development/contributing#sign-off) _or_ I have already signed off privately Signed-off-by: `0x1a8510f2 <[email protected]>` --------- Co-authored-by: devonh <[email protected]>
Reasoning
Go 1.19 has been out for over a month now (02/08/2022) and features some changes which are likely to be quite beneficial here (see #2564). Perhaps most importantly, the release allows one to set a soft memory limit on the runtime which helps a lot in the P2P usecase.
Other noteworthy changes include:
go.uber.org/atomic
anymore)Hence, I think it's worth incrementing the minimum supported version to benefit from the above.
As an alternative, I could revert backwards-incompatible changes and just use go 1.19 in CI to benefit from most of this but still allow building with 1.18. That said, I personally think the code removed because of the upgrade warrants incrementing the minimum.
No tests needed beyond what already exists as this doesn't really change the code in any meaningful way.
Changes Summary
go.uber.org/atomic
withsync/atomic
and move the former into the indirect deps section ingo.mod
Side Note
dendrite/syncapi/storage/postgres/filtering.go
Lines 41 to 50 in af9a204
I came across this comment while searching for
1.18
. I assume the comment means the function should be merged with the duplicate below it using generics? Unfortunately, Go generics don't allow for accessing properties, only methods. Should I remove the comment?Pull Request Checklist
Signed-off-by:
0x1a8510f2 <[email protected]>