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

btcec: update gomod and go version #2239

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

yyforyongyu
Copy link
Contributor

@yyforyongyu yyforyongyu commented Aug 20, 2024

TODO:

  • tag btcec/v2.3.5
  • tag btcutil/psbt/v1.1.10
  • tag btcutil/v1.1.7

btcec/go.mod Outdated
@@ -1,16 +1,19 @@
module github.com/btcsuite/btcd/btcec/v2

go 1.17
go 1.22.6
Copy link
Collaborator

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

Copy link
Contributor Author

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

Copy link
Contributor Author

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.

Copy link
Member

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

Copy link
Contributor Author

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?

@yyforyongyu yyforyongyu force-pushed the update-gomod-btcec branch 3 times, most recently from f9831f7 to 950749c Compare August 20, 2024 22:45
@coveralls
Copy link

coveralls commented Aug 20, 2024

Pull Request Test Coverage Report for Build 10484833064

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 12 unchanged lines in 4 files lost coverage.
  • Overall coverage decreased (-2.0%) to 55.262%

Files with Coverage Reduction New Missed Lines %
btcutil/gcs/gcs.go 1 80.95%
mempool/mempool.go 1 66.67%
database/ffldb/db.go 5 91.23%
peer/peer.go 5 73.49%
Totals Coverage Status
Change from base Build 10411760109: -2.0%
Covered Lines: 29835
Relevant Lines: 53988

💛 - Coveralls

@guggero guggero requested a review from sputn1ck August 22, 2024 12:39
Copy link
Member

@davecgh davecgh left a 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
Copy link
Member

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.

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.

5 participants