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

Rust-bins: Upgrade tonic to v0.12 for v0.1.9 #7598

Conversation

nepet
Copy link
Collaborator

@nepet nepet commented Aug 21, 2024

Upgrade tonic to v0.12 for rust bins v0.1.9
Description
This PR upgrades the tonic dependency of the rust libraries to v0.12. Latest version used was v0.8. Tonic had some major bugfixes in between these versions. We need to upgrade tonic for downstream consumers of our libs to be able to upgrade tonic as well.

This PR also introduces a new versioning scheme where we bump the minor semver version on any breaking api change such as the removal of deprecated calls as well as changes to the requirements of fields.

The whole rust lib stack cln-grpc cln-rpc cln-plugin and cln-grpc-plugin is now on v0.2.x, on top of tag cln v24.02.2 for the underlying message scheme.

@nepet nepet requested a review from cdecker as a code owner August 21, 2024 13:13
@nepet nepet changed the title Cln rust bins v0.3 upgrade tonic version for v0.1.9 Rust-bins: Upgrade tonic to v0.12 for v0.1.9 Aug 21, 2024
@nepet nepet force-pushed the cln-rust-bins-v0.3_upgrade-tonic-version branch from 7c8844f to bf84b61 Compare September 10, 2024 15:19
@nepet nepet changed the base branch from cln-rust-bins-v0.3 to master September 10, 2024 15:20
@nepet nepet force-pushed the cln-rust-bins-v0.3_upgrade-tonic-version branch 2 times, most recently from 0820036 to bdd9e33 Compare September 10, 2024 15:36
@nepet
Copy link
Collaborator Author

nepet commented Sep 10, 2024

This PR changed a bit now:

We don't want to keep different branches for different minor versions of the rust crates for now, just update on master.
This PR now does:

  • Bump tonic: v0.8 --> v0.12
  • Bump minor version of all cln-* crates to give us more flexibility going on: a depenency update or a bug fix may now just be a patch while api incompatibilities like removed or changed api calls can be signaled by an increasing minor version.

If you are ok with these changes @daywalker90, #7623 becomes obsolete.

@daywalker90
Copy link
Contributor

I'm fine with it, but i think @cdecker had concerns with upgrading dependencies when i made a PR for it.

@nepet
Copy link
Collaborator Author

nepet commented Sep 10, 2024

I'm fine with it, but i think @cdecker had concerns with upgrading dependencies when i made a PR for it.

Yeah, I remember that upgrading some crate led to a lot of pain in the past, but I can't remember which crate it was. Upgrading tonic, on the other hand, brings some fixes that help us get better transport errors and is both wanted and needed by Greenlight, so this upgrade should be fine :)

@nepet
Copy link
Collaborator Author

nepet commented Sep 13, 2024

So, I could locally reproduce the error CI runs into. The error appears after we try to establish a connection with a certificate unknown to the cln-grpc plugin. From what we can tell, the plugin stops listening on the grpc port after a tls hanshake error.

I am going to invert the commit that upgrades tonic to v0.12.2 which caused the error to appear. We still should bump the version to a new minor version giving us the oportunity to uprade tonic once this problem got solved.

@nepet nepet force-pushed the cln-rust-bins-v0.3_upgrade-tonic-version branch from bdd9e33 to 6a463f0 Compare September 13, 2024 13:29
Tonic had some breaking changes since 0.8. We want to be closer to newer
versions for downstream consumers to be able to benefit from changes in tonic.

Signed-off-by: Peter Neuroth <[email protected]>
We bump the version to minor version 0.2 to allow ourselves to bump on
patch level when we need to fix a bug or update a dependency.

Changelog-Changed: Plugins: `cln-grpc` Upgrade tonic version and
introduce new versioning scheme.

Signed-off-by: Peter Neuroth <[email protected]>
@nepet nepet force-pushed the cln-rust-bins-v0.3_upgrade-tonic-version branch from 6a463f0 to 2412fac Compare September 17, 2024 05:23
Copy link
Member

@cdecker cdecker left a comment

Choose a reason for hiding this comment

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

ACK

@ShahanaFarooqui ShahanaFarooqui merged commit 0a6870f into ElementsProject:master Sep 22, 2024
38 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants