oneshot transitively depends on windows
#1736
Comments
|
What do you mean by "pull in"? AFAIK, cfg-conditional dependencies get recorded in the lockfile by design (such that you have a stable lockfile across platforms and such), but only built when the cfg is actually active. I'm not sure when they're downloaded. |
|
I'm guessing "pull in" means copied by That's annoying indeed. I think inlining should be okay, the crate itself is very small. Another option would be to implement the API ourselves using a Mutex like we had before. The main reason to pull in that crate was that it had a nice API, performance was secondary. I can take this on. Do you have a preference between the two? |
|
@jplatte ah yes, sorry; as bendk said: by cargo vendor and we have additional tooling making sure that we don't vendor some crates (and |
|
I forked the oneshot repo and made this commit that removes the I think we can use this to keep the dependency simple in UniFFI and avoid blowing up the vendor directory in moz-central. |
This avoids pulling in the loom dependency.
This avoids pulling in the loom dependency.
This is needed to vendor this code into moz-central, see mozilla/uniffi-rs#1736 for details.
This brings in the `into_raw` and `from_raw` methods that we may want to use to pass oneshot senders over the FFI. It still uses my patched version to avoid mozilla#1736.
This brings in the `into_raw` and `from_raw` methods that we may want to use to pass oneshot senders over the FFI. It still uses my patched version to avoid #1736.
|
I'm currently in the process of vendoring the UniFFI crates for use in Android. The forked oneshot crate is making my life a little interesting here. Not a blocker, but I thought you might be interested in hearing about it: https://r.android.com/3055962. Basically, I wanted to enable the unit tests in oneshot-uniffi, and to do this I renamed the crate back to oneshot. I then realized that the unit tests don't actually run in https://github.com/bendk/oneshot, which is sub-optimal. I'll probably just disable the tests for now. |
|
This has been a real pain to maintain. Maybe we should create our own simple oneshot implementation using Mutex. The performance would be slower, but I'm not sure it would really matter. The main reason I pulled in that crate was that it had a nice API, not the tricks it did to avoid locking. |
|
Nice APIs are important 🙂 I also very much like the idea of exporting complexity from my projects — if the code goes into another crate, then I hopefully won't have to pay much attention to maintaining it. But here it seems like this tradeoff might not pay off? Android is a bit special in the way it vendors Rust crates: we only want one version of each library, to avoid making the system images bigger than absolutely necessary. To do this, we ignore the Cargo version bounds when importing the crates(!) and instead rely on the unit tests to tell us if things work. Enabling tests for the vendored crates helps us know that they work together, despite us using an unexpected mix of versions. Since this is a small crate, it's probably not a small loss to disable the tests for it so I can proceed with the vendoring. |
Implemented our own oneshot channel using a Mutex. It's not quite as efficient as the `oneshot` one, but I think it should be fine for our purposes. My gut feeling is that the loss of overhead is neglibable compared the other existing overhead that UniFFI adds. The API of the new oneshot is basically the same, except send/recv are not failable.
|
Yeah, I'm feeling like the tradeoff isn't worth it. It's very simple to implement this on our own and I don't expect this code to need much updating: #2096. |
|
Thanks @bendk! I'll un-vendor the oneshot-uniffi crate from AOSP for the next release. That will clean things up a little. |
|
Closing as we dropped the dependency now. |
Implemented our own oneshot channel using a Mutex. It's not quite as efficient as the `oneshot` one, but I think it should be fine for our purposes. My gut feeling is that the loss of overhead is neglibable compared the other existing overhead that UniFFI adds. The API of the new oneshot is basically the same, except send/recv are not failable.
Implemented our own oneshot channel using a Mutex. It's not quite as efficient as the `oneshot` one, but I think it should be fine for our purposes. My gut feeling is that the loss of overhead is neglibable compared the other existing overhead that UniFFI adds. The API of the new oneshot is basically the same, except send/recv are not failable.
Implemented our own oneshot channel using a Mutex. It's not quite as efficient as the `oneshot` one, but I think it should be fine for our purposes. My gut feeling is that the loss of overhead is neglibable compared the other existing overhead that UniFFI adds. The API of the new oneshot is basically the same, except send/recv are not failable.
|
I just wanted to make you aware that I just released a version of |
d4a6e65 pulled in the
oneshotdependency, which itself depends on loom under acfg: https://github.com/faern/oneshot/blob/80f4c3f0ef6bf2051bf3e59166194fde3c1d2699/Cargo.toml#L26-L27loomdepends on thewindowscrate which we don't want to pull into m-c.unfortunately the dependency resolution is not smart enough and does pull that in, making m-c fail on an import of uniffi with this.
The hacky way is to patch
oneshot(remove the testing parts) locally.I wonder if there's a better way to handle that in uniffi instead?
Definitely annoying but maybe a short term solution is to inline that crate?
cc @bendk
The text was updated successfully, but these errors were encountered: