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

Fix Go dependencies #5088

Merged
merged 2 commits into from
Oct 23, 2023
Merged

Fix Go dependencies #5088

merged 2 commits into from
Oct 23, 2023

Conversation

2opremio
Copy link
Contributor

@2opremio 2opremio commented Oct 23, 2023

#5084 broke the Go dependencies, because:

  1. 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)
  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)

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)
@2opremio
Copy link
Contributor Author

https://github.com/stellar/soroban-tools is also broken. We should fix it next

go.mod Show resolved Hide resolved
@2opremio 2opremio force-pushed the fix-go-dependencies branch from d8a4331 to 5a68a24 Compare October 23, 2023 14:34
@tsachiherman
Copy link
Contributor

I'm surprised you're saying that go mod tidy wasn't called. I did call that, and deployed 1.19 before doing that.

Also, if it broke the build, how come all the CI tests passed ?!

@2opremio
Copy link
Contributor Author

2opremio commented Oct 23, 2023

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)

@2opremio
Copy link
Contributor Author

2opremio commented Oct 23, 2023

Also, if it broke the build, how come all the CI tests passed ?!

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)

@2opremio 2opremio merged commit a3f37f4 into stellar:master Oct 23, 2023
36 checks passed
@2opremio 2opremio deleted the fix-go-dependencies branch October 23, 2023 17:25
@tsachiherman
Copy link
Contributor

Also, if it broke the build, how come all the CI tests passed ?!

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 ?

@2opremio
Copy link
Contributor Author

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:

we should probably enforce this in CI and make sure there are no diffs

@leighmcculloch
Copy link
Member

I guess CI works because Go updates go.mod when building with 1.21

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:

Build commands like go build and go test no longer modify go.mod and go.sum by default. Instead, they report an error if a module requirement or checksum needs to be added or updated

Ref: https://go.dev/doc/go1.16

This was strengthened in Go 1.18 with additional commands being changed to not modify go.mod:

Automatic go.mod and go.sum updates

The go mod graph, go mod vendor, go mod verify, and go mod why subcommands no longer automatically update the go.mod and go.sum files. (Those files can be updated explicitly using go get, go mod tidy, or go mod download.)

Ref: https://go.dev/doc/go1.18

I think something else is probably happening.

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.

4 participants