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

Move client proto generation to oak_utils #1205

Merged
merged 3 commits into from
Jul 1, 2020
Merged

Move client proto generation to oak_utils #1205

merged 3 commits into from
Jul 1, 2020

Conversation

ipetr0v
Copy link
Contributor

@ipetr0v ipetr0v commented Jun 26, 2020 •
edited
Loading

This change moves Protobuf generation function to oak_utils.

Fixes #1120
Ref #1093

Checklist

  • Pull request affects core Oak functionality (e.g. runtime, SDK, ABI)

@daviddrysdale
Copy link
Contributor

This looks like it might interact with #1108; maybe worth syncing up with @wildarch?

@wildarch
Copy link
Contributor

For #1108 we'll need to get rid of all direct calls to tonic_build's compile and do this via oak_utils so we can correctly set the PROTOC environment variable. Since I've done a similar thing in #1108 those changes would conflict, but maybe the easiest way to resolve that is for me to rebase my PR on this one (or ideally this will have been merged to main by then).

@ipetr0v ipetr0v marked this pull request as ready for review June 29, 2020 12:20
@ipetr0v
Move client proto generation to oak_utils
c5e3176 
Update examples

Update functions
tiziano88
tiziano88 approved these changes Jun 29, 2020
View reviewed changes
examples/trusted_information_retrieval/backend/build.rs Outdated Show resolved Hide resolved
oak_utils/src/lib.rs Outdated
Comment on lines 213 to 214
build_client: bool,
build_server: bool,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps define two enums BuildClient { Yes, No } and BuildServer { Yes, No } (or similar) instead of vague bools? In fact then you can just expose this function, you don't need to have the helper function to make it actually readable.

Copy link
Contributor

Choose a reason for hiding this comment

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

See also

Copy link
Contributor

Choose a reason for hiding this comment

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

I think David's comment applies here. Maybe we can use this PR to introduce a configuration object for compiling protos (optionally with tonic)? Also fine to defer that to a later PR.

I would recommend to go with a builder pattern if you do decide to go with a config struct, that way you can easily add new fields later without changing example code, which I think happens quite frequently now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added the following config struct:

/// Options for building gRPC code.
pub struct CodegenOptions {
    /// Specify whether to build client related code.
    build_client: bool,
    /// Specify whether to build server related code.
    build_server: bool,
}

wildarch
wildarch reviewed Jun 29, 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.

Thanks Ivan!

examples/trusted_information_retrieval/backend/build.rs Outdated Show resolved Hide resolved
oak_utils/src/lib.rs Outdated
Comment on lines 213 to 214
build_client: bool,
build_server: bool,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think David's comment applies here. Maybe we can use this PR to introduce a configuration object for compiling protos (optionally with tonic)? Also fine to defer that to a later PR.

I would recommend to go with a builder pattern if you do decide to go with a config struct, that way you can easily add new fields later without changing example code, which I think happens quite frequently now.

oak_utils/src/lib.rs Outdated Show resolved Hide resolved
@ipetr0v ipetr0v requested a review from wildarch June 30, 2020 12:09
wildarch
wildarch reviewed Jun 30, 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.

Thanks Ivan!

oak_utils/src/lib.rs
}

/// Generate gRPC code from Protobuf using `tonic` library.
pub fn generate_grpc_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: prost_build and tonic_build both put this method on the config struct itself, which might make this a more fluent API and potentially be more familiar to users. This is probably personal preference though, what you have is also fine 😄

oak_utils/src/lib.rs Outdated Show resolved Hide resolved
oak_utils/src/lib.rs Show resolved Hide resolved
oak_utils/src/lib.rs Outdated Show resolved Hide resolved
@ipetr0v ipetr0v requested a review from wildarch June 30, 2020 17:59
wildarch
wildarch approved these changes Jul 1, 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.

This looks great, thanks! 😄

@ipetr0v ipetr0v merged commit 2cf69a7 into project-oak:main Jul 1, 2020
@ipetr0v ipetr0v deleted the utils_progo_generation branch July 1, 2020 09:47
@github-actions
Copy link

