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.

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

Drop C++ server code #1016

Merged
merged 23 commits into from
May 26, 2020
Merged

Drop C++ server code #1016

merged 23 commits into from
May 26, 2020

Conversation

daviddrysdale
Copy link
Contributor

@daviddrysdale daviddrysdale commented May 22, 2020

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 daviddrysdale added the WIP Work in progress label May 22, 2020
@daviddrysdale
Copy link
Contributor Author

@ipetr0v @tiziano88 : purely an FYI at this point, to make sure no-one spends time doing duplicating something that's already done or in-progress.

Please don't look at this yet.

(Unless you're about to work on something similar, in which case it's worth checking the commit list to see if it's got anything relevant).

@daviddrysdale daviddrysdale removed the WIP Work in progress label May 23, 2020
This removes the C++ Runtime implementation, including:
 - the gRPC server pseudo-Node (replaced by Rust version)
 - the gRPC client pseudo-Node (replaced by Rust version)
 - the storage pseudo-Node (dropped)
 - the Roughtime client pseudo-Node (dropped).
No longer any need for a C++ compile-time switch to disable
debugging features.
Now that cargo raze is not required, remove the pin on
tonic version 0.1.1 and update to latest.
The gRPC server-side functionality is now configured as an available
pseudo-Node type, so the top-level port is not used.
There is no longer any C++ code that builds ApplicationConfiguration
messages so drop the helper functions for building them.
There is currently no C++ code that uses these definitions.

If we ever add support (#744) for writing Nodes in C++ we'll need
a more complete ABI definition anyway, so not worth keeping one
constant and one type around in the meanwhile.
Changes required by commit 68fbbe3 ("Use Rust Loader for running
examples (#993)"):
 - Add and use a config that doesn't include a translator node.
 - Add C++ code that is equivalent to oak::grpc::server::init().

Change required by commit 64d3f90 ("Use bytes to represent gRPC
message body (#833)"):
 - Re-work hand-coded response message for current GrpcResponse proto
   message definition.

For #1009
There is no longer a need to have them visible to make Bazel
integration easier.

This is mostly a revert of commit 01644c9 ("runtime/rust: check
in prost-generated Rust files")

Fixes #850.
@daviddrysdale daviddrysdale marked this pull request as ready for review May 24, 2020 10:21
This is mostly a revert of commit 5f95a10 ("builds: drop support
for MacOS (#940)"), but with conflicts resolved and scripts updated.

The main changes from a revert are:
 - Reorder sections in the docs/INSTALL-OSX.md doc to reflect the
   primacy of Rust.
 - Add an OS conditional clause to avoid using a musl build for the
   server binary, because musl is Linux-specific (it is built on the
   Linux syscall layer).
 - Fiddle with one of the checks in check_formatting to get it working
   on both macOS and Linux.
Restore the commented out code, and fix the bug where the NodeStopper
instance for a new Node was registered against the creating Node's
ID rather than its own.

Fixes #1002.
Commit 41b2777 ("Rust gRPC server pseudo-node (#772)") changed
the gRPC server pseudo-Node to be created explicitly by the Application
rather than being implicitly created by the Runtime.  Update the docs
to reflect that.
@daviddrysdale
Copy link
Contributor Author

Probably easiest to review commit-by-commit – there are various preparatory commits to set things up so that the commit which removes the C++ server code is a pure deletion.

One thing still to do: this disables the hello_world Docker build, I'll look into re-enabling that in parallel.

Fixes: #945, #936, #850, #916, #1002
Obviates: #944, #875, #851, #724,
Also affects: #727, #942

@jul-sh
Copy link
Contributor

jul-sh commented May 26, 2020

image

👀 — So many rm'ed build files :)

Copy link
Collaborator

@tiziano88 tiziano88 left a comment

Choose a reason for hiding this comment

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

💯

@ipetr0v
Copy link
Contributor

ipetr0v commented May 26, 2020

Goodbye C++

@github-actions
Copy link

Reproducibility index:

decf268bc29fcca1d10c433d0b87835285b85fde961c7e154a701d62a6546cba  ./target/wasm32-unknown-unknown/release/abitest_0_frontend.wasm
b0841f70d32d423a83d731c0990546131ed394ace6a69a5fe0ca6fb06423999b  ./target/wasm32-unknown-unknown/release/abitest_1_backend.wasm
8d7981c0ed6ee8447160495121ab3acae4d5d28e1ce4c6163055593ea1c424d9  ./target/wasm32-unknown-unknown/release/aggregator.wasm
6d972065aede6b2e79c13ce73ff54e57ca2fe146a81610ae31df3ebfc8c55209  ./target/wasm32-unknown-unknown/release/chat.wasm
a6aee6a97cc394522ce4871879d8f7de66f218f43223d3a5ba5bcfc60541bdec  ./target/wasm32-unknown-unknown/release/hello_world.wasm
1134fc3e30193384b56b226db2a6915d6be694c7e1d7d15489f6f8516ce8bd0e  ./target/wasm32-unknown-unknown/release/machine_learning.wasm
5fa2081c5279512fbca000284cab4802a76e83164dd59543ad7da32545f30e17  ./target/wasm32-unknown-unknown/release/private_set_intersection.wasm
e814b30f662463c38b61916a51231f501217f780d472a1d48297a177a5b7d864  ./target/wasm32-unknown-unknown/release/running_average.wasm
be0395b448aac0885c99d3b8884c41eb2fde8c838843dafdfc45aa71bb6451d6  ./target/wasm32-unknown-unknown/release/translator.wasm
dff7d6df1f016173518d99c1f8a78e885c73a9466ceddaf90808f533b056e50c  ./target/x86_64-unknown-linux-musl/release/oak_loader

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants