Move oak_runtime and oak_loader crates #1369
Conversation
There was a problem hiding this comment.
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",
|
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
force-pushed
the
tzn_oak_runtime_top_level
branch
3 times, most recently
from
1b0de53 to
84b09af
Compare
|
|
||
| [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" } |
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
Would cargo fmt --all --exclude prost --exclude prost-build --exclude prost-derive --exclude prost-types work?
There was a problem hiding this comment.
My version of cargo fmt does not seem to have an --exclude flag, does yours? If so, what version is it?
There was a problem hiding this comment.
Ah no, sorry – I didn't try it, I just followed rust-lang/cargo#2878 & rust-lang/cargo#4031 and assumed --exclude applied everywhere.
There was a problem hiding this comment.
Maybe create a third_party/prost/.rustfmt.toml file with an ignore directive that matches *.rs ?
There was a problem hiding this comment.
Great idea! I actually created it under third_party directly, and ignored everything. Thanks for the suggestion!
tiziano88
force-pushed
the
tzn_oak_runtime_top_level
branch
2 times, most recently
from
4146bf9 to
6b58ebe
Compare
|
I think everything is fixed now, including the things you mentioned (and some more). PTAL. |
There was a problem hiding this comment.
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!
|
|
||
| # List all workspaces. | ||
| declare -ar ALL_CRATES=("oak_utils" "oak_abi" "oak/server" "sdk" "examples" "runner" "experimental") |
There was a problem hiding this comment.
(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)
There was a problem hiding this comment.
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
force-pushed
the
tzn_oak_runtime_top_level
branch
from
6b58ebe to
e80b924
Compare
Done. |
|
Reproducibility Index: 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
|
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
Cloudbuild
cover any TODOs and/or unfinished work.
construction.