Vendor prost locally #1199

daviddrysdale · 2020-06-26T11:01:02Z

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.

wildarch · 2020-06-26T11:10:00Z

And then there were two (#1200) 🙈

tiziano88 · 2020-06-26T11:15:42Z

Which one will make it first to the main branch? 🏁

wildarch

Thanks David!

I'm happy to go with your PR 😄

wildarch · 2020-06-26T11:12:20Z

scripts/vendor_prost.sh

+  done
+
+  # Check the local version builds, which also regenerates Cargo.lock
+  cargo build


This is a good idea!

Could we maybe do something like PROTOC=protoc cargo test instead?

Good idea, done.

wildarch · 2020-06-26T11:13:08Z

third_party/prost/0001-Patch-to-generate-typesafe-Sender-and-Receiver.patch

@@ -0,0 +1,51 @@
+From df8d3e37563d373ac38cc90f75f97416ba766b61 Mon Sep 17 00:00:00 2001
+From: Daan de Graaf <daandegraaf9@gmail.com>


Should we also change this to daagra@google.com?

wildarch · 2020-06-26T11:15:25Z

scripts/vendor_prost.sh

+
+# Unzip and move into the vendored directory (stripping the prefix).
+unzip "$PROST_ZIP_PREFIX.zip"
+mv "$PROST_ZIP_PREFIX" vendored


I thought about this for a bit, maybe it's cleaner to put it under third_party/prost instead of third_party/prost/vendored

I thought that too, but didn't want to cause extra merge pain. If you're happy to include a directory change in your rebase, I'll move it.

Taken from https://github.com/danburkert/prost as of commit 802562779979 ("prost-build: fix compilation error when using --no-default-features"), with the exception of the prost-build/third-party subdirectory (which contains an unneeded copy of the protobuf distribution).

wildarch

Thanks! The changes look good to me.

scripts/vendor_prost.sh

tiziano88 · 2020-06-26T15:07:36Z

scripts/vendor_prost.sh

+PROST_ZIP_URL="https://github.com/danburkert/prost/archive/80256277997975948d257faf3f35c2890bf12787.zip"
+PROST_ZIP_SHA256="02c086f2d1b7aad110745ac701f001618cd8343f882fc9ce034aa007c4c53773"
+PROST_ZIP_PREFIX="prost-80256277997975948d257faf3f35c2890bf12787"


tiziano88 · 2020-06-26T15:08:58Z

scripts/vendor_prost.sh

+# support for the message_type field option.
+
+# Master branch as of 2020-06-05
+PROST_ZIP_URL="https://github.com/danburkert/prost/archive/80256277997975948d257faf3f35c2890bf12787.zip"


Perhaps make the commit id a separate variable and reuse here and below, so it's the only thing we need to change when updating.

Also make it consistent with what we do in Dockerfile:

oak/Dockerfile

Lines 143 to 152 in 56e2996

ARG bazel_tools_version=2.2.1

ARG buildifier_sha256=731a6a9bf8fca8a00a165cd5b3fbac9907a7cf422ec9c2f206b0a76c0a7e3d62

ARG buildifier_dir=/usr/local/buildifier/bin

ARG buildifier_bin=${buildifier_dir}/buildifier

ENV PATH "${buildifier_dir}:${PATH}"

RUN mkdir --parents ${buildifier_dir} \

&& curl --location https://github.com/bazelbuild/buildtools/releases/download/${bazel_tools_version}/buildifier > ${buildifier_bin} \

&& sha256sum --binary ${buildifier_bin} && echo "${buildifier_sha256} *${buildifier_bin}" | sha256sum --check \

&& chmod +x ${buildifier_bin} \

&& buildifier --version

tiziano88 · 2020-06-26T15:09:22Z

scripts/vendor_prost.sh

+cd "${SCRIPTS_DIR}/../third_party" || exit
+
+# Download the prost zip only if it does not yet exist in the filesystem.
+if [ ! -f "$PROST_ZIP_PREFIX.zip" ]; then


tiziano88 · 2020-06-26T15:10:31Z

scripts/vendor_prost.sh

+
+# Download the prost zip only if it does not yet exist in the filesystem.
+if [ ! -f "$PROST_ZIP_PREFIX.zip" ]; then
+    curl -L "$PROST_ZIP_URL" -o "$PROST_ZIP_PREFIX.zip"


curl --location (long options). also worth using shell redirect to write to the file, for consistency with dockerfile? or use --output there too? (not sure there is any benefit in either of them)

The scripts/vendor_prost.sh script updates the local copy of prost to the version included in it, applying our local patches. Also check in a fixed Cargo.lock for prost.

wildarch · 2020-06-26T15:59:46Z

Thanks for this PR @daviddrysdale! I'll send a follow-up PR to switch all Rust code to use the vendored version once this is merged.

github-actions · 2020-06-26T18:02:56Z

Reproducibility Index:

562715d812c1ed5397402d3f4b02b03addf416ccb4a8f071eac00a8015082b42  ./examples/target/wasm32-unknown-unknown/release/abitest_0_frontend.wasm
e89bb8621965e356156c939997a61fa1af30352159ae9e7a59f50a8eb101648d  ./examples/target/wasm32-unknown-unknown/release/abitest_1_backend.wasm
b256b1c1402a8f25381352047ee48cc9a07314d4227a64bd786369a17c61d1df  ./examples/target/wasm32-unknown-unknown/release/aggregator.wasm
44eff10ce70a232c84cc32c225e75eca695634abc9ecfbf543022aa0a5682cd7  ./examples/target/wasm32-unknown-unknown/release/chat.wasm
b28dc802af588ea6512974d2d9ec17557dd454f760b379ab80b11efbe444ecfd  ./examples/target/wasm32-unknown-unknown/release/hello_world.wasm
1123ddd109dd00155b0f59ccc2f3025974bacc82e15a4d1fe2a83ff6ceb2b3cd  ./examples/target/wasm32-unknown-unknown/release/machine_learning.wasm
66ae9e10c2d72cab7e629b2fea2ae98939649c391480ab255d589f5da8c7ae0f  ./examples/target/wasm32-unknown-unknown/release/private_set_intersection.wasm
0cd8cb0baae0c59852e775a697ca5408f975c1c1fdc53cc8f3a0e7d07e23f5af  ./examples/target/wasm32-unknown-unknown/release/running_average.wasm
15da62b4701e02ae731276dbe8bdfd29805871e7f863e8e15b1fe11983023159  ./examples/target/wasm32-unknown-unknown/release/translator.wasm
571d966c66ebf487d1f07ae0aba89fe22bcf8d09fcd861a490aec215c4b94aa9  ./examples/target/wasm32-unknown-unknown/release/trusted_information_retrieval.wasm
00bf32262f96d1da956c69eb9acfe116074dda5bcf22fd8eef3954b0a0d6ff0b  ./oak/server/target/x86_64-unknown-linux-musl/release/oak_loader

Reproducibility Index diff:

googlebot added the cla: yes label Jun 26, 2020

daviddrysdale mentioned this pull request Jun 26, 2020

Pass handles between nodes using Protocol Buffers #1108

Merged

10 tasks

daviddrysdale requested review from tiziano88 and wildarch June 26, 2020 11:03

wildarch approved these changes Jun 26, 2020

View reviewed changes

daviddrysdale requested a review from wildarch June 26, 2020 12:41

wildarch approved these changes Jun 26, 2020

View reviewed changes

scripts/vendor_prost.sh Show resolved Hide resolved

scripts/vendor_prost.sh Outdated Show resolved Hide resolved

tiziano88 approved these changes Jun 26, 2020

View reviewed changes

scripts: add script to update & patch local prost

470dbc3

The scripts/vendor_prost.sh script updates the local copy of prost to the version included in it, applying our local patches. Also check in a fixed Cargo.lock for prost.

wildarch mentioned this pull request Jun 26, 2020

Vendor and patch Prost #1200

Closed

6 tasks

daviddrysdale merged commit 6881ee8 into project-oak:main Jun 26, 2020

daviddrysdale deleted the vendor-prost branch June 26, 2020 17:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Vendor prost locally #1199

Vendor prost locally #1199

daviddrysdale commented Jun 26, 2020

wildarch commented Jun 26, 2020

tiziano88 commented Jun 26, 2020

wildarch left a comment

wildarch Jun 26, 2020

daviddrysdale Jun 26, 2020

wildarch Jun 26, 2020

daviddrysdale Jun 26, 2020

wildarch Jun 26, 2020

daviddrysdale Jun 26, 2020

wildarch left a comment

tiziano88 Jun 26, 2020

daviddrysdale Jun 26, 2020

tiziano88 Jun 26, 2020

daviddrysdale Jun 26, 2020

tiziano88 Jun 26, 2020

daviddrysdale Jun 26, 2020

tiziano88 Jun 26, 2020

daviddrysdale Jun 26, 2020

wildarch commented Jun 26, 2020

github-actions bot commented Jun 26, 2020

		@@ -0,0 +1,51 @@
		From df8d3e37563d373ac38cc90f75f97416ba766b61 Mon Sep 17 00:00:00 2001
		From: Daan de Graaf <daandegraaf9@gmail.com>

	ARG bazel_tools_version=2.2.1
	ARG buildifier_sha256=731a6a9bf8fca8a00a165cd5b3fbac9907a7cf422ec9c2f206b0a76c0a7e3d62
	ARG buildifier_dir=/usr/local/buildifier/bin
	ARG buildifier_bin=${buildifier_dir}/buildifier
	ENV PATH "${buildifier_dir}:${PATH}"
	RUN mkdir --parents ${buildifier_dir} \
	&& curl --location https://github.com/bazelbuild/buildtools/releases/download/${bazel_tools_version}/buildifier > ${buildifier_bin} \
	&& sha256sum --binary ${buildifier_bin} && echo "${buildifier_sha256} *${buildifier_bin}" \| sha256sum --check \
	&& chmod +x ${buildifier_bin} \
	&& buildifier --version

Vendor prost locally #1199

Vendor prost locally #1199

Conversation

daviddrysdale commented Jun 26, 2020

Checklist

wildarch commented Jun 26, 2020

tiziano88 commented Jun 26, 2020

wildarch 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

wildarch 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

wildarch commented Jun 26, 2020

github-actions bot commented Jun 26, 2020