signalapp / ringrtc

GNU Affero General Public License v3.0
547 stars 133 forks source link

ringrtc tracks libsignal on git, introduces duplicate dependency #55

Open rubdos opened 4 months ago

rubdos commented 4 months ago

ringrtc depends (currently) on:

zkgroup = { git = "https://github.com/signalapp/libsignal", tag = "v0.37.0" }

Signal Android depends (currently) on:

version("libsignal-client", "0.51.1")

On applications that use FFI, this doesn't really matter, because ringrtc and libsignal get pulled in independently. However, Rust applications that pull in ringrtc and libsignal in the same Rust dependency tree yield a conflict on cargo doc:

error: There are multiple `zkgroup` packages in your project, and the specification `zkgroup` is ambiguous.
Please re-run this command with one of the following specifications:
  https://github.com/signalapp/libsignal#zkgroup@0.9.0
  https://github.com/signalapp/libsignal#zkgroup@0.9.0

Keeping the ringrtc git tag version spec in sync with apps is, in my opinion, only a band-aid here. Releasing on crates.io (https://github.com/signalapp/libsignal/issues/490) would probably resolve this correctly, because it allows to track the versions of the actual crates, instead of depending on a git tag. I'll submit a PR for bumping libsignal on this repo now.

I'm not sure whether I should post this issue here or on libsignal itself. It might make more sense over there...

jrose-signal commented 4 months ago

Teeeechnically, since we haven't been bumping the version for the zkgroup crate and changes usually aren't breaking, I would expect you to be able to [patch] around this situation. But it is kind of annoying, yeah, and I haven't actually tried it.

rubdos commented 4 months ago

would expect you to be able to [patch] around this situation

I think the [patch] section does not allow disambiguating which zkgroup reference to patch, although in my case "patch-em-all" would be fine. We can currently just use 0.51.1; on the next libsignal-protocol bump I'll test this out.

rubdos commented 2 months ago

Another effect:

error: failed to resolve patches for `https://github.com/signalapp/libsignal/`

Caused by:
  patch for `zkgroup` in `https://github.com/signalapp/libsignal/` points to the same source, but patches must point to different sources

There's a trick though:

[patch."https://github.com/signalapp/libsignal/"]
# The source and target refs are the same URL. If we add another / in there,
# cargo sees this as a valid patch.  Very ugly workaround.
# https://github.com/rust-lang/cargo/issues/5478#issuecomment-1055365283
zkgroup = { git = "https://github.com/signalapp//libsignal", tag = "v0.56.1" }

... but that duplicates all transitive dependencies, yielding to other conflicts. Not ideal for now...

jrose-signal commented 2 months ago

Definitely not ideal. As a workaround, though, you could patch your own dependencies the same way, though, correct?

rubdos commented 2 months ago

Definitely not ideal. As a workaround, though, you could patch your own dependencies the same way, though, correct?

Yes, correct indeed, that's what I went with for now:

[patch."https://github.com/signalapp/libsignal/"]
# The source and target refs are the same URL. If we add another / in there,
# cargo sees this as a valid patch.  Very ugly workaround.
# https://github.com/rust-lang/cargo/issues/5478#issuecomment-1055365283
zkgroup = { git = "https://github.com/signalapp//libsignal", tag = "v0.56.1" }
libsignal-core = { git = "https://github.com/signalapp//libsignal", tag = "v0.56.1" }
signal-crypto = { git = "https://github.com/signalapp//libsignal", tag = "v0.56.1" }