continue prost - #286
Conversation
|
@jokemanfire I appreciate you taking on this work very much. Thanks agian. |
There are some conflicts and API changes that need to be resolved and some code needs to be packaged to adapt to the current API. It's not easy to operate with the previous version of PR. I need to add some commit to complete this feature. I have communicated with the original author. |
dcf4304 to
781c816
Compare
|
@Tim-Zhang @justxuewei Local test all pass but i don't know how to install protoc in CI. |
21ebbd3 to
4a76b7b
Compare
a29312f to
5fadae0
Compare
|
Basically enough @Tim-Zhang @justxuewei just take a look . :) |
|
Nice work, thanks @jokemanfire! Could you cleanup your commits? It makes us easier to review and maintain.
Thanks! |
This commit refactors the ttrpc-codegen and the compiler, and merges the two crates into a single crate, named "codegen". The codegen uses prost crate, a protobuf compiler for Rust. Signed-off-by: Xuewei Niu <niuxuewei.nxw@antgroup.com>
The ttrpc provides a "prost" feature to support the new version of codegen. An "example2" has been added to demonstrate how to use the codegen. Signed-off-by: Xuewei Niu <niuxuewei.nxw@antgroup.com>
Install protoc when executing `make deps` of the ttrpc. Add codegen's check and build, and example2' build to the ci testing. Signed-off-by: Xuewei Niu <niuxuewei.nxw@antgroup.com>
Fix all issues reported by cargo clippy to make ci testing pass. Signed-off-by: Xuewei Niu <niuxuewei.nxw@antgroup.com>
merge from remote branch. ref: containerd#173 Co-authored-by: Xuewei Niu <niuxuewei.nxw@antgroup.com> Signed-off-by: jokemanfire <hu.dingyang@zte.com.cn>
fix ci for protoc Signed-off-by: jokemanfire <hu.dingyang@zte.com.cn>
because of the api change 1. fix the marco in prost 2. fix the test Signed-off-by: jokemanfire <hu.dingyang@zte.com.cn>
fix rebase error. Signed-off-by: jokemanfire <hu.dingyang@zte.com.cn>
I have changed it. |
aa000cc to
4da8148
Compare
b25b1a0 to
00fce61
Compare
skip build example2 in windows Signed-off-by: jokemanfire <hu.dingyang@zte.com.cn>
|
Once this feature gets merged, I prefer to bump a major version to 2 to avoid some compatibility issues, as it nearly changes the entire codegen engine. WDYT? @jokemanfire @Tim-Zhang |
This modification is indeed quite significant, and I agree. It depends on the maintainer's thoughts |
| let path: PathBuf = [out_dir.clone(), "mod.rs".to_string()].iter().collect(); | ||
| fs::write(path, "pub mod ttrpc;").unwrap(); | ||
|
|
||
| generate_ttrpc(&out_dir); |
There was a problem hiding this comment.
To get unified internal APIs which was defined in ttrpc.proto, how about keeping build script untouched?
I think we can try to avoid so many condition compiling sentences like #[cfg(not(feature = "prost"))] in code.
There was a problem hiding this comment.
right ,I think adding a transformation to the camel hump naming convention can reduce this conditional compilation code
while use protoc,rename the Code enum. Signed-off-by: jokemanfire <hu.dingyang@zte.com.cn>
fix marco Signed-off-by: jokemanfire <hu.dingyang@zte.com.cn>
|
I tried to minimize conditional compilation as much as possible @Tim-Zhang @justxuewei |
|
@Tim-Zhang Can you take some time to advance this feature? In the previous puncture, it was found that the prost library uses less memory during operation than the Rust Protobuf library |
There was a problem hiding this comment.
Several generator edge cases, size-limit regressions, and CI issues remain. The inline comments also include refactoring and idiomatic-Rust feedback focused on containing backend-specific behavior, reducing duplication, and avoiding unnecessary allocations and API divergence. Please address the inline findings before merge and add focused fixtures for imported types, multiple services with overlapping method names, and package-less schemas.
| 1. The protoc should be installed. | ||
| 2. Enabling "prost" feature for the ttrpc-rust. | ||
| 3. The Rust files are named based on their package name, rather than the proto | ||
| filename, e.g. `ttrpc = { version = "1.0", features = ["prost"] }`. |
There was a problem hiding this comment.
This example enables prost while keeping default features, which also enables rustprotobuf; the mutual-exclusion check then aborts compilation. Please document default-features = false and include the desired runtime feature explicitly, for example features = ["sync", "prost"].
| () => {}; | ||
| ($first:tt $(,$rest:tt)*) => { | ||
| $( | ||
| #[cfg(all(feature = $first, feature = $rest))] |
There was a problem hiding this comment.
docs.rs is configured with all-features = true, so it activates both protobuf backends and always reaches this compile error. Configure docs.rs with one explicit compatible backend feature set instead of all features.
| fn method_handler_impl_sync_token(&self, struct_name: &Ident, method: &Method) -> TokenStream { | ||
| let mod_path = ttrpc_mod(); | ||
| let context = self.ttrpc_context(false); | ||
| let input_type = format_ident!("{}", method.input_type); |
There was a problem hiding this comment.
method.input_type can be a qualified path such as super::google::protobuf::Empty. Passing it to format_ident! panics because it is not a single identifier. Use the path-aware type_token(&method.input_type) conversion here and in the async handler.
| fn finalize_package(&mut self, _package: &str, _buf: &mut String) {} | ||
| /// Generate services | ||
| fn generate(&mut self, service: Service, buf: &mut String) { | ||
| self.generate_type_aliases(buf); |
There was a problem hiding this comment.
prost-build calls generate once per service and appends services from a package into the same module. Emitting HashMap, Arc, Message, and async_trait imports here for every service causes E0252 on packages with multiple services. Emit package-level imports once, for example from finalize_package.
| } | ||
|
|
||
| fn method_handler_token(&self, service: &Service, method: &Method) -> TokenStream { | ||
| let struct_name = format_ident!("{}Method", to_camel_case(method.proto_name.as_str())); |
There was a problem hiding this comment.
The handler type is derived only from the method name, so Foo.Get and Bar.Get in one package both generate GetMethod. Include the service name in this type, for example FooGetMethod.
| } | ||
| } | ||
|
|
||
| #[derive(Default)] |
There was a problem hiding this comment.
Idiomatic Rust: the boolean and enum settings can store their defaults directly rather than using Option, and build should normally consume self. The P: Default bound is unrelated to paths, while borrowing all inputs makes this builder unnecessarily restrictive. Consider owned PathBuf/Vec<PathBuf> fields and build(self).
| # lock home to avoid conflict with latest version | ||
| home = "=0.5.9" | ||
| protobuf-codegen = "3.1.0" | ||
| prost-build = { version = "0.13", optional = true } |
There was a problem hiding this comment.
Refactor/dependencies: the runtime uses prost 0.11 here while the root build uses prost-build 0.13, and the new codegen crate pins the Prost family to 0.11. Keep the Prost crates on one compatible version and centralize the versions to avoid duplicate dependency trees and generated/runtime drift.
| let mut camel_case_name = String::with_capacity(name.len()); | ||
| for s in NameSpliter::new(name) { | ||
| let mut chs = s.chars(); | ||
| camel_case_name.extend(chs.next().unwrap().to_uppercase()); |
There was a problem hiding this comment.
Refactor/correctness: this custom name splitter can yield an empty segment after leading underscores and then panic at this unwrap; it also reimplements casing already handled by established Rust casing utilities and by normalized prost-build names. Prefer those normalized names or a well-tested casing helper.
| @@ -0,0 +1,55 @@ | |||
| [package] | |||
There was a problem hiding this comment.
Scope/refactor: example2 duplicates the existing examples and brings in large third-party proto trees solely to switch protobuf backends, so future fixes and tests can diverge. Prefer running the existing example under a backend feature matrix and add small focused fixtures only for Prost-specific codegen behavior.
| example2/protocols/**/*.rs | ||
| !example2/protocols/**/mod.rs | ||
| src/ttrpc.rs | ||
| example2/protocols/**/*.rs |
There was a problem hiding this comment.
These example2 ignore/negation rules duplicate lines 10-11. Please remove the redundant entries and keep the file's final newline.
Superseded by the inline review with additional refactoring feedback.
|
@jokemanfire I have left a few review comments, and btw please resolve the conflicts, thanks. |
thanks , I will take some time to resolve it. |
work continue with #173