containerd / ttrpc-rust

Rust implementation of ttrpc (GRPC for low-memory environments)
Apache License 2.0
197 stars 47 forks source link

Code generator doesn't take into account package names #2

Closed mxpv closed 4 years ago

mxpv commented 4 years ago

It looks like that the rust version of ttrpc code generator doesn't take into account proto package names when generating the code. This leads to all endpoints are being prefixed (example) with /grpc.ServiceName instead of /PackageName.ServiceName. This makes it incompatible with the existing Go based generator, which adds package names (for instance events: events.proto, events.pb.go)

How to reproduce: I took this file: https://github.com/containerd/containerd/blob/master/runtime/v2/task/shim.proto and ran with:

protoc \
    --rust_out=. \
    --ttrpc_out=. \
    --plugin=protoc-gen-ttrpc=$(shell which ttrpc_rust_plugin) \
    -I/work \
    -I$(shell go env GOPATH)/src \
    -I$(shell go env GOPATH)/src/github.com/gogo/protobuf \
    dir

The output of shim_ttrpc.rs looks like this:

pub fn create_task(service: Arc>) -> HashMap > { let mut methods = HashMap::new(); methods.insert("/grpc.Task/State".to_string(), std::boxed::Box::new(state_method{service: service.clone()}) as std::boxed::Box<::ttrpc::MethodHandler + Send + Sync>); methods.insert("/grpc.Task/Create".to_string(), std::boxed::Box::new(create_method{service: service.clone()}) as std::boxed::Box<::ttrpc::MethodHandler + Send + Sync>); methods.insert("/grpc.Task/Start".to_string(), std::boxed::Box::new(start_method{service: service.clone()}) as std::boxed::Box<::ttrpc::MethodHandler + Send + Sync>); methods.insert("/grpc.Task/Delete".to_string(), std::boxed::Box::new(delete_method{service: service.clone()}) as std::boxed::Box<::ttrpc::MethodHandler + Send + Sync>); methods.insert("/grpc.Task/Pids".to_string(), std::boxed::Box::new(pids_method{service: service.clone()}) as std::boxed::Box<::ttrpc::MethodHandler + Send + Sync>); methods.insert("/grpc.Task/Pause".to_string(), std::boxed::Box::new(pause_method{service: service.clone()}) as std::boxed::Box<::ttrpc::MethodHandler + Send + Sync>); methods.insert("/grpc.Task/Resume".to_string(), std::boxed::Box::new(resume_method{service: service.clone()}) as std::boxed::Box<::ttrpc::MethodHandler + Send + Sync>); methods.insert("/grpc.Task/Checkpoint".to_string(), std::boxed::Box::new(checkpoint_method{service: service.clone()}) as std::boxed::Box<::ttrpc::MethodHandler + Send + Sync>); methods.insert("/grpc.Task/Kill".to_string(), std::boxed::Box::new(kill_method{service: service.clone()}) as std::boxed::Box<::ttrpc::MethodHandler + Send + Sync>); methods.insert("/grpc.Task/Exec".to_string(), std::boxed::Box::new(exec_method{service: service.clone()}) as std::boxed::Box<::ttrpc::MethodHandler + Send + Sync>); methods.insert("/grpc.Task/ResizePty".to_string(), std::boxed::Box::new(resize_pty_method{service: service.clone()}) as std::boxed::Box<::ttrpc::MethodHandler + Send + Sync>); methods.insert("/grpc.Task/CloseIO".to_string(), std::boxed::Box::new(close_io_method{service: service.clone()}) as std::boxed::Box<::ttrpc::MethodHandler + Send + Sync>); methods.insert("/grpc.Task/Update".to_string(), std::boxed::Box::new(update_method{service: service.clone()}) as std::boxed::Box<::ttrpc::MethodHandler + Send + Sync>); methods.insert("/grpc.Task/Wait".to_string(), std::boxed::Box::new(wait_method{service: service.clone()}) as std::boxed::Box<::ttrpc::MethodHandler + Send + Sync>); methods.insert("/grpc.Task/Stats".to_string(), std::boxed::Box::new(stats_method{service: service.clone()}) as std::boxed::Box<::ttrpc::MethodHandler + Send + Sync>); methods.insert("/grpc.Task/Connect".to_string(), std::boxed::Box::new(connect_method{service: service.clone()}) as std::boxed::Box<::ttrpc::MethodHandler + Send + Sync>); methods.insert("/grpc.Task/Shutdown".to_string(), std::boxed::Box::new(shutdown_method{service: service.clone()}) as std::boxed::Box<::ttrpc::MethodHandler + Send + Sync>); methods }
teawater commented 4 years ago

@mxpv I test this part with ttrpc-go generation code. But I am not sure whether the generation command is right. Did you test this part with containerd?

mxpv commented 4 years ago

@teawater yes, I'm testing rust client/server against containerd. After I replaced endpoints manually, everything worked fine.

teawater commented 4 years ago

@mxpv Thanks for your report! I was confused by the package name and protocol. Do you want to post PR for this issue?

teawater commented 4 years ago

@mxpv Could you help me test the PR?

mxpv commented 4 years ago

Do you want to post PR for this issue?

@teawater sorry, was going to have a look next week.

Could you help me test the PR?

Yep, the fix from fix_package_name branch works like a charm :)