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

Relax tonic version requirements #53

Merged
merged 1 commit into from
Dec 9, 2024
Merged

Conversation

cryo28
Copy link
Contributor

@cryo28 cryo28 commented Dec 6, 2024

Currently tonic_support.rs implements a single trait Connect from tonic (impl Connected for VsockStream)

This trait is relatively stable on the tonci side. It was modified last by e5e311853bff347355722bc829d40f54e8954aee 3.5 years ago and was included into tonic v0.5.0 (see git tag --contains e5e311853bff347355722bc829d40f54e8954aee). The trait has not been modified since then up to the current tonic tag of v0.12.3.

So, let's relax the dependency version requirements to make the most recent version of tokio-vsock usable with older versions of tonic.

Motivation for this change is that in complex projects it is not easy to move between arbitrary versions of tonic (which is a major dependency) to be able to use the most recent version of tokio-vsock (or use it with tonic v.0.9.x, that is not support by versions of tonic-vsock).

Given that tonic-vsock's dependency on tonic is very limit (impls a single relatively stable trait), relaxing the version should be safe.

Copy link
Contributor

@jalil-salame jalil-salame left a comment

Choose a reason for hiding this comment

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

LGTM

I'd like to verify that all versions major versions of tonic (covered by this change) work on CI, or at least that both 0.12 and 0.5 work.

@qwandor or someone else knows what/how to do this? I remember an unstable cargo flag that selects the minimum version of all dependencies, but I don't think we run/want to run nightly on CI.

Cargo.toml Outdated Show resolved Hide resolved
@cryo28 cryo28 force-pushed the master branch 2 times, most recently from fcc45fa to 38b15ff Compare December 6, 2024 23:39
@cryo28 cryo28 requested a review from jalil-salame December 7, 2024 00:28
@jalil-salame
Copy link
Contributor

Sorry about the delay, I missed the GH notifications, you need to fixup the commit message for CI to pass:

wrap the body of the commit message at 72 characters, and signoff the commit (git commit --amend --no-edit --sign-off)

Currently tonic_support.rs implements a single trait Connect from tonic
(impl Connected for VsockStream). This trait is relatively stable on
the tonic side. It was modified last by
e5e311853bff347355722bc829d40f54e8954aee 3.5 years ago and was included
into tonic v0.5.0:
(see `git tag --contains e5e311853bff347355722bc829d40f54e8954aee`).

The trait has not been modified since then up to the current tonic tag
of v0.12.3. So, let's relax the dependency version requirements to
make the most recent version of tokio-vsock usable with older versions
of tonic. Motivation for this change is that in complex projects it
is not easy to move between arbitrary versions of tonic
(which is a major dependency) to be able to use the most recent
version of tokio-vsock (or use it with tonic v.0.9.x, that is not
supported by versions of tonic-vsock). Given that tonic-vsock's
dependency on tonic is very limit (impls a single relatively stable
trait), relaxing the version should be safe.

Signed-off-by: Artem Ignatyev <[email protected]>
@jalil-salame jalil-salame merged commit ae7a784 into rust-vsock:master Dec 9, 2024
2 checks passed
@cryo28
Copy link
Contributor Author

cryo28 commented Dec 9, 2024

@jalil-salame Thanks for accepting the changes. Is it possible to release 0.6.1 so we can avoid depending on a github commit and can pull the package from crates.io? Let me know if you'd prefer me to prepare another pull request with the version bump?

facebook-github-bot pushed a commit to facebook/sapling that referenced this pull request Dec 13, 2024
Summary:
1. Added `tonic-conn` feature of tokio-vsock
2. This caused problems due to version mismtach described [in this post](https://fb.workplace.com/groups/rust.language/posts/27494647173490537)
3. Relaxed tonic dependency version [in the upstream](rust-vsock/tokio-vsock#53)
4. Now waiting for upstream to release 0.6.1 or higher. So, pointing Cargo.toml at a specific commit in the meanwhile

Differential Revision: D66719971

fbshipit-source-id: dc5587769877d07cd3bbccbb6faa1be01d6ad1db
facebook-github-bot pushed a commit to facebookexperimental/rust-shed that referenced this pull request Dec 13, 2024
Summary:
1. Added `tonic-conn` feature of tokio-vsock
2. This caused problems due to version mismtach described [in this post](https://fb.workplace.com/groups/rust.language/posts/27494647173490537)
3. Relaxed tonic dependency version [in the upstream](rust-vsock/tokio-vsock#53)
4. Now waiting for upstream to release 0.6.1 or higher. So, pointing Cargo.toml at a specific commit in the meanwhile

Differential Revision: D66719971

fbshipit-source-id: dc5587769877d07cd3bbccbb6faa1be01d6ad1db
@jalil-salame
Copy link
Contributor

@cryo28 sorry about the delay, I'd like to add some tests before releasing, so I'll work on that first.

@cryo28
Copy link
Contributor Author

cryo28 commented Dec 15, 2024

@cryo28 sorry about the delay, I'd like to add some tests before releasing, so I'll work on that first.

@jalil-salame No worries. I really appreciate it and I can wait!

Tim-Zhang added a commit to Tim-Zhang/tokio-vsock that referenced this pull request Dec 20, 2024
Bump the major version for API has changed in rust-vsock#54 and features
removed in rust-vsock#56

Also this release includes rust-vsock#53 and rust-vsock#55

Signed-off-by: Tim Zhang <[email protected]>
Tim-Zhang added a commit to Tim-Zhang/tokio-vsock that referenced this pull request Dec 20, 2024
Bump the major version for rust-vsock#54 which changed API and rust-vsock#56 which removed features.

Also this release includes rust-vsock#53 and rust-vsock#55.

Signed-off-by: Tim Zhang <[email protected]>
@Tim-Zhang Tim-Zhang mentioned this pull request Dec 20, 2024
Tim-Zhang added a commit to Tim-Zhang/tokio-vsock that referenced this pull request Dec 20, 2024
Bump the major version for rust-vsock#54 which changed API and rust-vsock#56
which removed features.

Also this release includes rust-vsock#53 and rust-vsock#55.

Signed-off-by: Tim Zhang <[email protected]>
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