Move oak_runtime and oak_loader crates #1369

tiziano88 · 2020-08-18T16:32:50Z

Make them top-level crates, and fix imports transitively

This is more in line with standard Rust conventions, and also makes the
code more organized and easier to navigate.

Move proto folder to top level too.

Use more explicit dependencies between local crates (using path).

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.

daviddrysdale

Changes so far seem OK, but I suspect there are some things missing – from a tweaked grep or two:

./oak/server/rust/oak_loader/src/main.rs://! cargo run --manifest-path=oak/server/rust/oak_loader/Cargo.toml -- \
./oak/server/rust/oak_runtime/src/node/wasm/mod.rs:    // /oak/server/rust/oak_abi/src/lib.rs
./oak/common/label.cc:// Keep in sync with /oak/server/rust/oak_runtime/src/node/grpc/server/mod.rs.
./sdk/rust/oak_app_build/src/main.rs://! cargo run --manifest-path=oak/server/rust/oak_app_build/Cargo.toml -- \
./examples/hello_world/client/nodejs/index.js:// Keep in sync with /oak/server/rust/oak_runtime/src/node/grpc/server/mod.rs.
./examples/hello_world/client/android/java/com/google/oak/hello_world/MainActivity.java:  // Keep in sync with /oak/server/rust/oak_runtime/src/node/grpc/server/mod.rs.
./examples/aggregator/scripts/run_server:exec cargo run --release --target=x86_64-unknown-linux-musl --manifest-path=oak/server/rust/oak_loader/Cargo.toml -- \
./examples/translator/client/translator.go:// Keep in sync with /oak/server/rust/oak_runtime/src/node/grpc/server/mod.rs.
./examples/authentication/README.md:cargo run --manifest-path=oak/server/rust/oak_loader/Cargo.toml -- \
./scripts/generate_root_ca_certs:readonly TARGET_FILE="${SCRIPTS_DIR}/../oak/server/rust/oak_loader/src/certs/roots.pem"
./oak/module/oak_main.h:// #include "oak/proto/oak_api.pb.h"
./docs/sdk.md:[`oak::proto`](https://project-oak.github.io/oak/sdk/doc/oak/proto/index.html)
./examples/hello_world/module/cpp/BUILD:        # "//oak/proto:oak_api_cc_proto",
./examples/chat/proto/chat.proto:import "oak/proto/handle.proto";
./examples/chat/proto/BUILD:        "//oak/proto:handle_proto",
./examples/injection/proto/BUILD:        "//oak/proto:handle_proto",
./examples/injection/proto/injection.proto:import "oak/proto/handle.proto";
./examples/tensorflow/module/cpp/BUILD:        # "//oak/proto:oak_api_cc_proto",
./examples/tensorflow/module/cpp/BUILD:        # "//oak/proto:oak_api_cc_proto",
./examples/authentication/client/build.rs:        "../../../oak/proto",

tiziano88 · 2020-08-18T17:42:53Z

Ah yes thanks, I was actually waiting to see if CI passed and wanted to get a preliminary go / no-go from you before fixing all the outstanding things -- in fact I was probably too quick to add you as reviewer, I should have just cc'd you and made this clear.

Thanks for looking into it and doing the grepping already, I'll look into those shortly.

tiziano88 · 2020-08-18T23:48:21Z

oak_io/Cargo.toml

+
+[patch.crates-io]
+prost = { path = "../third_party/prost" }
+prost-build = { path = "../third_party/prost/prost-build" }
+prost-derive = { path = "../third_party/prost/prost-derive" }
+prost-types = { path = "../third_party/prost/prost-types" }


@daviddrysdale I have added these patches to various crates, which actually we should have done in the past already; if you look at the diff in the corresponding Cargo.lock, the previous version used to depend on prost from github, while now it depends on the local vendored copy (which is expected).

The problem is that when we run cargo fmt --all, it tries to format all those third party crates too for whatever reason. Have you come across this issue before? Do you have any idea how to prevent it?

The main thing I can think of is to reverse the logic in runner so that it only runs cargo fmt on non-workspace Cargo.toml files, i.e. the opposite of what it does now. I guess it may make that step more fine grain and therefore possibly a bit slower, but I don't think it matters that much.

WDYT?

Would cargo fmt --all --exclude prost --exclude prost-build --exclude prost-derive --exclude prost-types work?

My version of cargo fmt does not seem to have an --exclude flag, does yours? If so, what version is it?

Ah no, sorry – I didn't try it, I just followed rust-lang/cargo#2878 & rust-lang/cargo#4031 and assumed --exclude applied everywhere.

Maybe create a third_party/prost/.rustfmt.toml file with an ignore directive that matches *.rs ?

Great idea! I actually created it under third_party directly, and ignored everything. Thanks for the suggestion!

tiziano88 · 2020-08-19T13:54:05Z

I think everything is fixed now, including the things you mentioned (and some more). PTAL.

daviddrysdale

Might be nicer to move testdata/ to be under oak_runner (and adjust the paths in src/node/wasm/tests.rs) rather than having it be promoted to a new top-level directory.

(GitHub seems to have no way of making a comment on a renamed-without-changes file...)

Also: thanks for fixing various pre-existing outdated references along the way!

daviddrysdale · 2020-08-19T13:55:29Z

scripts/common


 # List all workspaces.
-declare -ar ALL_CRATES=("oak_utils" "oak_abi" "oak/server" "sdk" "examples" "runner" "experimental")


(pre-existing nit): the comment says "all workspaces" but the variable is ALL_CRATES (and in fact I think the list is a combination of the two)

Clarified the comment, I left the name as it is for now anyways.

Make them top-level crates, and fix imports transitively This is more in line with standard Rust conventions, and also makes the code more organized and easier to navigate. Move proto folder to top level too. Use more explicit dependencies between local crates (using `path`).

tiziano88 · 2020-08-19T14:41:07Z

Might be nicer to move testdata/ to be under oak_runner (and adjust the paths in src/node/wasm/tests.rs) rather than having it be promoted to a new top-level directory.

Done.

github-actions · 2020-08-19T15:48:28Z

Reproducibility Index:

697d5be162a4671a9fb262d7e97ad9c2bb2cf4365bf3dad4bd678f7dc7d1f8b5  ./examples/target/wasm32-unknown-unknown/release/abitest_0_frontend.wasm
b76f092469d01829ee686096f928bdef7da895f9066f1cbe0eb2ebef5d357581  ./examples/target/wasm32-unknown-unknown/release/abitest_1_backend.wasm
513dea5c6cf8a801a962b148d4469d5fc021d45fac1e9848b3236b9940d71bba  ./examples/target/wasm32-unknown-unknown/release/aggregator.wasm
e6f9d0b88acb1c47ec1acad03eb080d507ec1fcaa6c5d22197351633d4368490  ./examples/target/wasm32-unknown-unknown/release/chat.wasm
04af5bf30d1964e9b8e0728b2297425b93f10da2bc6bf66adcae83fedff22246  ./examples/target/wasm32-unknown-unknown/release/database_proxy.wasm
7116b241f484f473f4f4a77850c59009bbaebe5da098d797fcd2aa3c69c79984  ./examples/target/wasm32-unknown-unknown/release/hello_world.wasm
9d646d62700c8a3957b33ae405abecd6c013b4845f6ee410352f6b08da3597e7  ./examples/target/wasm32-unknown-unknown/release/http_server.wasm
2c1e246c5e66eaf88ea3000d2ec315bebe94235b86a0674265c259ae811faf96  ./examples/target/wasm32-unknown-unknown/release/injection.wasm
43c089dc594e9e99c53f32a3d776c61d85746d88d5195d89aa0ea17fab85dcb8  ./examples/target/wasm32-unknown-unknown/release/machine_learning.wasm
6f0e612c442ab5fd7814d14fac607648166056daf2fad77602267889176a113d  ./examples/target/wasm32-unknown-unknown/release/private_set_intersection.wasm
b42a3425ae81104f0c58df85a73c59ebaf5eb98929842f30e04fd340a574bcfd  ./examples/target/wasm32-unknown-unknown/release/running_average.wasm
bb98acd7cc1213138011de276d9904d22933dd9bd6259259ed00a39098ff3f9c  ./examples/target/wasm32-unknown-unknown/release/translator.wasm
3d34fff7feec1c8568517d270e8a74f82ed12612c26df6c32a3bd8251b1301e0  ./examples/target/wasm32-unknown-unknown/release/trusted_information_retrieval.wasm
15beb75f30e69b2f08969c2bc833cd25d68d98b04802ee43a774d654860c6285  ./oak_loader/target/x86_64-unknown-linux-musl/release/oak_loader

Reproducibility Index diff:

diff --git a/reproducibility_index b/reproducibility_index
index 97a6f2b..080a2da 100644
--- a/reproducibility_index
+++ b/reproducibility_index
@@ -1,14 +1,14 @@
-e0d18649da5e954fb776ebf3554fda49a83cd4ad492f265c883dd3bf8964ead9  ./examples/target/wasm32-unknown-unknown/release/abitest_0_frontend.wasm
-b8f14470d063daa3191af0cb7b09b58785a804d299e8d1427694b5267fb31a88  ./examples/target/wasm32-unknown-unknown/release/abitest_1_backend.wasm
-784fe260b67f874e9340e3a4a8ac98bab00cae6f479139eae3236ea44a51f917  ./examples/target/wasm32-unknown-unknown/release/aggregator.wasm
-4013cc3313faebf34986a7d75b0d3195125b87deac108c113ac96822819f171c  ./examples/target/wasm32-unknown-unknown/release/chat.wasm
-dccbc7ce3b1de430019824b61611a10c2b638af30d0fc58d69464732d8fe27bc  ./examples/target/wasm32-unknown-unknown/release/database_proxy.wasm
-705bfadde9870201b55a9bf572863c9148da406225662e5a8025cdf51ef1b627  ./examples/target/wasm32-unknown-unknown/release/hello_world.wasm
-3952e9874fcdf612235b016dc6133aefb0f7d79af3de18a9d39f9ed8320ae8d2  ./examples/target/wasm32-unknown-unknown/release/http_server.wasm
-2b80502982a744b980f9694223dbb58d33788078aea58bfc082d60346f0d2a2c  ./examples/target/wasm32-unknown-unknown/release/injection.wasm
-929ca52a81265081f0ad0fc7cda03ccdf0bb850c283666d4bedca2e7e81b7164  ./examples/target/wasm32-unknown-unknown/release/machine_learning.wasm
-cba804edbe9f2995e5a0866bc7244011eb0243c7de9b2a1d3af546fe07d83859  ./examples/target/wasm32-unknown-unknown/release/private_set_intersection.wasm
-0aeae2f99011270e2800f172fcd40c15b91f8f4b9794ab163673d33e8784bcb1  ./examples/target/wasm32-unknown-unknown/release/running_average.wasm
-864f5f3a50d30dee889f8e891266a73641b0d398b8c8ba0698a965dd839e7706  ./examples/target/wasm32-unknown-unknown/release/translator.wasm
-1f00a776eeeb3513053cb1933dbec9688ea399207a03d0e32d182dce5f9f1ea4  ./examples/target/wasm32-unknown-unknown/release/trusted_information_retrieval.wasm
-2fea9d717bd2d74d8a7b115ff57c385a08b8bd2e2602caf896796f97d4f4e2e2  ./oak/server/target/x86_64-unknown-linux-musl/release/oak_loader
+697d5be162a4671a9fb262d7e97ad9c2bb2cf4365bf3dad4bd678f7dc7d1f8b5  ./examples/target/wasm32-unknown-unknown/release/abitest_0_frontend.wasm
+b76f092469d01829ee686096f928bdef7da895f9066f1cbe0eb2ebef5d357581  ./examples/target/wasm32-unknown-unknown/release/abitest_1_backend.wasm
+513dea5c6cf8a801a962b148d4469d5fc021d45fac1e9848b3236b9940d71bba  ./examples/target/wasm32-unknown-unknown/release/aggregator.wasm
+e6f9d0b88acb1c47ec1acad03eb080d507ec1fcaa6c5d22197351633d4368490  ./examples/target/wasm32-unknown-unknown/release/chat.wasm
+04af5bf30d1964e9b8e0728b2297425b93f10da2bc6bf66adcae83fedff22246  ./examples/target/wasm32-unknown-unknown/release/database_proxy.wasm
+7116b241f484f473f4f4a77850c59009bbaebe5da098d797fcd2aa3c69c79984  ./examples/target/wasm32-unknown-unknown/release/hello_world.wasm
+9d646d62700c8a3957b33ae405abecd6c013b4845f6ee410352f6b08da3597e7  ./examples/target/wasm32-unknown-unknown/release/http_server.wasm
+2c1e246c5e66eaf88ea3000d2ec315bebe94235b86a0674265c259ae811faf96  ./examples/target/wasm32-unknown-unknown/release/injection.wasm
+43c089dc594e9e99c53f32a3d776c61d85746d88d5195d89aa0ea17fab85dcb8  ./examples/target/wasm32-unknown-unknown/release/machine_learning.wasm
+6f0e612c442ab5fd7814d14fac607648166056daf2fad77602267889176a113d  ./examples/target/wasm32-unknown-unknown/release/private_set_intersection.wasm
+b42a3425ae81104f0c58df85a73c59ebaf5eb98929842f30e04fd340a574bcfd  ./examples/target/wasm32-unknown-unknown/release/running_average.wasm
+bb98acd7cc1213138011de276d9904d22933dd9bd6259259ed00a39098ff3f9c  ./examples/target/wasm32-unknown-unknown/release/translator.wasm
+3d34fff7feec1c8568517d270e8a74f82ed12612c26df6c32a3bd8251b1301e0  ./examples/target/wasm32-unknown-unknown/release/trusted_information_retrieval.wasm
+15beb75f30e69b2f08969c2bc833cd25d68d98b04802ee43a774d654860c6285  ./oak_loader/target/x86_64-unknown-linux-musl/release/oak_loader

tiziano88 requested a review from daviddrysdale August 18, 2020 16:32

googlebot added the cla: yes label Aug 18, 2020

daviddrysdale reviewed Aug 18, 2020

View reviewed changes

tiziano88 force-pushed the tzn_oak_runtime_top_level branch 3 times, most recently from 1b0de53 to 84b09af Compare August 18, 2020 22:56

tiziano88 commented Aug 18, 2020

View reviewed changes

tiziano88 force-pushed the tzn_oak_runtime_top_level branch 2 times, most recently from 4146bf9 to 6b58ebe Compare August 19, 2020 12:16

tiziano88 requested a review from daviddrysdale August 19, 2020 13:54

daviddrysdale approved these changes Aug 19, 2020

View reviewed changes

tiziano88 force-pushed the tzn_oak_runtime_top_level branch from 6b58ebe to e80b924 Compare August 19, 2020 14:37

tiziano88 merged commit 620a7a0 into project-oak:main Aug 19, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move oak_runtime and oak_loader crates #1369

Move oak_runtime and oak_loader crates #1369

tiziano88 commented Aug 18, 2020

daviddrysdale left a comment

tiziano88 commented Aug 18, 2020

tiziano88 Aug 18, 2020

daviddrysdale Aug 19, 2020

tiziano88 Aug 19, 2020

daviddrysdale Aug 19, 2020

daviddrysdale Aug 19, 2020

tiziano88 Aug 19, 2020

tiziano88 commented Aug 19, 2020

daviddrysdale left a comment

daviddrysdale Aug 19, 2020

tiziano88 Aug 19, 2020

tiziano88 commented Aug 19, 2020

github-actions bot commented Aug 19, 2020


		# List all workspaces.
		declare -ar ALL_CRATES=("oak_utils" "oak_abi" "oak/server" "sdk" "examples" "runner" "experimental")

Move oak_runtime and oak_loader crates #1369

Move oak_runtime and oak_loader crates #1369

Conversation

tiziano88 commented Aug 18, 2020

Checklist

daviddrysdale left a comment

Choose a reason for hiding this comment

tiziano88 commented Aug 18, 2020

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

tiziano88 commented Aug 19, 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

tiziano88 commented Aug 19, 2020

github-actions bot commented Aug 19, 2020