Remove Bazel dependency for serialiser and docker images #1214

anghelcovici · 2020-06-30T12:01:36Z

Fixes #1058. I think this is the first step towards replacing the whole serialiser with a Rust version. I also want to make it a bit more self contained, but that is for the next version.

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.

tiziano88

a couple random quick comments

examples/abitest/config/build.sh

Dockerfile

tiziano88 · 2020-07-02T08:47:46Z

examples/abitest/config/build.sh

+
+readonly DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" >/dev/null 2>&1 && pwd )"
+readonly MODULES_DIR="${DIR}/../../target/wasm32-unknown-unknown/release"
+readonly OUT_DIR="${DIR}/../bin"


Are we settling on bin as the output directory? Why not... out? No need to change it anyways, we'll see later on.

cc @ipetr0v

I like the name bin better, as it is a place where we dump binary artefacts. Out seems... strange.

Is there an out directory for oak_loader?

No, this is for the examples. I don't think the oak_loader should be there

I think we should have it as oak/server/bin/oak_loader at some point.

+1 to @tiziano88 suggestion

examples/aggregator/config/build.sh

tiziano88 · 2020-07-02T08:48:38Z

examples/aggregator/config/build.sh

@@ -0,0 +1,11 @@
+#!/usr/bin/env bash
+
+readonly DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" >/dev/null 2>&1 && pwd )"


No idea what's going on here, is it trying to resolve its own location?

Could it be a function defined in scripts/common?

It can... but this is a bit different. As we source this, I wanted to get the folder of the script, not of the location where this is called. the method we use in most of the scripts works, as we mostly call them directly, not via source. I think it's a somehow unique usecase. As the comment below, I wouldn't spend too much time on this, as I do think we should remove these files soon and replace them with .tomls

tiziano88 · 2020-07-02T08:49:02Z

examples/aggregator/config/build.sh

@@ -0,0 +1,11 @@
+#!/usr/bin/env bash
+


Could you also source scripts/common here, or is it too complicated?

It's not, but afaict, these bash script will be there for a very short time (as @ipetr0v is replacing them with toml files). @ipetr0v, is that correct?

Yes, but app_config_serializer is the only thing that changes in #1219.
Examples are still compiled from scripts/build_example

examples/aggregator/server/Dockerfile

scripts/docker_run

ipetr0v · 2020-07-02T10:45:22Z

.gitignore

@@ -35,6 +35,9 @@
 # Ignore downloaded temporary files, such as the trusted root CA certificates.
 /downloads/

+# Ignore generated binary example files
+**/examples/**/bin


Is this directory used to store Wasm files and serialized application configs?

I think we should just pick a directory name (bin is fine, out was also suggested) and use it to generate everything everywhere (not just examples) (including binaries, wasm modules, additional stuff), rather than trying to get things out from target/wasm32-unknown-unknown and similar.

For cargo build, there is an --out-dir flag that should do that already.

Just pointing out that I believe --out-dir is still experimental

Dockerfile

ipetr0v · 2020-07-02T10:47:47Z

examples/Dockerfile

+FROM oak_docker:latest
+
+# This runs in the context of each example, so we just copy the bin
+COPY bin/config.bin .


Which example does it copy?
Or it is being run sequentially for every example?
Is bin/config.bin rewritten every time a new example is built?

Docker has a "context" in which is ran. You can think of it as the "parent folder". For each example image, I use the folder examples/EXAMPLE_NAME as the context, so bin/config.bin is the file for that e.g. examples/hello_world/bin/config.bin

It does read a bit weird though. I wonder whether having an explicit ARG config_file which is passed as docker build --build-arg=config_file=... would be more readable.

What does read a bit weird mean? This is how the Dockerfiles work... maybe the comment should be different?

The weirdness I was referring to is that usually a Docker image is built from a known location, but here you are expecting to move it around and be able to rebuild it in other folders.

We are building an image of an example, so it kind make sense.. but I do see where you are coming from.

ipetr0v · 2020-07-02T10:48:42Z

