Provide more external type information to FfiType::RustBuffer
#2195
Conversation
In service of Java bindgen being able to generate fully qualified `RustBuffer`s when necessary.
`uniffi-bindgen-java` is external to the uniffi repo, so the fixtures/examples are all `dev-dependencies`, which aren't built on a call to `cargo build`. `cargo test --no-run` causes them to be built but doesn't cause a run of tests in place. `.no_deps()` seems like it has a similar reaction as an external bindings generator, it ignores dependencies in the cargo metadata, which is where all the fixtures/examples used in tests cdylibs will be.
skeet70
force-pushed
the
thread-external-type-metadata-rustbuffer
branch
from
ec0167b
to
310aa55
Compare
There was a problem hiding this comment.
It might make sense to split this so the 2 issues being dealt with here can be considered independently.
Regarding no_deps, I'm not sure this command-line flag is the way forward. As you will note in library_mode, the intent is that cargo metadata support can be disabled by a feature, so even at face value, this option existing in that scenario seems odd. But more generally, the longer term idea is that people who disable that feature (because they don't use cargo) probably still need the capability of supplying the map so the crate source can be located. ie, we might be better doing what this comment says.
If that capability existed, then maybe our test code could itself just invoke cargo metadata however it wants, do the simple parsing and pass the resulting map in.
I must however admit that I'm mildly concerned this no_deps() might cause other breakage - the fact it doesn't break uniffi itself is one thing, but if it ends up causing breakage for other build environments we have a bit of a problem and might need to rethink it. In other words, maybe the issue we are having here might just be because it's a non-trivial build environment and the problem will actually turn out to be common. I guess I need to at least see what happens in our own build environment!
| @@ -159,7 +159,8 @@ fn get_cargo_metadata() -> Metadata { | |||
|
|
|||
| fn get_cargo_build_messages() -> Vec<Message> { | |||
| let mut child = Command::new(env!("CARGO")) | |||
| .arg("build") | |||
| .arg("test") | |||
| .arg("--no-run") | |||
There was a problem hiding this comment.
Could this be build --profile=dev? That seems to capture the intent better to me.
There was a problem hiding this comment.
It was already build (without --release), which should've defaulted to --profile=dev right? cdylibs of uniffi-bindgen-java dev deps weren't found on creation of the TestHelper as it was. cargo build --profile=test might work, but that feels a little less straightforward than test --no-run to me personally.
| pub name: String, | ||
| pub module_path: String, | ||
| pub namespace: String, | ||
| } |
There was a problem hiding this comment.
This makes a lot of sense to me, but I know that @mhammond has some plans for external types. Would this change conflict with them?
Hopefully at some point we can consolidate module_path and namespace, but it makes sense to me to have 2 fields for now since Type::External also have 2 fields.
There was a problem hiding this comment.
I do want to kill Type::External and replace it with the actual type but that's my problem :)
I don't get why we need both namespace and module_path though? If the name is to the Rust ffi function namespace might be problematic - that's not known by the udl/scaffolding, so gets fixed for the binding generation - meaning rust code generated from udl might have a different value than the bindings get.
ExternalFfiMetadata or similar is probably a better name so it's not confused as a uniffi_meta struct.
Not that you should add it now, but I wonder if you will ever strike a RustArcPtr ever needing a fq-name too?
There was a problem hiding this comment.
The namespace and module_path are needed to generate the qualified type path for RustBuffer in Java. The module_path is used to get the crate to look up in config.external_packages, and the namespace is needed for the default if nothing is found.
I haven't yet needed a fully qualified RustArcPtr and looking at how it's used I don't think I will, but it's certainly possible.
There was a problem hiding this comment.
Right, same as us :) But yeah, that namespace probably isn't going to work in many cases - in the case it does (eg, "single crate with udl") namespace is probably module path anyway. But yeah, I'm totally fine with carrying that all around, I just long for a time when it's less muddy.
Maybe the test code could even build the map itself and avoid the cargo metadata call. That could lead to some speedups. |
This continues the cargo_metadata feature work but making the execution of cargo_metadata the resonsibility of the uniffi_bindgen callers rather that executing it implicitly. This means the CLI, which in-turn means the top-level uniffi crate also gets a `cargo-metadata` feature. This reverts what we did to fix mozilla#2183 - by making `no_deps` the default, it means we will be unable to support reuse of UniFFI components, because it means we only support all components being in the same workspace. While this is a common use-case, it's not the only use-case we want to support. So grabbing all dependencies from cargo_metadata is again the default, but there's a new command-line option to avoid it. It also replaces some of mozilla#2195.
This continues the cargo_metadata feature work but making the execution of cargo_metadata the resonsibility of the uniffi_bindgen callers rather that executing it implicitly. This means the CLI, which in-turn means the top-level uniffi crate also gets a `cargo-metadata` feature. This reverts what we did to fix #2183 - by making `no_deps` the default, it means we will be unable to support reuse of UniFFI components, because it means we only support all components being in the same workspace. While this is a common use-case, it's not the only use-case we want to support. So grabbing all dependencies from cargo_metadata is again the default, but there's a new command-line option to avoid it. It also replaces some of #2195.
All the currently supported official languages support some form of aliasing so nothing output changes for them. For languages without aliasing (ie Java), this allows the fully qualified type name to be generated in place where necessary.
The
uniffi_testingswitch fromcargo buildtocargo test --no-runincludes dev dependencies in the message tree, which is needed for external binding authors where the uniffi fixtures are included as dev dependencies (vs in the same workspace as they are here). Makingno_depsduring component discovery is for the same problem, deps are needed when discovering externalcdylibsto test with.uniffi-bindgen-javadoes test these changes by its test suite running at all, this fixes a regression (from our POV) introduced in this commit. If there are tests I can write to cover both use cases (#2183 and ours), I can do it given direction.