-
Notifications
You must be signed in to change notification settings - Fork 17
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
Golang 1.17 upgrade #15
Golang 1.17 upgrade #15
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems fine, I just have a couple questions.
|
||
require ( | ||
golang.org/x/mod v0.2.0 // indirect | ||
golang.org/x/xerrors v0.0.0-20191204190536-9bdfabe68543 // indirect |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this in a separate require block? did go mod tidy create it this way?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it was from running tidy. I left it that way since I liked the fact that direct deps were being separated from indirect. Not aware of whether this is a convention or something we don't do in other repos.
@@ -4,8 +4,6 @@ | |||
# Installation can still be performed with a | |||
# normal `go install`. | |||
|
|||
# generated integration test files | |||
GGEN = ./_generated/generated.go ./_generated/generated_test.go |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What was the failure that made it so this needed to be removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume there will be a lot of them. The code these tests use was ripped out. For example, https://github.com/algorand/msgp/blob/master/_generated/convert_test.go#L13
uses https://github.com/tinylib/msgp/blob/43b487371e0cdd1a3fe1be9a7802d8bce917a1af/msgp/write.go#L118 from the original repository which was removed from our fork in this commit 546fb5f
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, assuming you're able to generate the desired code on go-algorand let's go ahead and merge. We'll need to create a tag on this repo and update go-algorand to use the new version.
I tested that the generated Going to merge this in and create tag |
Migrate writing build headers from
//+build
to//go:build
.This change was made in go1.17 which means it will be backwards incompatible with anything older than that. I have go upgrades for all of go-sdk, go-algorand, and indexer in PR so that shouldn't be a problem.
I also removed all of the _generated tests from the Makefile since they cause
make test
to fail.Additionally, I've added coverage output and a unit test for the function I've updated which is currently uncovered (possibly because the _generated tests no longer work.