Define example manifest files and add `--watch` flag #1105

tiziano88 · 2020-06-05T13:19:55Z

Each example has a manifest file that determines how to build and run the oak application and related clients.

The new --watch flag allows runner to keep running in the background and re-run tests or any commands when any file in the repository changes.

Example video: https://screencast.googleplex.com/cast/NDYzNjQyNTc2Mjg5NzkyMHwyY2VlYjM0Zi04Mw (Google internal only)

Checklist

Pull request includes prototype/experimental work that is under
construction.

anghelcovici · 2020-06-05T13:44:07Z

examples/aggregator/example.toml

+name = "aggregator"
+
+[modules.module]
+lang = "Rust"


You probably don't need both the lang and cargo_manifest? If it has a cargo_manifest it should be clear it's cargo :)

In pure Rust this would be an enum with different fields, and everything would be type safe. In principle, that could be done for TOML too, but the parser does not seem to like those kind of enums :/ toml-rs/toml-rs#390

anghelcovici · 2020-06-05T13:44:35Z

examples/aggregator/example.toml

+[clients.cpp]
+lang = "Cpp"
+additional_args = ["--bucket=test", "--data=1:10,2:20,3:30"]


Same here - if you have a clients.cpp maybe you don't need to specify the lang?

jul-sh · 2020-06-10T10:24:50Z

examples/abitest/example.toml

+  "--cert_chain=../../../../../../../../examples/certs/local/local.pem",
+  "--private_key=../../../../../../../../examples/certs/local/local.key",


I know it's a minor thing, but the ../../../../../../../../ paths honestly confuse me so much. But I guess with us moving away from bazel that's a seperate thing. :)

Yeah, that's orthogonal to this change, please ignore for now :)

Feel free to ignore this since it's not actionable, but purely to satisfy my curiosity,:

This basically navigates us out of the bazel runner directory and back into the working directory to load the certs, right? Had we considered instead copying the certs in into the bazel runner dir with a build rule?

Yes, bazel artifacts are nested deep inside the guts of bazel cache.

I don't think it's possible to just copy additional stuff in the cache folder, but I haven't looked into it closely.

I've seen some examples do it, but yeah don't know what's best practice. Relying on the cache path seems a bit like using internals, not APIs, but thankfully I'm told we're moving away from bazel anyway. :)

jul-sh · 2020-06-10T10:31:56Z

runner/src/main.rs

                }),
            },
        ],
    }
 }

+fn run_client(name: &str, client: &Client) -> Step {
+    match &client.target {
+        Target::Cargo { .. } => todo!(),


TIL that Rust had a literal todo macro. 😅

jul-sh · 2020-06-10T10:33:18Z

runner/src/main.rs

-                        vec![
-                            "run".to_string(),
-                            "--".to_string(),
-                            format!("//examples/{}/client:client", example.name),
-                            "--ca_cert=../../../../../../../../examples/certs/local/ca.pem"
-                                .to_string(),
-                        ]
+                    steps: example
+                        .clients
                        .iter()
-                        .chain(example.additional_client_flags.iter()),
-                    ),
+                        .map(|(name, client)| run_client(name, &client))
+                        .collect(),


That's nice, it seems like this PR is unofficially adding support for running multiple clients

jul-sh · 2020-06-10T10:43:58Z

runner/src/main.rs

+#[derive(serde::Deserialize, Debug)]
+#[serde(deny_unknown_fields)]
+enum Target {
+    Bazel { bazel_target: String },
+    Cargo { cargo_manifest: String },
+}


What I like about the manifest is that it specifies everything needed to run an example in one place, as opposed if-conditions in our various build scripts. :)

With that approach, it has occurred to me that maybe it would make sense to keep the target logic outside of the runner and instead in each manifest.

Eg instead of specifying a target, and the runner having instructions on how to build with that target, could the manifest simply list a command that should be run, leaving the details of the build system up to it?

Lemme know if that makes sense. :)

Also curious to hear @rbehjati thoughts, since this somewhat relates to our conversation in the Oak Channel.

