Use the vendored prost everywhere. #1225

wildarch · 2020-07-03T07:45:22Z

This also introduces a ProtoOptions struct to oak_utils much like CodegenOptions,
so adding new options in the future is easier.

This should make it easier to subsequently land #1108.

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.
- I have raised an issue to
  cover any TODOs and/or unfinished work.
Pull request includes prototype/experimental work that is under
construction.

wildarch · 2020-07-03T08:13:53Z

@daviddrysdale, could you take a look? 😄

wildarch · 2020-07-03T11:19:28Z

CC @tiziano88

daviddrysdale

LGTM if Cloudbuild is happy.

daviddrysdale · 2020-07-03T11:41:35Z

oak_utils/src/lib.rs

@@ -163,6 +163,20 @@ impl prost_build::ServiceGenerator for OakServiceGenerator {
    }
 }

+/// Options for generating Protocol buffer Rust types.
+pub struct ProtoOptions {
+    /// Generates service code for inter-node communication. Default: **true**.


nit: could we make this even more explicit, to allow for other options/variants that will come later? e.g. "Generates Oak-specific service code for inter-node communication; generated code will depend on the oak SDK crate."

daviddrysdale · 2020-07-03T11:42:36Z

oak_utils/src/lib.rs

+fn set_protoc_env_if_unset() {
+    if std::env::var("PROTOC").is_err() {
+        // Use the system protoc if no override is set, so prost-build does not try to use the
+        // bundled one that we removing as part of the vendoring process.


nit: "remove" or "removed"

tiziano88 · 2020-07-03T11:29:12Z

oak_abi/build.rs

+// Keep clippy from complaining about a needless call to `Default::default()`.
+#[allow(clippy::needless_update)]


I actually think it's better to remove the default call; if we add extra fields to ProtoOptions, it would be good if the compiler tells us we need to update this file too. Anyways, up to you.

I would actually argue the exact opposite 😅 . When you add new options here, this is most likely due to a new feature that you need to support for a particular use case, it should not require changes to existing code (if possible). In other words, putting the call to Default::default() here allows for some degree of backwards compatibility.

Yes I understand, and certainly if this was a third party crate I would agree. I just think that in our case it's nice for the compiler to tell us all the places that require update, just to double check we are not missing anything. your choice anyways :)

Aah, I was under the impression that third parties that want to integrate with Oak would also use oak_utils to generate code? Or is it just that we're not designing for that explicitly yet?

tiziano88 · 2020-07-03T11:50:14Z

oak_utils/src/lib.rs

+impl Default for ProtoOptions {
+    fn default() -> ProtoOptions {
+        ProtoOptions {
+            generate_services: true,


I find this a bit confusing, also that it needs to be kept in sync with the comment above (what does it mean for a bool field to have a default of true? really it's just this method that sets it to true, so perhaps the comment should be here). I think having an Option would be more explicit, or just require callers to set it to true (I don't think we need to worry about that kind of repetition).

I agree it would be nice to put the explanation of the default value next to where that is actually defined, but as a user you would probably only read the documentation on each of the fields, having to drill down into the Default impl is not very intuitive here.

The Option strategy is interesting, but that would mean we do things differently between ProtoOptions and CodegenOptions, which we would probably want to avoid if possible. What do you think?

I still think the default should be false, and we set it to true where needed :) But I'll leave it to you to decide.

rbehjati

LGTM.

rbehjati · 2020-07-03T14:02:06Z

oak_utils/src/lib.rs

-/// definitions, emitting generated code to crate's `OUT_DIR`.
-pub fn compile_protos_without_services<P>(inputs: &[P], includes: &[P])
+/// Like `compile_protos`, but allows for configuring options through
+/// [`ProtoOptions`](struct.ProtoOptions.html).


Is the URL (struct.ProtoOptions.html) really needed?

Not really I suppose, I'll simplify it to justProtoOptions

rbehjati · 2020-07-03T14:12:38Z

examples/Cargo.toml

@@ -51,3 +51,7 @@ trusted_information_retrieval_client = { path = "trusted_information_retrieval/c
 # Third party.
 expect = { path = "../third_party/expect" }
 roughenough = { path = "../third_party/roughenough" }
+prost = { path = "../third_party/prost" }


Do these need to be added to experimental/Cargo.toml too?

I'm not sure actually. If we add the overrides you couldn't use a different version of prost for experimental stuff, which at some point we might want? Also, nothing in experimental seems to need the vendored version right now.

What do you think?

I don't know much about prost or the vendored version of it, nor do I know much about the experimental crate! I guess @conradgrobler can be a better judge of it. I just mentioned it for completeness :-) But this is certainly something that can be fixed later if needed.

I don't think we will need it in the experimental crate at the moment. The only code in there is an unfinished attempt at proxying gRPC with TLS over shared memory.

This also introduces a `ProtoOptions` struct much like `CodegenOptions`, so adding new options in the future is easier. Note: this also needs to whitelist the ZLib license, as the dependency graph now includes [`tinyvec`](https://github.com/Lokathor/tinyvec).

wildarch · 2020-07-03T15:26:42Z

I think we're good to merge! 😃

github-actions · 2020-07-06T16:50:00Z

Reproducibility Index:

27b563913abba69ab96699ca4576a5406231800739297d43f14647dfe28182d3  ./examples/target/wasm32-unknown-unknown/release/abitest_0_frontend.wasm
aaa701e9c0fc58fd65e5956bf07dbc17a9fe30170d776f8c734e833ecc30718e  ./examples/target/wasm32-unknown-unknown/release/abitest_1_backend.wasm
4b88fc85c633e5413002b05ff66bec9ec653397f07f6799f889aeb7af14fb6e1  ./examples/target/wasm32-unknown-unknown/release/aggregator.wasm
1e8c5a8ff13e83e228fa461eb049e7eb37600b980ef17e1e42aed2b9b14c8cc1  ./examples/target/wasm32-unknown-unknown/release/chat.wasm
400c13da142185bb700bf6daaed1de58f38a6ebc98d52a67a0c93f425a806c77  ./examples/target/wasm32-unknown-unknown/release/database_proxy.wasm
9035e13788e682a01e22c1ba42c125fccbfae4df296d7bd46318fb56044e19a2  ./examples/target/wasm32-unknown-unknown/release/hello_world.wasm
c5646c404e5d4f4d354f1961e39bc9ca23989922741efee90b9f270625bdcd29  ./examples/target/wasm32-unknown-unknown/release/machine_learning.wasm
3267614a8c860eef6449c40856cef55b04aceb95a206fe9d3cd70d2b3931d967  ./examples/target/wasm32-unknown-unknown/release/private_set_intersection.wasm
656e89a34c1d964fd003e7421d204f96fd5c3853d27b142a9eec619f6f86f96b  ./examples/target/wasm32-unknown-unknown/release/running_average.wasm
aea5aa8876b8158f0d781665e089307e9de53b59104407e71422a58ed4cff6d2  ./examples/target/wasm32-unknown-unknown/release/translator.wasm
080c53ca4491dbd9e279725cc32abddca83d0da14e645915981455b5b9f8ecd2  ./examples/target/wasm32-unknown-unknown/release/trusted_information_retrieval.wasm
f7e04fec862016c4c4dfdb816062cd5227cf8325b3340e5cdbf7d29ff7d5a714  ./oak/server/target/x86_64-unknown-linux-musl/release/oak_loader

Reproducibility Index diff:

diff --git a/reproducibility_index b/reproducibility_index
index 9789e7e..f55aea2 100644
--- a/reproducibility_index
+++ b/reproducibility_index
@@ -1,12 +1,12 @@
-2dacd4bdf59c41335d3539547aaccae774b4ac47b9ad874e65c4ff913ff24b0e  ./examples/target/wasm32-unknown-unknown/release/abitest_0_frontend.wasm
-94d0174fceaa83485122b5be7f4a9c7a9d50ebdbaedd23cd0d0e66eb4287ec43  ./examples/target/wasm32-unknown-unknown/release/abitest_1_backend.wasm
-2ee66e6f80a04c71d63c76f91b0bf92b4bd1d277a0be335ade2b60e789af5d89  ./examples/target/wasm32-unknown-unknown/release/aggregator.wasm
-05d3ae848e0ccbbf0fd09f409f202e80489327f1d5d15760e380094d5d60f1ff  ./examples/target/wasm32-unknown-unknown/release/chat.wasm
-8fccfe9b81329c23007bc1a6812b82c2be62975ba831019f7e002555f31bcf0a  ./examples/target/wasm32-unknown-unknown/release/database_proxy.wasm
-e658111a1de72d79ea6b1832e3b4a0af27f6fe3b9c2f121d040ae6109db15e7f  ./examples/target/wasm32-unknown-unknown/release/hello_world.wasm
-5c4c161f2c5977c2d2e8654ef2b6c13a9977d38dfa0a8dafa8a69b8a991419f9  ./examples/target/wasm32-unknown-unknown/release/machine_learning.wasm
-b43c4609da35262424e7f843840820dc5c9aadd0b600daffb7a1aea73c3641d2  ./examples/target/wasm32-unknown-unknown/release/private_set_intersection.wasm
-bcecf6e27239dfce41fbc42026a56f5da271b0fe9d2cdc430ec296f1cb5b8ec7  ./examples/target/wasm32-unknown-unknown/release/running_average.wasm
-ea7628b0b3158c94b36601609c65fa7c68eeea817760f5097c53bab652821e2d  ./examples/target/wasm32-unknown-unknown/release/translator.wasm
-d49a200cbd8932cd1adaa050832e73def9436a09ca60c370cf4b56f850f786e8  ./examples/target/wasm32-unknown-unknown/release/trusted_information_retrieval.wasm
-4c640f02b645dfd84b76624955e494dfbf6b66e26bd9f6e642bb56889d201d7b  ./oak/server/target/x86_64-unknown-linux-musl/release/oak_loader
+27b563913abba69ab96699ca4576a5406231800739297d43f14647dfe28182d3  ./examples/target/wasm32-unknown-unknown/release/abitest_0_frontend.wasm
+aaa701e9c0fc58fd65e5956bf07dbc17a9fe30170d776f8c734e833ecc30718e  ./examples/target/wasm32-unknown-unknown/release/abitest_1_backend.wasm
+4b88fc85c633e5413002b05ff66bec9ec653397f07f6799f889aeb7af14fb6e1  ./examples/target/wasm32-unknown-unknown/release/aggregator.wasm
+1e8c5a8ff13e83e228fa461eb049e7eb37600b980ef17e1e42aed2b9b14c8cc1  ./examples/target/wasm32-unknown-unknown/release/chat.wasm
+400c13da142185bb700bf6daaed1de58f38a6ebc98d52a67a0c93f425a806c77  ./examples/target/wasm32-unknown-unknown/release/database_proxy.wasm
+9035e13788e682a01e22c1ba42c125fccbfae4df296d7bd46318fb56044e19a2  ./examples/target/wasm32-unknown-unknown/release/hello_world.wasm
+c5646c404e5d4f4d354f1961e39bc9ca23989922741efee90b9f270625bdcd29  ./examples/target/wasm32-unknown-unknown/release/machine_learning.wasm
+3267614a8c860eef6449c40856cef55b04aceb95a206fe9d3cd70d2b3931d967  ./examples/target/wasm32-unknown-unknown/release/private_set_intersection.wasm
+656e89a34c1d964fd003e7421d204f96fd5c3853d27b142a9eec619f6f86f96b  ./examples/target/wasm32-unknown-unknown/release/running_average.wasm
+aea5aa8876b8158f0d781665e089307e9de53b59104407e71422a58ed4cff6d2  ./examples/target/wasm32-unknown-unknown/release/translator.wasm
+080c53ca4491dbd9e279725cc32abddca83d0da14e645915981455b5b9f8ecd2  ./examples/target/wasm32-unknown-unknown/release/trusted_information_retrieval.wasm
+f7e04fec862016c4c4dfdb816062cd5227cf8325b3340e5cdbf7d29ff7d5a714  ./oak/server/target/x86_64-unknown-linux-musl/release/oak_loader

googlebot added the cla: yes label Jul 3, 2020

wildarch force-pushed the use_vendored_prost branch from 8826c00 to 743585d Compare July 3, 2020 08:43

tiziano88 requested review from daviddrysdale and rbehjati July 3, 2020 11:20

daviddrysdale approved these changes Jul 3, 2020

View reviewed changes

tiziano88 reviewed Jul 3, 2020

View reviewed changes

wildarch force-pushed the use_vendored_prost branch from 743585d to 2eeb6a0 Compare July 3, 2020 12:19

wildarch mentioned this pull request Jul 3, 2020

Pass handles between nodes using Protocol Buffers #1108

Merged

10 tasks

rbehjati approved these changes Jul 3, 2020

View reviewed changes

wildarch force-pushed the use_vendored_prost branch from 2eeb6a0 to 743eae1 Compare July 3, 2020 14:56

wildarch force-pushed the use_vendored_prost branch from 743eae1 to 7e4edf8 Compare July 3, 2020 14:58

daviddrysdale merged commit 467f509 into project-oak:main Jul 6, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use the vendored prost everywhere. #1225

Use the vendored prost everywhere. #1225

wildarch commented Jul 3, 2020

wildarch commented Jul 3, 2020

wildarch commented Jul 3, 2020

daviddrysdale left a comment

daviddrysdale Jul 3, 2020

daviddrysdale Jul 3, 2020

tiziano88 Jul 3, 2020

wildarch Jul 3, 2020

tiziano88 Jul 3, 2020

wildarch Jul 3, 2020

tiziano88 Jul 3, 2020

wildarch Jul 3, 2020

tiziano88 Jul 3, 2020

rbehjati left a comment

rbehjati Jul 3, 2020

wildarch Jul 3, 2020

rbehjati Jul 3, 2020

wildarch Jul 3, 2020

rbehjati Jul 3, 2020

conradgrobler Jul 6, 2020

wildarch commented Jul 3, 2020

github-actions bot commented Jul 6, 2020

		// Keep clippy from complaining about a needless call to `Default::default()`.
		#[allow(clippy::needless_update)]

Use the vendored prost everywhere. #1225

Use the vendored prost everywhere. #1225

Conversation

wildarch commented Jul 3, 2020

Checklist

wildarch commented Jul 3, 2020

wildarch commented Jul 3, 2020

daviddrysdale left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rbehjati left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wildarch commented Jul 3, 2020

github-actions bot commented Jul 6, 2020