-
Notifications
You must be signed in to change notification settings - Fork 500
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
Fix Go dependencies #5088
Fix Go dependencies #5088
Conversation
stellar#5084 broke go.mod becaue: 1. go mod tidy wasn't invoked before merging (we should probably enforce this is CI and make sure there are no diffs) 2. It ugraded github.com/creachadair/jrpc2 to 1.1.1 which requires Go 1.21 (breaking the two-Go-release compatibility guarantee we make)
https://github.com/stellar/soroban-tools is also broken. We should fix it next |
d8a4331
to
5a68a24
Compare
I'm surprised you're saying that Also, if it broke the build, how come all the CI tests passed ?! |
It wasn't (at least if you were running Go 1.21 locally), since otherwise it would had written 1.21 to go.mod (just try it locally) |
I never wrote it broke the build, it broke the two-Golang release policy. I guess CI works because Go updates go.mod when building with 1.21 (or does a best effort when using an earlier version) |
that's a bit concerning. should we add a test to verify that the go.mod/go.sum doesn't get updated during the build ? i.e. so that it's only developer-driven ? |
Yes, I suggested that in the PR description:
|
This is surprising. Go since version 1.16 has not updated the go.mod automatically. In the release notes for Go 1.16 it says:
Ref: https://go.dev/doc/go1.16 This was strengthened in Go 1.18 with additional commands being changed to not modify go.mod:
Ref: https://go.dev/doc/go1.18 I think something else is probably happening. |
#5084 broke the Go dependencies, because:
go mod tidy
wasn't invoked before merging leading to a misaligned Go version in go.mod (we should probably enforce this in CI and make sure there are no diffs)