Eg instead of specifying a target, and the runner having instructions on how to build with that target, could the manifest simply list a command that should be run, leaving the details of the build system up to it?

then we end up in the same copy paste collage that we already have in our scripts. the point of these manifests is to factor out as much common logic as possible.

To me the main advantage isn't to get rid of redundancy (which seems more common in our formatting scripts anyway), but rather to avoid the various if [[ "${EXAMPLE}" == 'foo' ]]; conditionals in our shell scripts, putting all the logic for an example in one place.

Then in order to contribute an example, one needs to merely understand the manifest, not have a mental model of all of our build scripts.

I think if we can also eliminate the some redundancy, that's a bonus. My fear with being too aggressive in abstracting-common logic is that we'll eventually have similar conditionals in the rust runner to account for special & one-of cases, which would defeat the advantage outlined above.

It's not that I strongly feel about needing to go the other way here, but I do think it's worth keeping in mind. :)

Sorry for the delay. I like what you are proposing @juliettepretot. Whenever there is a discussion of reuse, it reminds me of this hilarious talk.
I am also a bit worried about lines like this (although I think this line can easily be removed, and be added to each manifest):

oak/runner/src/main.rs

Line 408 in 2709157

"--ca_cert=../../../../../../../../examples/certs/local/ca.pem".to_string(),

Thanks for sharing, that was a fun talk. :)

Re the line you mentioned, that's unrelated to the manifest. The same line exists in our bash script, and thankfully it will be phased out as we switch away from bazel.

I think --ca_cert=... could be added to the additional_args list for each example, to make the manifests self-contained.

It is not an issue now, especially since we are going to remove bazel. But I think in general it is better to keep the manifests self-contained. I am also thinking of these manifest files as a form of documentation and a reference for how to run the examples. Similar to what you wrote :)

Then in order to contribute an example, one needs to merely understand the manifest, not have a mental model of all of our build scripts.

jul-sh

LGTM :)

tiziano88 · 2020-06-10T14:38:50Z

deny.toml