examples/abitest/config/build.sh

+
+readonly DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" >/dev/null 2>&1 && pwd )"
+readonly MODULES_DIR="${DIR}/../../target/wasm32-unknown-unknown/release"
+readonly OUT_DIR="${DIR}/../bin"


Is there an out directory for oak_loader?

ipetr0v · 2020-07-02T10:50:05Z

examples/abitest/config/build.sh

+readonly OUT_DIR="${DIR}/../bin"
+mkdir --parents "${OUT_DIR}"
+
+bazel run //oak/common:app_config_serializer -- \


So this PR doesn't remove Bazel for building examples.
Why was it moved to build.sh?

It's about the linked issue, which is removing Bazel from the invocation of the app_serializer. It is described in the first comment, but not reflected in the title, so I will change that. I think we will always build examples with Bazel (c++ ones), we don't have a goal to remove that.

ipetr0v · 2020-07-02T10:56:48Z

examples/aggregator/config/build.sh

@@ -0,0 +1,11 @@
+#!/usr/bin/env bash
+


Yes, but app_config_serializer is the only thing that changes in #1219.
Examples are still compiled from scripts/build_example

ipetr0v · 2020-07-02T10:58:57Z

scripts/run_clang_tidy

@@ -8,6 +8,7 @@ bazel_build_flags+=(
  '--experimental_action_listener=@io_kythe//kythe/cxx/tools/generate_compile_commands:extract_json'
  '--noshow_progress'
  '--noshow_loading_progress'
+  '--experimental_extra_action_top_level_only'


What does it do?

clang_tidy was not working, not really related to the PR. I am still not 100% sure why, but the "hack-ish" experimental code was trying to compile all bazel targets and getting into some GO code that it didn't configure well. This keeps it to only the first level of targets, which is exactly what we need.

scripts/build_example

tiziano88 · 2020-07-02T11:17:38Z

scripts/build_example

+  # Build the base server.
+  docker build -t oak_docker -f ./oak/server/Dockerfile ./oak/server
+  # Build and save the example
+  docker build -t "${EXAMPLE}_image" -f ./examples/Dockerfile "./examples/${EXAMPLE}"


Why the _image suffix? All docker images are... images.

I am just reusing the existing names. Happy to change it, but I think we use it in a lot of places. cc: @ipetr0v - any reason for using _image?

Since it's an example that is being loaded by Oak server inside Docker, it also could be "${EXAMPLE}_server".
But just "${EXAMPLE}" is also fine.

tiziano88 · 2020-07-02T11:18:07Z

scripts/build_example

+  docker build -t oak_docker -f ./oak/server/Dockerfile ./oak/server
+  # Build and save the example
+  docker build -t "${EXAMPLE}_image" -f ./examples/Dockerfile "./examples/${EXAMPLE}"
+  docker save "${EXAMPLE}_image" > "./examples/${EXAMPLE}/bin/${EXAMPLE}_image.tar"


What is this step for?

We first build the server image, on top of that we put the "application" and we save it so we can later "push it". I am not really sure we need the .tar step, but @ipetr0v almost convinced me in a slack chat :)

It's actually quite interesting. We could, potentially, have 1 single image, but the issue is that the Docker context would need to be the first common parent directory of the example and server, which is oak top-most directory. And because we have a .dockerignore there for the "overall" image, that conflicts with this. So this is why I made it a two step image. It's also nicer, since we can ideally build the server image once and add to it for each example

github-actions · 2020-07-03T08:44:31Z

Reproducibility Index:

