oneshot transitively depends on `windows`

[badboy]

https://github.com/mozilla/uniffi-rs/commit/d4a6e652a122bd0a669cf002c99c6026b8f60997 pulled in the oneshot dependency, which itself depends on loom under a cfg: https://github.com/faern/oneshot/blob/80f4c3f0ef6bf2051bf3e59166194fde3c1d2699/Cargo.toml#L26-L27 loom depends on the windows crate 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

[jplatte]

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.

[bendk]

I'm guessing "pull in" means copied by cargo vendor.

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?

[badboy]

@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 windows is huge so it's currently not allowed).

[bendk]

I forked the oneshot repo and made this commit that removes the loom target: https://github.com/bendk/oneshot/commit/1f3c657c8073aec4f0b6ebac7be33b4851644745

I think we can use this to keep the dependency simple in UniFFI and avoid blowing up the vendor directory in moz-central.

[mgeisler]

Hi @badboy and @bendk,

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.

[bendk]

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.

[mgeisler]

Nice APIs are important :slightly_smiling_face: 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.

[bendk]

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: https://github.com/mozilla/uniffi-rs/pull/2096.

[mgeisler]

Thanks @bendk! I'll un-vendor the oneshot-uniffi crate from AOSP for the next release. That will clean things up a little.

[badboy]

Closing as we dropped the dependency now.

[faern]

I just wanted to make you aware that I just released a version of oneshot (0.1.8) that does not depend on loom any longer. This was fixed in https://github.com/faern/oneshot/pull/45. Just FYI