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

Expose choice of supported tonic version #56

Merged
merged 1 commit into from
Dec 20, 2024

Conversation

dtolnay
Copy link
Contributor

@dtolnay dtolnay commented Dec 16, 2024

The current way that tokio-vsock tries to support multiple tonic versions is problematic, and is a regression from 0.6.0 (which used version = "0.12.0" for the tonic dependency).

tonic = { version = ">= 0.5.0, < 0.13.0", optional = true }

This does not mean "tokio-vsock supports every tonic version from 0.5 through 0.12". It means "tokio-vsock supports a single random version of tonic between 0.5 and 0.12 according to Cargo's whim". Cargo does not expose any public way to control what choice of tonic would be used by this dependency.

One possible way the problems would manifest (there are many) is:

  • Somebody writes a Cargo.toml depending on tokio-vsock = { version = "0.6", features = ["tonic"] } and tonic = "0.11". In this situation Cargo (probably) resolves the tonic dependency inside tokio-vsock to 0.11 during local development.

  • They write Rust code counting on tonic 0.11's Connected impl to exist for VsockStream, such as by calling .connect_info() with use tonic::transport::server::Connected; referring to tonic 0.11.

  • They test their code extensively, locally and in CI, and everything works.

  • They publish to crates.io as a library.

  • Somebody else has a different project already using tonic = "0.12", and they run cargo add to add a new dependency on the library from the previous step.

  • The library, despite having been extensively tested in CI, now does not compile because Cargo decided that it depends on tonic 0.11 and tokio-vsock 0.6 while tokio-vsock 0.6 now depends on tonic 0.12, and the necessary Connected impl is absent.

This is a regression from tokio-vsock 0.6.0 because at least in 0.6.0 I could reliably use tokio-vsock with tonic 0.12 and write Rust code counting on that Connected impl to exist. In tokio-vsock master, there is no longer any way to use the Connected impl correctly. It may or may not exist from moment-to-moment depending on what else is going on in the project's dependency graph.

This PR makes tokio-vsock correctly support a range of tonic versions. If built with tokio-vsock = { features = ["tonic012"] } then tonic 0.12's Connected trait is guaranteed to be implemented.

The current way that tokio-vsock tries to support multiple tonic
versions is problematic, and is a regression from 0.6.0 (which used
`version = "0.12.0"` for the tonic dependency).

    tonic = { version = ">= 0.5.0, < 0.13.0", optional = true }

This does not mean "tokio-vsock supports every tonic version from 0.5
through 0.12". It means "tokio-vsock supports a *single random* version
of tonic between 0.5 and 0.12 according to Cargo's whim". Cargo does not
expose any public way to control what choice of tonic would be used by
this dependency.

One possible way the problems would manifest (there are many) is:

- Somebody writes a Cargo.toml depending on `tokio-vsock = { version =
  "0.6", features = ["tonic"] }` and `tonic = "0.11"`. In this situation
  Cargo (probably) resolves the tonic dependency inside tokio-vsock to
  0.11 during local development.

- They write Rust code counting on tonic 0.11's `Connected` impl to
  exist for `VsockStream`, such as by calling `.connect_info()` with
  `use tonic::transport::server::Connected;` referring to tonic 0.11.

- They test their code extensively, locally and in CI, and everything
  works.

- They publish to crates.io as a library.

- Somebody else has a different project already using `tonic = "0.12"`,
  and they run `cargo add` to add a new dependency on the library from
  the previous step.

- The library, despite having been extensively tested in CI, now does
  not compile because Cargo decided that it depends on tonic 0.11 and
  tokio-vsock 0.6 while tokio-vsock 0.6 now depends on tonic 0.12, and
  the necessary `Connected` impl is absent.

This is a regression from tokio-vsock 0.6.0 because at least in 0.6.0 I
could reliably use tokio-vsock with tonic 0.12 and write Rust code
counting on that `Connected` impl to exist. In tokio-vsock master, there
is no longer any way to use the `Connected` impl correctly. It may or
may not exist from moment-to-moment depending on what else is going on
in the project's dependency graph.

This commit makes tokio-vsock correctly support a range of tonic
versions. If built with `tokio-vsock = { features = ["tonic012"] }` then
tonic 0.12's `Connected` trait is guaranteed to be implemented.

Signed-off-by: David Tolnay <[email protected]>
@jalil-salame
Copy link
Contributor

Thanks for catching this! Thankfully we didn't publish this yet c:

We probably need to adjust #55 to use cargo hack instead of -Z direct-minimum-dependencies after this.

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.

Are these features additive? Gut feeling says yes, but I'm not sure

@dtolnay
Copy link
Contributor Author

dtolnay commented Dec 16, 2024

Officially they are additive, and docs.rs will build them all.

tokio-vsock/Cargo.toml

Lines 33 to 34 in cdb58ef

[package.metadata.docs.rs]
all-features = true

Technically they are not additive, in the sense that it is possible to write contrived Rust code that compiles if a feature is off and does not compile if a feature is on, but this is true of almost every feature in every library.

Example:

// [dependencies]
// tokio-vsock = { features = ["tonic012"] }
// tonic011 = { package = "tonic", version = "0.11" }
// tonic012 = { package = "tonic", version = "0.12" }

use tokio_vsock::VsockStream;
use tonic011::transport::server::Connected as _;
use tonic012::transport::server::Connected as _;

fn repro(vs: VsockStream) {
    let _ = vs.connect_info(); // ambiguous if tokio-vsock/tonic011 enabled
}

@cryo28
Copy link
Contributor

cryo28 commented Dec 16, 2024

Thanks @dtolnay for doing correctly what I tried to do incorrectly in my earlier PR.

@jalil-salame jalil-salame merged commit 42901f2 into rust-vsock:master Dec 20, 2024
2 checks passed
@Tim-Zhang
Copy link
Contributor

Hi, @dtolnay thank you for your contribution to tokio-vsock. I have invited you as a developer. If you are interested, you are welcome to join us.

@dtolnay dtolnay deleted the tonic branch December 20, 2024 12:14
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]>
facebook-github-bot pushed a commit to facebook/sapling that referenced this pull request Jan 13, 2025
Summary: The required [changes](rust-vsock/tokio-vsock#56) to allow most recent version of tokio-vsock crate to coexist with tonic 0.9.2 have been [landed](rust-vsock/tokio-vsock#57) in the upstream. Let's use this version and remove the temporary override

Reviewed By: dtolnay

Differential Revision: D68104235

fbshipit-source-id: 40d0e78f1c54ef9cb4f95f46b9b8293d3457c379
facebook-github-bot pushed a commit to facebookexperimental/rust-shed that referenced this pull request Jan 13, 2025
Summary: The required [changes](rust-vsock/tokio-vsock#56) to allow most recent version of tokio-vsock crate to coexist with tonic 0.9.2 have been [landed](rust-vsock/tokio-vsock#57) in the upstream. Let's use this version and remove the temporary override

Reviewed By: dtolnay

Differential Revision: D68104235

fbshipit-source-id: 40d0e78f1c54ef9cb4f95f46b9b8293d3457c379
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