Bug 2018944 - glean-sym - a Rust API built on top of the Glean UniFFI C FFI#3426
Bug 2018944 - glean-sym - a Rust API built on top of the Glean UniFFI C FFI#3426
Conversation
dab03d7 to
23adcc4
Compare
23adcc4 to
a6d1ef4
Compare
e1571f1 to
6a7603a
Compare
48eeb4d to
9b3dd70
Compare
…ated code over FFI This will be used later by glean-sys to build a Rust API via C FFI.
…C FFI This loads `libxul.so` (currently hard-coded), looks up the required symbols and wraps that in a nice Rust API. Other crates can depend on `glean-sym`, without needing to have Glean in the same library.
not done yet
…vokes functions in them
9b3dd70 to
85d08d6
Compare
|
I would like to land this (experimentally) sometime soon. The plan is to test it out with a metric in appservices, see https://bugzilla.mozilla.org/show_bug.cgi?id=2034534 Given this is an experiment and not a released crate we can tackle the open questions as we go along, instead of landing the perfectly finished PR. |
travis79
left a comment
There was a problem hiding this comment.
Okay, a decent first pass with several questions. Nothing really big aside from the comments in the metrics.rs about the mismatched Double type being bool
| std::mem::transmute( | ||
| (crate::GLEAN | ||
| .uniffi_glean_core_fn_clone_countermetric)( | ||
| std::mem::transmute(self.handle), | ||
| &mut call_status, | ||
| ), |
There was a problem hiding this comment.
We call transmute twice in each of the clone_handle methods in this file, is that needed? Won't the underlying type be u64 on both sides?
| } | ||
|
|
||
| fn resolve_ffi(&self) -> TokenStream { | ||
| quote! { bool } |
There was a problem hiding this comment.
Should this be bool here? That doesn't seem correct
|
|
||
| impl TypeResolver for weedle::term::Boolean { | ||
| fn resolve(&self) -> TokenStream { | ||
| quote! { i8 } |
There was a problem hiding this comment.
Does this mean consumers will need to cast this to a Rust bool?
| @@ -0,0 +1,119 @@ | |||
| // This Source Code Form is subject to the terms of the Mozilla Public | |||
There was a problem hiding this comment.
I take it this is pretty much duplication of types from glean-core? And, if we make changes there we need to make changes here too?
Be sure and document anything like that, preferably in a comment close to where these live so we don't forget to update things in both places.
|
|
||
| impl CloneFfiArg<RustBuffer> for RustBuffer { | ||
| fn clone_for_ffi(&self) -> RustBuffer { | ||
| // SAFETY: trust me, bro. |
| use xshell::{Shell, cmd}; | ||
|
|
||
| #[test] | ||
| fn generated_metrics_code_up_to_date() { |
There was a problem hiding this comment.
Is this test what keeps things in sync? I see you writing to a committed file and then checking for modifications, and that's what I assume. Could you add some documentation comment(s) on what this does?
| version = "0.3.7" | ||
| criteria = "safe-to-run" | ||
|
|
||
| [[exemptions.prettyplease]] |
There was a problem hiding this comment.
Just out of curiosity, should we document these exemptions anywhere?
| command: | | ||
| cd samples/glean-sym-test | ||
| python3 -m venv ${PWD}/venv | ||
| venv/bin/pip install "git+ssh://git@github.com/mozilla/glean_parser@push-lskqopuplvts#egg=glean-parser" |
There was a problem hiding this comment.
Might be worth filing a bug to update this once it's no longer experimental? I expect this will eventually be pinned to a proper version
| edition = "2024" | ||
|
|
||
| [dependencies] | ||
| bytes = "1.11.1" |
There was a problem hiding this comment.
I don't see this being useed anywhere
This small patch on a-s makes use of the new glean-sys crate: mozilla/application-services@main...badboy:application-services:glean-dylib
Todo:
Naming! Not sure I likeit was decided: glean-sym (as in glean-symbols)glean-sys.glean-sysand we inject alibxul.sointo it at test timeglean-sys/src/metrics.rsis generated. Currently by a test. That has a bit of a problem that you need to compile the code to generate the code, which breaks if the code doesn't compile. Might want to have a non-test way to generate that code.libxul.sois hard-coded. Ok for manual testing in Fenix, not sure we want to ship that as-islibxul.glean-build) to generate the correct code usable withglean-sys