github-actions bot commented Jul 1, 2020

Reproducibility Index:

4c95bc66c9e0494daee4fab1df40fc342cd8ef454295baf186286d0eeac45f23  ./examples/target/wasm32-unknown-unknown/release/abitest_0_frontend.wasm
87992c11e0b01a4ab6f9a490dd558fe472ee96d7346af2ea629230c603cb4b4c  ./examples/target/wasm32-unknown-unknown/release/abitest_1_backend.wasm
a74f5ba4c36276be597f5923dfbe290a1d933d2b2faf4d1afab54e25d7b17f8c  ./examples/target/wasm32-unknown-unknown/release/aggregator.wasm
012d9ee0db65f4f366cae28864de0368db9adbcca7639c20d5ec4872f41646c8  ./examples/target/wasm32-unknown-unknown/release/chat.wasm
37931c13fe10440196d4fe0caaf04bcbb7f99aa7445a09b7e10ccc6432f8930b  ./examples/target/wasm32-unknown-unknown/release/database_proxy.wasm
03b910296f9b88357cebd56de4f8d6b6c05cd998a1148719cf69e6b6fbc4c472  ./examples/target/wasm32-unknown-unknown/release/hello_world.wasm
ecda9155dd8e0614f06c8384d34560e8c80a8cb29abcd5e7a78118236a6545e8  ./examples/target/wasm32-unknown-unknown/release/machine_learning.wasm
3983d834d805cc5d56366c0ff06215d930e6c98687df6e6f9563ddbd7089d3ac  ./examples/target/wasm32-unknown-unknown/release/private_set_intersection.wasm
de8e819e5173411d55ce13318d9caa25cd165bf47337db227cbf80bb53a9989d  ./examples/target/wasm32-unknown-unknown/release/running_average.wasm
e30a9e7e8fd6510ea1b89703c8dc5dc7541dc2689d84015ba1a2f561d75b384e  ./examples/target/wasm32-unknown-unknown/release/translator.wasm
8b6a37cf40e4bd4b16188b9a382581cec5774550adc7e8b36f47682d274c9182  ./examples/target/wasm32-unknown-unknown/release/trusted_information_retrieval.wasm
21dbf6657c74a9d527955a9b33bdea65c5017ea080c105c34dd9f9cfb3f3c873  ./oak/server/target/x86_64-unknown-linux-musl/release/oak_loader

Reproducibility Index diff:

diff --git a/reproducibility_index b/reproducibility_index
index 2461e1a..be1e4a2 100644
--- a/reproducibility_index
+++ b/reproducibility_index
@@ -1,12 +1,12 @@
-562715d812c1ed5397402d3f4b02b03addf416ccb4a8f071eac00a8015082b42  ./examples/target/wasm32-unknown-unknown/release/abitest_0_frontend.wasm
-e89bb8621965e356156c939997a61fa1af30352159ae9e7a59f50a8eb101648d  ./examples/target/wasm32-unknown-unknown/release/abitest_1_backend.wasm
-b256b1c1402a8f25381352047ee48cc9a07314d4227a64bd786369a17c61d1df  ./examples/target/wasm32-unknown-unknown/release/aggregator.wasm
-44eff10ce70a232c84cc32c225e75eca695634abc9ecfbf543022aa0a5682cd7  ./examples/target/wasm32-unknown-unknown/release/chat.wasm
-50c1455386c91fc6fc1d5adb31d2194d6ab183eac3ad54eb3cb388eb4aafad72  ./examples/target/wasm32-unknown-unknown/release/database_proxy.wasm
-b28dc802af588ea6512974d2d9ec17557dd454f760b379ab80b11efbe444ecfd  ./examples/target/wasm32-unknown-unknown/release/hello_world.wasm
-1123ddd109dd00155b0f59ccc2f3025974bacc82e15a4d1fe2a83ff6ceb2b3cd  ./examples/target/wasm32-unknown-unknown/release/machine_learning.wasm
-66ae9e10c2d72cab7e629b2fea2ae98939649c391480ab255d589f5da8c7ae0f  ./examples/target/wasm32-unknown-unknown/release/private_set_intersection.wasm
-0cd8cb0baae0c59852e775a697ca5408f975c1c1fdc53cc8f3a0e7d07e23f5af  ./examples/target/wasm32-unknown-unknown/release/running_average.wasm
-15da62b4701e02ae731276dbe8bdfd29805871e7f863e8e15b1fe11983023159  ./examples/target/wasm32-unknown-unknown/release/translator.wasm
-c2b22d73cbeee8a7945043cc1ffaa415b92cb25d6de91e4e46523435390bad9f  ./examples/target/wasm32-unknown-unknown/release/trusted_information_retrieval.wasm
-91ca4391145ce09746c870f0c0b4d7fd9d380fa08c79aa0e929cc64d6f8aa292  ./oak/server/target/x86_64-unknown-linux-musl/release/oak_loader
+4c95bc66c9e0494daee4fab1df40fc342cd8ef454295baf186286d0eeac45f23  ./examples/target/wasm32-unknown-unknown/release/abitest_0_frontend.wasm
+87992c11e0b01a4ab6f9a490dd558fe472ee96d7346af2ea629230c603cb4b4c  ./examples/target/wasm32-unknown-unknown/release/abitest_1_backend.wasm
+a74f5ba4c36276be597f5923dfbe290a1d933d2b2faf4d1afab54e25d7b17f8c  ./examples/target/wasm32-unknown-unknown/release/aggregator.wasm
+012d9ee0db65f4f366cae28864de0368db9adbcca7639c20d5ec4872f41646c8  ./examples/target/wasm32-unknown-unknown/release/chat.wasm
+37931c13fe10440196d4fe0caaf04bcbb7f99aa7445a09b7e10ccc6432f8930b  ./examples/target/wasm32-unknown-unknown/release/database_proxy.wasm
+03b910296f9b88357cebd56de4f8d6b6c05cd998a1148719cf69e6b6fbc4c472  ./examples/target/wasm32-unknown-unknown/release/hello_world.wasm
+ecda9155dd8e0614f06c8384d34560e8c80a8cb29abcd5e7a78118236a6545e8  ./examples/target/wasm32-unknown-unknown/release/machine_learning.wasm
+3983d834d805cc5d56366c0ff06215d930e6c98687df6e6f9563ddbd7089d3ac  ./examples/target/wasm32-unknown-unknown/release/private_set_intersection.wasm
+de8e819e5173411d55ce13318d9caa25cd165bf47337db227cbf80bb53a9989d  ./examples/target/wasm32-unknown-unknown/release/running_average.wasm
+e30a9e7e8fd6510ea1b89703c8dc5dc7541dc2689d84015ba1a2f561d75b384e  ./examples/target/wasm32-unknown-unknown/release/translator.wasm
+8b6a37cf40e4bd4b16188b9a382581cec5774550adc7e8b36f47682d274c9182  ./examples/target/wasm32-unknown-unknown/release/trusted_information_retrieval.wasm
+21dbf6657c74a9d527955a9b33bdea65c5017ea080c105c34dd9f9cfb3f3c873  ./oak/server/target/x86_64-unknown-linux-musl/release/oak_loader

tiziano88 pushed a commit to tiziano88/oak that referenced this pull request Jul 2, 2020
@ipetr0v @tiziano88
Move client proto generation to oak_utils (project-oak#1205)
ece83d3 
This change moves Protobuf generation function to `oak_utils`.

Fixes project-oak#1120
Ref project-oak#1093
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tiziano88 tiziano88 tiziano88 approved these changes

@wildarch wildarch wildarch approved these changes

@daviddrysdale daviddrysdale Awaiting requested review from daviddrysdale

Assignees
No one assigned
Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add Rust client library function for building Protobuf
5 participants
@ipetr0v @daviddrysdale @wildarch @tiziano88 @googlebot