@@ -14,6 +14,7 @@ allow = [
  "Apache-2.0",
  "Apache-2.0 WITH LLVM-exception",
  "BSD-2-Clause",
+  "CC0-1.0",


FYI @conradgrobler I added this to the list of allowed licenses, as per go/thirdpartylicenses#unencumbered .

github-actions · 2020-06-10T16:01:21Z

Reproducibility Index:

d434db81c81d1b685fb4be9ca219f1e995b7c34e948feea069cd7f77d87632b8  ./examples/target/wasm32-unknown-unknown/release/abitest_0_frontend.wasm
9aa561d610eb08127c1423e5d86cef9b188a9675cb9c9edcf8aab67eae1ec988  ./examples/target/wasm32-unknown-unknown/release/abitest_1_backend.wasm
54f5c95d43fa1be478f939279e7cc9eca71e2bba0c233500a20954c83a15e35d  ./examples/target/wasm32-unknown-unknown/release/aggregator.wasm
3085cad7be998e21fe5e06869aaea2a2cf9964fde5e0f62f27de020938042987  ./examples/target/wasm32-unknown-unknown/release/chat.wasm
2c3d4c46498f5d4c811fe957f9d3250449630acf8a91fc3599ed8c6d5c64242b  ./examples/target/wasm32-unknown-unknown/release/hello_world.wasm
8ec460ba9e68b055364e406966783f6046367e39d69894bbcd5a6300d75d40e0  ./examples/target/wasm32-unknown-unknown/release/machine_learning.wasm
e3612f7bf9831864a71c18698b634e2e70b338e7643ecf97fb8c60fc3e376191  ./examples/target/wasm32-unknown-unknown/release/private_set_intersection.wasm
5dfd1fccc9f0a64cce2db298cafab0b4bd4e52cb238a5bf91444c20e40fcd75a  ./examples/target/wasm32-unknown-unknown/release/running_average.wasm
35c3857230d911d5c7c7f111872c9ab411356baff0e92288bd9eccbe6e38c2de  ./examples/target/wasm32-unknown-unknown/release/translator.wasm
0edcc0a56fbb20b84899a097f4c3b4a1ce9febb98e5b0ce97ff66440335269d9  ./examples/target/wasm32-unknown-unknown/release/trusted_information_retrieval.wasm
ce9279f19bb5219fbd709501031284832d493740689a2e23c2e984102c0a851c  ./target/x86_64-unknown-linux-musl/release/oak_loader

Reproducibility Index diff:

PR submission overlap between #1105 and #1117

googlebot added the cla: yes label Jun 5, 2020

anghelcovici reviewed Jun 5, 2020

View reviewed changes

tiziano88 force-pushed the tzn_example_manifest branch 3 times, most recently from f5b2352 to 3f7797b Compare June 10, 2020 10:18

tiziano88 changed the title ~~Define example manifest files~~ Define example manifest files and add --watch flag Jun 10, 2020

tiziano88 requested a review from jul-sh June 10, 2020 10:20

tiziano88 marked this pull request as ready for review June 10, 2020 10:20

tiziano88 force-pushed the tzn_example_manifest branch from 3f7797b to cda22e5 Compare June 10, 2020 10:21

jul-sh reviewed Jun 10, 2020

View reviewed changes

jul-sh approved these changes Jun 10, 2020

View reviewed changes

tiziano88 added 3 commits June 10, 2020 13:51

Define example manifest files

d00e2cf

Use enums for build target

1a84a5a

Add watch flag to runner

b2008e1

tiziano88 force-pushed the tzn_example_manifest branch 3 times, most recently from a693332 to 06436a1 Compare June 10, 2020 13:55

WIP

34c5d87

tiziano88 force-pushed the tzn_example_manifest branch from 06436a1 to 34c5d87 Compare June 10, 2020 14:37

tiziano88 commented Jun 10, 2020

View reviewed changes

tiziano88 merged commit 595eabd into project-oak:master Jun 10, 2020

tiziano88 deleted the tzn_example_manifest branch June 10, 2020 15:17

daviddrysdale mentioned this pull request Jun 13, 2020

abitest: re-enable server streaming tests #1150

Merged

6 tasks

daviddrysdale added a commit that referenced this pull request Jun 15, 2020

abitest: re-enable server streaming tests (#1150)

e007ec8

PR submission overlap between #1105 and #1117

jul-sh mentioned this pull request Jun 24, 2020

Always run all clients in run_examples #1176

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Define example manifest files and add `--watch` flag #1105

Define example manifest files and add `--watch` flag #1105

tiziano88 commented Jun 5, 2020 •

edited

Loading

anghelcovici Jun 5, 2020

tiziano88 Jun 5, 2020

anghelcovici Jun 5, 2020

jul-sh Jun 10, 2020

tiziano88 Jun 10, 2020

jul-sh Jun 10, 2020

tiziano88 Jun 10, 2020

jul-sh Jun 10, 2020

jul-sh Jun 10, 2020

jul-sh Jun 10, 2020

jul-sh Jun 10, 2020

tiziano88 Jun 10, 2020

jul-sh Jun 10, 2020 •

edited

Loading

rbehjati Jun 11, 2020

jul-sh Jun 12, 2020

rbehjati Jun 12, 2020

jul-sh left a comment

tiziano88 Jun 10, 2020

github-actions bot commented Jun 10, 2020

		"--cert_chain=../../../../../../../../examples/certs/local/local.pem",
		"--private_key=../../../../../../../../examples/certs/local/local.key",

Define example manifest files and add --watch flag #1105

Define example manifest files and add --watch flag #1105

Conversation

tiziano88 commented Jun 5, 2020 • edited Loading

Checklist

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jul-sh Jun 10, 2020 • edited Loading

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

jul-sh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jun 10, 2020

Define example manifest files and add `--watch` flag #1105

Define example manifest files and add `--watch` flag #1105

tiziano88 commented Jun 5, 2020 •

edited

Loading

jul-sh Jun 10, 2020 •

edited

Loading