Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Use prost instead of rust-protobuf #829

Merged
merged 1 commit into from
Apr 9, 2020
Merged

Use prost instead of rust-protobuf #829

merged 1 commit into from
Apr 9, 2020

Conversation

tiziano88
Copy link
Collaborator

@tiziano88 tiziano88 commented Apr 8, 2020 •
edited

Main differences:

  • use ::default instead of ::new to create instances
  • use native field accessors
  • use native Option and Vec types for optional and repeated fields
  • dispatcher struct has a more qualified name
  • gRPC generator code is encapsulated in a prost ServiceGenerator
  • use quote! macros instead of string concatenation in the generator

Fixes #668

Checklist

  • Pull request affects core Oak functionality (e.g. runtime, SDK, ABI)
    • I have written tests that cover the code changes.
    • I have checked that these tests are run by Cloudbuild
    • I have updated documentation accordingly.

@tiziano88 tiziano88 force-pushed the tzn_prost_everywhere branch 11 times, most recently from 193ae98 to c16fd28 Compare April 8, 2020 22:27
@tiziano88 tiziano88 added this to In progress in planning via automation Apr 8, 2020
@tiziano88 tiziano88 marked this pull request as ready for review April 8, 2020 22:34
@tiziano88
Copy link
Collaborator Author

heads-up to @project-oak/core and @wildarch . If anyone has any opinions about this please feel free to leave a comment.

@tiziano88 tiziano88 force-pushed the tzn_prost_everywhere branch 3 times, most recently from 633b296 to 9b75a26 Compare April 8, 2020 22:58
daviddrysdale
daviddrysdale approved these changes Apr 9, 2020
View reviewed changes
examples/abitest/module_0/rust/src/lib.rs
"Deliberate error",
));
match req.method_result {
Some(proto::grpc_test_request::MethodResult::ErrCode(err_code)) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: use proto::grpc_test_request::MethodResult

examples/abitest/module_0/rust/src/lib.rs Show resolved Hide resolved
examples/aggregator/backend/Cargo.toml Show resolved Hide resolved
sdk/rust/oak/src/storage/mod.rs
crate::grpc::invoke_grpc_method(
grpc_method_name,
&operation_request,
Some(&type_url),
None,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC the type_url needed to be present in order for the C++ code to be able to reconstruct the inner protobuf request. Do the storage tests still work with this removed?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Edit: looking at the Cloudbuild logs, looks like this is still a problem:

I0408 23:14:00.449162 23205 abitest.cc:76] [ FAIL ] Storage
I0408 23:14:00.449169 23205 abitest.cc:79]     Details: Error: status code 3 message 'Failed to unpack request'

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Possible workaround in #831

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(also, we should remember to come back and ding the req_type_url parameter to invoke_grpc_method_[stream] now it's never used)

oak/server/rust/oak_abi/build.rs Outdated Show resolved Hide resolved
oak/server/rust/oak_runtime/build.rs Outdated Show resolved Hide resolved
sdk/rust/oak/src/error.rs Outdated Show resolved Hide resolved
sdk/rust/oak_utils/src/lib.rs Show resolved Hide resolved
sdk/rust/oak_utils/src/lib.rs Show resolved Hide resolved
@daviddrysdale daviddrysdale mentioned this pull request Apr 9, 2020
Merged
6 tasks
wildarch
wildarch reviewed Apr 9, 2020
View reviewed changes
Copy link
Contributor

@wildarch wildarch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome stuff @tiziano88!

oak/server/rust/oak_runtime/src/node/grpc_server.rs Outdated
@@ -185,11 +186,10 @@ impl GrpcServerNode {
http_request_body: &dyn hyper::body::Buf,
) -> Result<GrpcRequest, GrpcServerError> {
// Parse an HTTP request body as a [`protobuf::well_known_types::Any`] message.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be prost_types::Any now?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

sdk/rust/oak/src/lib.rs
pub mod proto {
pub mod google {
pub mod protobuf {
include!(concat!(env!("OUT_DIR"), "/google.protobuf.rs"));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we actually use these types somewhere?

@tiziano88 tiziano88 force-pushed the tzn_prost_everywhere branch 2 times, most recently from cfb6e46 to 24ee189 Compare April 9, 2020 09:50
@tiziano88 tiziano88 mentioned this pull request Apr 9, 2020
Closed
@tiziano88
Use prost instead of rust-protobuf
ed7c8da 
Main differences:

- use `::default` instead of `::new` to create instances
- use native field accessors
- use native Option and Vec types for optional and repeated fields
- dispatcher struct has a more qualified name
- gRPC generator code is encapsulated in a prost ServiceGenerator
- use `quote!` macros instead of string concatenation in the generator

Fixes project-oak#668
@tiziano88 tiziano88 merged commit a81d19c into project-oak:master Apr 9, 2020
planning automation moved this from In progress to Done Apr 9, 2020
@tiziano88 tiziano88 deleted the tzn_prost_everywhere branch April 9, 2020 11:03
@daviddrysdale
Copy link
Contributor

W00t!

@tiziano88 tiziano88 mentioned this pull request Apr 15, 2020
Closed
@daviddrysdale daviddrysdale mentioned this pull request Apr 21, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@daviddrysdale daviddrysdale daviddrysdale approved these changes

@wildarch wildarch wildarch left review comments

Assignees
No one assigned
Labels
cla: yes
Projects
planning
  
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Switch from rust_protobuf to prost for protobuf generation
4 participants
@tiziano88 @daviddrysdale @wildarch @googlebot