Navigation Menu

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

Remove circular dependency issue between btcec/v2 and main package #1823

Merged
merged 5 commits into from Mar 10, 2022

Conversation

guggero
Copy link
Collaborator

@guggero guggero commented Mar 8, 2022

Fixes #1822.

After merging this PR we need to push two tags: chaincfg/chainhash/v1.0.0 and btcec/v2/v2.1.1.
And I think the two tags cannot be on the same commit (because go mod is just mean that way), so I'd suggest adding chaincfg/chainhash/v1.0.0 to 7cc824e and btcec/v2/v2.1.1 to 4ad74cd.

cc @Roasbeef.

@coveralls
Copy link

coveralls commented Mar 8, 2022

Pull Request Test Coverage Report for Build 1956823903

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 79.676%

Totals Coverage Status
Change from base Build 1954395449: 0.0%
Covered Lines: 1231
Relevant Lines: 1545

💛 - Coveralls

@guggero
Copy link
Collaborator Author

guggero commented Mar 8, 2022

I added an updated version of #1780 as a commit to this PR as well (since the btcutil/go.sum needed to be updated).

EDIT: Removed that commit again since the author of the original PR already pushed a fix.

@Roasbeef
Copy link
Member

Roasbeef commented Mar 8, 2022

I merged #1780, so I think this just needs a rebase now. I think we'll also need to tag new btcec (etc) versions after that prior (and this) merge, so we can bundle them after this lands.

@Roasbeef
Copy link
Member

Roasbeef commented Mar 8, 2022

After merging this PR we need to push two tags: chaincfg/chainhash/v1.0.0 and btcec/v2/v2.1.1.
And I think the two tags cannot be on the same commit (because go mod is just mean that way

Weird, I think we've done this in the past? Though maybe there's some other sneaky interaction here.

Copy link
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🦟

Changes look good to me, if we really want to then we can eliminate the spew dep, but that only relies on the std-lib, so it doesn't bring any new deps into the fray.

btcec/go.mod Show resolved Hide resolved
@Roasbeef
Copy link
Member

Roasbeef commented Mar 8, 2022

Needs aa rebase, then we can merge this and push out the new tags, then update the mod files in another PR.

@guggero
Copy link
Collaborator Author

guggero commented Mar 9, 2022

Rebased!

@guggero
Copy link
Collaborator Author

guggero commented Mar 9, 2022

Again, I think it is very important to push the tags for the correct commits! Otherwise we might point to a version that has a replace directive, creating more pain points.

Here are the updated commit hashes to tag:

@Roasbeef Roasbeef merged commit 425ed7c into btcsuite:master Mar 10, 2022
@guggero guggero deleted the go-mod-pain-relief branch March 10, 2022 08:34
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.

break cyclic module require from btcec/v2 and btcd
3 participants