78fc826365e07a99f98ee387490256fe285b8e726856d34a578b64d939ab52fb  ./examples/target/wasm32-unknown-unknown/release/abitest_0_frontend.wasm
383aaf9d743ec02b3aaf6d1edf6c7184fdf957ff814bf9572f0422860edceb5b  ./examples/target/wasm32-unknown-unknown/release/abitest_1_backend.wasm
03ca59bdc1acc25cf831b94f77c3d74d1060e7c72beb37dc8b8c8b7e1779459d  ./examples/target/wasm32-unknown-unknown/release/aggregator.wasm
c2bf618be440628fea36a6e01826b6d6f2c1e68fff94a8f9f932f7300b045458  ./examples/target/wasm32-unknown-unknown/release/chat.wasm
d6ae990b2ee7ccb0d64966a187ad689b8f63a3bb2ceb2dbe60db90b4996cd3da  ./examples/target/wasm32-unknown-unknown/release/database_proxy.wasm
bc8035e2363ce03809b17a6a96f97b7c218a705e6a0465b48b07f6c0bab7c803  ./examples/target/wasm32-unknown-unknown/release/hello_world.wasm
540cb9cdd92e1fefde401a8b7b34c6e7338b9c63f5bd6433e17d3015f727e929  ./examples/target/wasm32-unknown-unknown/release/machine_learning.wasm
cf2a697e1b14184c5deeea77298ab06d65bdb88125d99eb1934826de19e12c39  ./examples/target/wasm32-unknown-unknown/release/private_set_intersection.wasm
f11c4bf4b75c7a42873643090cf37f848ef03154ee99b09381f378b6707e214c  ./examples/target/wasm32-unknown-unknown/release/running_average.wasm
14ce660fa06fbe014b925b80b96aca319fc3c6fe64f65ee6ac6317b806364da1  ./examples/target/wasm32-unknown-unknown/release/translator.wasm
348a44c8023adc10992df6c810628aeaeaf8bb3b5dd1359c498f4d73e9cc5ed3  ./examples/target/wasm32-unknown-unknown/release/trusted_information_retrieval.wasm
09eccc430ef539f6abfdc02f84149b26814c1a914565a2ca9911b869c7327857  ./oak/server/target/x86_64-unknown-linux-musl/release/oak_loader

Reproducibility Index diff:

googlebot added the cla: yes label Jun 30, 2020

tiziano88 reviewed Jun 30, 2020

View reviewed changes

examples/abitest/config/build.sh Outdated Show resolved Hide resolved

examples/abitest/config/build.sh Outdated Show resolved Hide resolved

anghelcovici marked this pull request as draft June 30, 2020 12:11

anghelcovici mentioned this pull request Jul 1, 2020

Add Rust application configuration serializer #1219

Merged

2 tasks

anghelcovici marked this pull request as ready for review July 1, 2020 17:32

anghelcovici requested review from ipetr0v, daviddrysdale and tiziano88 July 1, 2020 17:32

Removed Bazel dependency and replaced with bash scripts

21f7317

tiziano88 reviewed Jul 2, 2020

View reviewed changes

ipetr0v approved these changes Jul 2, 2020

View reviewed changes

tiziano88 reviewed Jul 2, 2020

View reviewed changes

anghelcovici changed the title ~~Removed Bazel dependency and replaced with bash scripts~~ Remove Bazel dependency for serialiser and docker images Jul 2, 2020

Removed bazel scripts for Docker images and replaced with Dockerfiles

d9dbaf6

anghelcovici merged commit b2382c2 into project-oak:main Jul 3, 2020

anghelcovici linked an issue Jul 3, 2020 that may be closed by this pull request

Build Docker images without Bazel #1215

Closed

anghelcovici deleted the iovi_remove_bazel branch July 3, 2020 10:22

ipetr0v mentioned this pull request Jul 3, 2020

Add bin directory for oak_loader #1230

Merged

1 task

		@@ -0,0 +1,11 @@
		#!/usr/bin/env bash

		readonly DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" >/dev/null 2>&1 && pwd )"

Remove Bazel dependency for serialiser and docker images #1214

Remove Bazel dependency for serialiser and docker images #1214

Conversation

anghelcovici commented Jun 30, 2020

Checklist

tiziano88 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

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

anghelcovici Jul 2, 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

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

github-actions bot commented Jul 3, 2020

anghelcovici Jul 2, 2020 •

edited

Loading