Upgraded protobuf to support cross crate imports. #24
Conversation
node/libs/protobuf_build/src/lib.rs
Outdated
| static INIT : {this}::Lazy<{this}::prost_reflect::MessageDescriptor> = {this}::Lazy::new(|| {{\ | ||
| DESCRIPTOR.load_global().get_message_by_name({proto_name:?}).unwrap()\ | ||
| }});\ | ||
| INIT.clone()\ |
There was a problem hiding this comment.
- As with the descriptor definition, ideally we'd want to move as much code from here to the crate itself as possible. Again, this'll probably require using a macro like
derive_reflect_message!($message:ty, $descriptor:ident). - Stupid question: since
DESCRIPTORis a static already, does it make sense to cache separateMessageDescriptors here instead of just returning a fresh clone each time? Is it significantly more efficient?
There was a problem hiding this comment.
load_global() is inefficient because it checks whether all the files of the descriptor have been already loaded. Plus, cloning prost_reflect::DescriptorPool in load_global() is inefficient because it is poorly written.
Co-authored-by: Bruno França <[email protected]>
There was a problem hiding this comment.
Couple of tricks after I've refreshed my memory on Protobuf handling in Exonum (it was used there as the main serialization format as well):
- It's possible to use the
quotecrate to generate Rust code more eloquently. See an example here. - It's possible to pass information from deps to the consumers using fake linking. E.g., here the Merkle tree crate publishes a path to its Protobuf files, which will then be available as the
DEP_EXONUM_PROTOBUF_MERKLEDB_PROTOSenv variable. The var name isDEP_$link_$var, wherelinkis a fake crate that the exporting library links to (it doesn't need to exist), and$varis the name inprintln!("cargo:$var={}", ...);. Multiple vars can be passed using this way; e.g., we could use this to pass paths to Protobuf descriptors to solve the "dependent needs to wait for all direct dependencies to fully compile" issue. (With this approach, it would be improved to "dependent needs to wait for the dependencies build scripts to execute".)
Not sure whether the second trick would be helpful, but using quote is IMO certainly preferable to generating Rust code as strings.
| //! Generates rust code from protobufs. | ||
| fn main() { | ||
| zksync_protobuf::build::Config { | ||
| input_root: "proto".into(), |
There was a problem hiding this comment.
Nit: Is src/proto or proto path more canonical as a path for "main" Protobuf declarations? Here, you use proto, but the zksync_protobuf crate uses src/proto. Personally, I slightly prefer proto since it cleanly separate source files from the Rust module wrapper.
There was a problem hiding this comment.
In my mind it, "proto" should be a subdirectory of a module that it is supposed to be embedded into. I haven't touched schema/proto, because I plan to split it in the next pr.
There was a problem hiding this comment.
Not sure I agree with this convention ("hiding" Protobuf files in the Rust source dir look suboptimal), but this can be elaborated later.
|
I've replaced string generation with quote. |
| .to_string(); | ||
| let rust_name: syn::Path = syn::parse_str(&rust_name).context("rust_name")?; | ||
| let proto_name = proto_name.to_string(); | ||
| let this: syn::Path = syn::parse_str(&self.this_crate().to_string()).context("this")?; |
There was a problem hiding this comment.
Nit: A more conventional way to do this is to use parse_quote!, or make RustName convertible to syn::Path (IIUC, RustName is semantically equivalent to it). With the current RustName internal structure, this is hardly possible.
| pub(crate) fn format(&self) -> anyhow::Result<String> { | ||
| let s = self.collect(); | ||
| Ok(prettyplease::unparse( | ||
| &syn::parse_str(&s).with_context(|| format!("syn::parse_str({s:?})"))?, |
There was a problem hiding this comment.
You can have RustModule wrap a syn::File internally (in general, dealing with syn types rather than strings) in order to get rid of AST -> string -> AST conversions. I'm happy to try doing this in a follow-up PR, so that it the basic functionality is delivered faster.
| //! Generates rust code from protobufs. | ||
| fn main() { | ||
| zksync_protobuf::build::Config { | ||
| input_root: "proto".into(), |
There was a problem hiding this comment.
Not sure I agree with this convention ("hiding" Protobuf files in the Rust source dir look suboptimal), but this can be elaborated later.
Upgraded protobuf to support cross crate imports.
We need it to allow defining protos in zksync-era as well.
Proto packages share a single common namespace, so I prefixed our packages with zksync..
I couldn't name the crate "protobuf", because cargo deny has mistaken it for the public crate with the same name. I've named it "zksync_protobuf", to be consistent with zksync-era repo.
CI protobuf compatibility check is now more complex, because we need to collect the descriptors from the cargo build artifacts. Currently it is implemented as a bash command, but can be upgraded to a proper github action in the future.