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

chore!: migrate to cometbft fork #1066

Merged
merged 1 commit into from
Aug 24, 2023
Merged

chore!: migrate to cometbft fork #1066

merged 1 commit into from
Aug 24, 2023

Conversation

cmwaters
Copy link
Contributor

This ultimately changes the go.mod to using cometbft instead of tendermint which we will be using for the v0.37 release and is what the sdk depends on in v0.47

@cmwaters cmwaters marked this pull request as ready for review August 23, 2023 09:10
Copy link
Member

@evan-forbes evan-forbes left a comment

Choose a reason for hiding this comment

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

we've changed these to tendermint -> lazyledger-core -> celestia-core -> tendermint -> comet

did not expect to change these so frequently 😆

// https://github.com/tendermint/tendermint/issues/1281
// https://github.com/cometbft/cometbft/issues/1281
Copy link
Member

@evan-forbes evan-forbes Aug 23, 2023

Choose a reason for hiding this comment

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

fwiw I'm don't think its worth going through and finding each broken link as I don't think they're super useful and we can just manually fix the links adhoc if we really need to

In particular, [tendermint/tendermint#6196](https://github.com/tendermint/tendermint/pull/6196) and previous changes that it depends on.
In particular, [tendermint/tendermint#6196](https://github.com/cometbft/cometbft/pull/6196) and previous changes that it depends on.
Copy link
Member

Choose a reason for hiding this comment

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

explicit links on the other hand (like http:// whatever) we might actually want to fix

that or disable the dead link linter

Copy link
Collaborator

@rootulp rootulp left a comment

Choose a reason for hiding this comment

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

Overall LGTM. I scrolled through the diff and most of it seems like fixing the import statements. Agreed w/ Evan on the broken links

@@ -1,4 +1,4 @@
module github.com/tendermint/tendermint
module github.com/cometbft/cometbft
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

When this PR lands in a release, we should call out in release notes that consumers of this library (i.e. celestia-app and celestia-node) need to change the replace statement in go.mod

@@ -138,7 +138,7 @@ func (r *ReqRes) InvokeCallback() {
// marked done and SetCallback is called before calling GetCallback as that
// will invoke the callback twice and create a potential race condition.
//
// ref: https://github.com/tendermint/tendermint/issues/5439
// ref: https://github.com/cometbft/cometbft/issues/5439
Copy link
Collaborator

Choose a reason for hiding this comment

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

[not blocking] Note this is now a broken link. I think we can fix the next time we pull upstream CometBFT code changes because CometBFT still uses the valid link.

See https://github.com/cometbft/cometbft/blob/8e6e6ca0bcc732747e10020be2c5e9f3779e6b0c/abci/client/client.go#L116-L121

@cmwaters
Copy link
Contributor Author

Yeah I'm aware of the broken links and am aware that in doing this quickly I have probably created a few broken links. I don't think this is problematic because I don't think there is anyone using our fork as a reference of some information (other than the ADRs).

I can use a link checker and go through and correct the places were I mistakenly changed the url in a different PR

@cmwaters cmwaters merged commit ca1411a into main Aug 24, 2023
14 checks passed
@cmwaters cmwaters deleted the cal/migrate-cometbft-fork branch August 24, 2023 10:31
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.

3 participants