deps(webrtc): don't include `webrtc` by default if `tokio` is enabled #4962

altonen · 2023-11-29T18:57:59Z

Description

Notes & open questions

Don't enable webrtc by default if tokio is enabled. Bug in cargo causes webrtc-related dependencies to be pulled even if webrtc is not enabled and causes issues downstream with incompatible dependencies.

Change checklist

I have performed a self-review of my own code
I have made corresponding changes to the documentation
I have added tests that prove my fix is effective or that my feature works
A changelog entry has been made in the appropriate crates

thomaseizinger

This looks good!

We can't really trust CI that much for backports because all sorts of stuff breaks with old Rust versions. Before we make the release, can you confirm that this PR works as intended with polkadot?

thomaseizinger · 2023-11-30T01:54:08Z

transports/webrtc/Cargo.toml

You'll need to bump the version of this crate to 0.4.0-alpha.5.

thomaseizinger · 2023-11-30T01:58:11Z

It seems that we break the MSRV with this update: https://github.com/libp2p/rust-libp2p/actions/runs/7037238673/job/19151620939?pr=4962. We don't normally break MSRV in patch versions but I think we might have to if we want to ship this.

@mxinden What do you think? I think it is not that bad too be honest. If somebody wants to stay on a lower MSRV, the can just not upgrade to the new patch release.

altonen · 2023-11-30T07:38:55Z

We don't support WebRTC right now in Polkadot but libp2p-webrtc get pulled anyway via the tokio feature of libp2p. This would only allow us to update schnorrkel. Is there a specific test you'd like me to run?

We're planning on bringing WebRTC support after the 0.52 upgrade has been merged

thomaseizinger · 2023-11-30T09:20:19Z

We don't support WebRTC right now in Polkadot but libp2p-webrtc get pulled anyway via the tokio feature of libp2p.

That seems like a bug! Are you using resolver = 2 in your workspace Cargo.toml? If you are using the default resolver then optional crates can be pulled in without you actually activating the feature.

altonen · 2023-11-30T09:37:41Z

We don't support WebRTC right now in Polkadot but libp2p-webrtc get pulled anyway via the tokio feature of libp2p.

That seems like a bug! Are you using resolver = 2 in your workspace Cargo.toml? If you are using the default resolver then optional crates can be pulled in without you actually activating the feature.

We are

I thought this was intentional and I tried working around with it but no luck. It's very easy to reproduce though, just creating a new project like this (no code):

[package]
name = "libp2p-webrtc-test"
version = "0.1.0"
edition = "2021"

[dependencies]
libp2p = { version = "0.51.3", features = ["tokio"], default-features = false }

Shows all kinds of webrtc-related dependencies in Cargo.lock but if you run cargo tree, there's nothing webrtc-related

thomaseizinger · 2023-12-01T06:49:49Z

I think you are running into the following cargo bug: rust-lang/cargo#10801.

This would only allow us to update schnorrkel.

Can you elaborate on how exactly this affects you? We don't depend on schnorrkel so how does this dependency ending up in Cargo.lock affect your ability to update?

thomaseizinger

I am somewhat hesitant to make a breaking change (MSRV) in a patch release if the actual motivation for this is just working around a cargo bug :(

I'll withdraw my approval for now and I'd like to understand better, what is actually going on here.

altonen · 2023-12-01T07:03:04Z

I think you are running into the following cargo bug: rust-lang/cargo#10801.

This would only allow us to update schnorrkel.

Can you elaborate on how exactly this affects you? We don't depend on schnorrkel so how does this dependency ending up in Cargo.lock affect your ability to update?

We're using an ancient version of schnorrkel (0.9.1 from three years ago) and would like to update to, according to my coworker's PR, to 0.11.3. If I run cargo check on the following empty project:

[package]
name = "libp2p-webrtc-test"
version = "0.1.0"
edition = "2021"

[dependencies]
libp2p = { version = "0.51.3", features = ["tokio"], default-features = false }
schnorrkel = { version = "0.11.3", features = ["preaudit_deprecated"], default-features = false }

I get the following error:

libp2p-webrtc-test >> cargo check                                                                                                                                                                                    git:master*
    Updating crates.io index
error: failed to select a version for `subtle`.
    ... required by package `aes-gcm v0.9.4`
    ... which satisfies dependency `aes-gcm = "^0.9.4"` of package `webrtc-srtp v0.9.0`
    ... which satisfies dependency `srtp = "^0.9.0"` of package `interceptor v0.8.0`
    ... which satisfies dependency `interceptor = "^0.8.0"` of package `webrtc v0.6.0`
    ... which satisfies dependency `webrtc = "^0.6.0"` of package `libp2p-webrtc v0.4.0-alpha.3`
    ... which satisfies dependency `libp2p-webrtc = "^0.4.0-alpha.3"` of package `libp2p v0.51.3`
    ... which satisfies dependency `libp2p = "^0.51.3"` of package `libp2p-webrtc-test v0.1.0 (/home/altonen/work/prototypes/libp2p-webrtc-test)`
versions that meet the requirements `>=2, <2.5` are: 2.4.1, 2.4.0, 2.3.0, 2.2.3, 2.2.2, 2.2.1, 2.1.1, 2.1.0, 2.0.0

all possible versions conflict with previously selected packages.

  previously selected package `subtle v2.5.0`
    ... which satisfies dependency `subtle = "^2.5.0"` of package `schnorrkel v0.11.3`
    ... which satisfies dependency `schnorrkel = "^0.11.3"` of package `libp2p-webrtc-test v0.1.0 (/home/altonen/work/prototypes/libp2p-webrtc-test)`

failed to select a version for `subtle` which could resolve this conflict

Updating webrtc to 0.8.0 would allow us to update schnorrkel.

thomaseizinger · 2023-12-01T07:17:13Z

Damn that is annoying. I think there might be a simpler fix though that is not a breaking change: We can remove the forwarding of the tokio feature to the libp2p-webrtc crate and instead unconditionally activate libp2p-webrtc/tokio when somebody activates webrtc. Because we only have an implementation using the tokio runtime anyway, somebody that wants the webrtc implementation must necessarily also use the tokio runtime so this is fine.

Doing that should remove the entire libp2p-webrtc dependency chain from your lockfile.

altonen · 2023-12-01T07:27:17Z

Like this?

diff --git a/libp2p/Cargo.toml b/libp2p/Cargo.toml
index f979366b..d36a4621 100644
--- a/libp2p/Cargo.toml
+++ b/libp2p/Cargo.toml
@@ -80,12 +80,12 @@ secp256k1 = ["libp2p-identity/secp256k1"]
 serde = ["libp2p-core/serde", "libp2p-kad?/serde", "libp2p-gossipsub?/serde"]
 tcp = ["dep:libp2p-tcp"]
 tls = ["dep:libp2p-tls"]
-tokio = ["libp2p-swarm/tokio", "libp2p-mdns?/tokio", "libp2p-tcp?/tokio", "libp2p-dns?/tokio", "libp2p-quic?/tokio", "libp2p-webrtc?/tokio"]
+tokio = ["libp2p-swarm/tokio", "libp2p-mdns?/tokio", "libp2p-tcp?/tokio", "libp2p-dns?/tokio", "libp2p-quic?/tokio"]
 uds = ["dep:libp2p-uds"]
 wasm-bindgen = ["futures-timer/wasm-bindgen", "instant/wasm-bindgen", "getrandom/js", "libp2p-swarm/wasm-bindgen"]
 wasm-ext = ["dep:libp2p-wasm-ext"]
 wasm-ext-websocket = ["wasm-ext", "libp2p-wasm-ext?/websocket"]
-webrtc = ["dep:libp2p-webrtc", "libp2p-webrtc?/pem"]
+webrtc = ["dep:libp2p-webrtc", "libp2p-webrtc?/pem", "libp2p-webrtc/tokio"]
 websocket = ["dep:libp2p-websocket"]
 yamux = ["dep:libp2p-yamux"]

thomaseizinger · 2023-12-01T07:34:07Z

I think so yes. Can you update this PR to that and then try if you can update schnorrkel once you [patch.crates-io] your libp2p dependency?

altonen · 2023-12-01T07:46:24Z

Yeah this works as well.

mxinden · 2023-12-01T21:51:41Z

@mxinden What do you think? I think it is not that bad too be honest. If somebody wants to stay on a lower MSRV, the can just not upgrade to the new patch release.

Fine by me. Though seems like you found a much better solution. Feel free to go ahead here. Thanks @thomaseizinger and @altonen.

thomaseizinger · 2023-12-01T22:40:28Z

Yeah this works as well.

Can you confirm that this PR solves your problem?

altonen · 2023-12-02T09:39:19Z

Yeah this works as well.

Can you confirm that this PR solves your problem?

Yes I confirmed it, this works for us.

thomaseizinger · 2023-12-03T23:56:44Z

Yeah this works as well.

Can you confirm that this PR solves your problem?

Yes I confirmed it, this works for us.

Awesome! Great to hear that we've found a solution. Can you bump the patch version of libp2p and write a short changelog entry?

I don't think this is worth / possible to forward-port to master because libp2p-webrtc is still in alpha and thus not included in libp2p anymore by default. Let me know if you think differently @mxinden !

This works around a bug in cargo which unconditionally pulls the dependencies even if the `webrtc` feature is not enabled and causes issues in downstream with other conflicting dependencies.

altonen · 2023-12-06T12:45:23Z

Friendly ping

mxinden · 2023-12-06T19:41:53Z

I don't think this is worth / possible to forward-port to master because libp2p-webrtc is still in alpha and thus not included in libp2p anymore by default. Let me know if you think differently @mxinden !

👍

thomaseizinger · 2023-12-06T22:57:20Z

Can you bump the patch version of libp2p

Damn, merged too quickly here. We forgot to actually bump the version.

thomaseizinger · 2023-12-06T23:05:14Z

This is now released: https://github.com/libp2p/rust-libp2p/releases/tag/libp2p-v0.51.4

altonen · 2023-12-07T06:29:46Z

Thanks Thomas and Max, really appreciate you accepting this patch. And sorry about forgetting to bump the version.

thomaseizinger changed the title ~~deps(webrtc): Update webrtc to 0.8.0~~ deps(webrtc): update webrtc to 0.8.0 Nov 30, 2023

thomaseizinger approved these changes Nov 30, 2023

View reviewed changes

transports/webrtc/Cargo.toml Outdated

Copy link

Contributor

thomaseizinger Nov 30, 2023

Choose a reason for hiding this comment

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

You'll need to bump the version of this crate to 0.4.0-alpha.5.

thomaseizinger requested changes Dec 1, 2023

View reviewed changes

altonen changed the title ~~deps(webrtc): update webrtc to 0.8.0~~ deps(webrtc): don't include webrtc by default if tokio is enabled Dec 1, 2023

Don't include webrtc into the tokio feature

faf496f

This works around a bug in cargo which unconditionally pulls the dependencies even if the `webrtc` feature is not enabled and causes issues in downstream with other conflicting dependencies.

thomaseizinger merged commit 2675484 into libp2p:v0.51 Dec 6, 2023
16 of 60 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

deps(webrtc): don't include `webrtc` by default if `tokio` is enabled #4962

deps(webrtc): don't include `webrtc` by default if `tokio` is enabled #4962

altonen commented Nov 29, 2023 •

edited by thomaseizinger

Loading

thomaseizinger left a comment

thomaseizinger Nov 30, 2023

thomaseizinger commented Nov 30, 2023 •

edited

Loading

altonen commented Nov 30, 2023

thomaseizinger commented Nov 30, 2023

altonen commented Nov 30, 2023

thomaseizinger commented Dec 1, 2023

thomaseizinger left a comment

altonen commented Dec 1, 2023

thomaseizinger commented Dec 1, 2023

altonen commented Dec 1, 2023

thomaseizinger commented Dec 1, 2023

altonen commented Dec 1, 2023

mxinden commented Dec 1, 2023

thomaseizinger commented Dec 1, 2023

altonen commented Dec 2, 2023

thomaseizinger commented Dec 3, 2023

altonen commented Dec 6, 2023

mxinden commented Dec 6, 2023

thomaseizinger commented Dec 6, 2023

thomaseizinger commented Dec 6, 2023 •

edited

Loading

altonen commented Dec 7, 2023

deps(webrtc): don't include webrtc by default if tokio is enabled #4962

deps(webrtc): don't include webrtc by default if tokio is enabled #4962

Conversation

altonen commented Nov 29, 2023 • edited by thomaseizinger Loading

Description

Notes & open questions

Change checklist

thomaseizinger left a comment

Choose a reason for hiding this comment

thomaseizinger Nov 30, 2023

Choose a reason for hiding this comment

thomaseizinger commented Nov 30, 2023 • edited Loading

altonen commented Nov 30, 2023

thomaseizinger commented Nov 30, 2023

altonen commented Nov 30, 2023

thomaseizinger commented Dec 1, 2023

thomaseizinger left a comment

Choose a reason for hiding this comment

altonen commented Dec 1, 2023

thomaseizinger commented Dec 1, 2023

altonen commented Dec 1, 2023

thomaseizinger commented Dec 1, 2023

altonen commented Dec 1, 2023

mxinden commented Dec 1, 2023

thomaseizinger commented Dec 1, 2023

altonen commented Dec 2, 2023

thomaseizinger commented Dec 3, 2023

altonen commented Dec 6, 2023

mxinden commented Dec 6, 2023

thomaseizinger commented Dec 6, 2023

thomaseizinger commented Dec 6, 2023 • edited Loading

altonen commented Dec 7, 2023

deps(webrtc): don't include `webrtc` by default if `tokio` is enabled #4962

deps(webrtc): don't include `webrtc` by default if `tokio` is enabled #4962

altonen commented Nov 29, 2023 •

edited by thomaseizinger

Loading

thomaseizinger commented Nov 30, 2023 •

edited

Loading

thomaseizinger commented Dec 6, 2023 •

edited

Loading