-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
btcec: update gomod and go version #2239
Conversation
btcec/go.mod
Outdated
@@ -1,16 +1,19 @@ | |||
module github.com/btcsuite/btcd/btcec/v2 | |||
|
|||
go 1.17 | |||
go 1.22.6 |
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 think we might need to keep this at an older version? Or at least have the old 2-part format, since this seems to break the unit tests (or the linter).
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 think there's something wrong with our CI here, still digging but the tests are failing locally but not on github actions hmmm
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.
turns out that locally all the sub-modules are pointed to the local versions as indicated by the go.work
, which is now fixed once the versions are updated here.
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.
Hmm, I do know there's this hardware wallet (?) that relies on GopherJS (Go transpiler) which doesn't support the latest version. In the past, we wanted to introduce some code using generics, but backed off at their request. Though ultimately we can't hold it all back forever...
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.
hmm yeah didn't know that before. It looks like they only support up to 0.19.0, should we use 0.19.0
instead?
f9831f7
to
950749c
Compare
Pull Request Test Coverage Report for Build 10484833064Details
💛 - Coveralls |
950749c
to
3fdad4b
Compare
3fdad4b
to
6f45414
Compare
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 no longer maintain this, so it's not my call, but I would highly suggest avoiding an entire dependency (and all of its dependencies, transitively) that everyone who uses the btcec module then has to also pull all for a single test.
I'm talking about testify here. Adding an entire dependency for a single test is kind of ridiculous.
@@ -1,16 +1,19 @@ | |||
module github.com/btcsuite/btcd/btcec/v2 | |||
|
|||
go 1.17 | |||
go 1.22 |
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.
Also, requiring Go 1.22 when it doesn't actually require it is bad for consumers of the dependency too.
TODO:
btcec/v2.3.5
btcutil/psbt/v1.1.10
btcutil/v1.1.7