Porting to build Rust wasm test datas with Bazel #613

Shikugawa · 2020-08-10T09:58:16Z

Signed-off-by: Shikugawa [email protected]

Commit Message: Port to enable building Rust wasm test datas with Bazel with some of cargo-raze supporting scripts.
Additional Description:
Risk Level: Low
Testing:
Docs Changes:
Release Notes:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Deprecated:]

Signed-off-by: Shikugawa <[email protected]>

PiotrSikora · 2020-08-10T11:35:04Z

test/extensions/filters/http/wasm/wasm_filter_test.cc

+        code = TestEnvironment::readFileToStringForTest(TestEnvironment::substitute(
+            absl::StrCat("{{ test_rundir }}/", kWasmRustDir, vm_configuration + "_rust.wasm")));
+      }
+    }


Why did you move all those test files out of their current location? It seems like an unnecessary change that requires extra switches.

This is because of the limitation of cargo-raze. We should generate build dependencies per Rust projects. But it will cause more complexity.

What's the issue? All those examples have the same dependencies, so cargo raze should always generate the same //cargo. Also, cargo raze is not executed as part of the build process, so it shouldn't really influence the location of checked-in *.rs and *.wasm files.

Having said that, this is me being pedantic, and if upstream doesn't mind and won't complain about this in the upstream PR, then we can merge this. cc @htuch

In this case, processing artifacts from cargo raze is pretty complex. That is because these should be placed on envoy specific paths. So I'd like to automate these processes. If we do that when Rust test data are scattered and required special library for new Rust test data in the future, it will be difficult to automate them.

Is the issue that we're not putting the test .rs in the same directory as the test itself? It seems desirable to do this IMHO, otherwise Rust/Wasm is some strange outlier.

Also, please don't manually construct paths like you do here, if you want runtime files, use https://github.com/envoyproxy/envoy/blob/466245b50e6d475be841e1c8d5f254dd6ea41272/test/test_common/environment.h#L114

Sure. But with desirable approach, we should manually edit cargo-raze generated scripts. IMHO it might have to be tech debt...., because we can't this process automatically.

jplevyak · 2020-08-11T18:04:23Z

Do we want this as part of the current upstream PR?

jplevyak · 2020-08-11T18:09:14Z

Looks like @htuch wants this in the upstream PR. @PiotrSikora @Shikugawa

htuch · 2020-08-12T00:39:59Z

bazel/external/cargo/remote/ahash-0.3.8.BUILD

@@ -0,0 +1,46 @@
+"""


Is the plan to check all of this into the main Envoy repo? Seems kind of terrifying from a maintenance perspective, even if automatically generated.

Exactly. But maintaining these automatically generated is not inevitable. I think that we should force to use this. And warn to use this script by DO NOT EDIT comment by inserting that "We must use tools/cargo-raze.sh".

or, automatically generate with cargo-raze.sh from pre-push script by checking the Cargo.toml had staged or not.

Meanwhile, yes, eventually I think it can made into rules_rust to generate WORKSPACE rule, just like rules_python's pip3_import.

Definitely want a tracking issue for this one, since this is a lot of boilerplate that we should be able to minimize. But I'd be willing to take it as MVP.

Signed-off-by: Shikugawa <[email protected]>

…est-flakes-bazel Signed-off-by: Shikugawa <[email protected]>

Signed-off-by: Shikugawa <[email protected]>

PiotrSikora

Mostly LGTM, with one seemingly unnecessary change.

PiotrSikora · 2020-08-14T17:56:12Z

test/extensions/bootstrap/wasm/test_data/logging_rust/Cargo.toml

@@ -10,9 +10,16 @@ proxy-wasm = "0.1"
 log = "0.4"

 [lib]
-crate-type = ["cdylib"]
+path = "src/*.rs"


I don't think you need this change? (here and in all other files).

Cargo.toml is only to trigger to generate bazel definition. So no need to take care.

I actually meant that the path = "src/*.rs" is not necessary, but my fault for not being clear about this.

Signed-off-by: Shikugawa <[email protected]>

PiotrSikora

LGTM, will defer to htuch@ to comment if you need any other changes to make this upstreamable.

Signed-off-by: Shikugawa <[email protected]>

htuch

Generally LGTM, a few remaining comments.

htuch · 2020-08-18T00:45:42Z

bazel/crates.bzl

+    _new_http_archive(
+        name = "raze__hashbrown__0_7_2",
+        url = "https://crates-io.s3-us-west-1.amazonaws.com/crates/hashbrown/hashbrown-0.7.2.crate",
+        type = "tar.gz",


These are all very similar, can you create a _rust_crate(..) macro that factors out all the boilerplate? it should be possible to have just _rust_crate(name="hashbrown", version="0.7.2") and it can fill the rest I think.

Those are all automatically generated by cargo raze tool, using macros here would turn this into manual work.

htuch · 2020-08-18T00:46:51Z

bazel/external/cargo/remote/ahash-0.3.8.BUILD

@@ -0,0 +1,46 @@
+"""


Definitely want a tracking issue for this one, since this is a lot of boilerplate that we should be able to minimize. But I'd be willing to take it as MVP.

htuch · 2020-08-18T00:47:53Z

test/extensions/filters/http/wasm/test_data/BUILD

+wasm_rust_binary(
+    name = "shared_queue_rust.wasm",
+    srcs = ["shared_queue_rust/src/lib.rs"],
+    rustc_flags = ["-Clink-arg=-zstack-size=32768"],


Why do we have this stack size boiler plate?

There is one test that verifies linear memory size for test_rust.rs, but it can be removed from other tests... and test_rust.rs can be probably completely removed now that we have tests written using Proxy-Wasm Rust SDK.

Just deleted unused stack size specification right now. I'm not sure if the test_rust.rs is required so missing it.

Signed-off-by: Shikugawa <[email protected]>

Porting to build Rust wasm test datas with Bazel

e2b9313

Signed-off-by: Shikugawa <[email protected]>

Shikugawa requested review from jplevyak, lizan and PiotrSikora as code owners August 10, 2020 09:58

Shikugawa added 2 commits August 10, 2020 19:04

module

88cbcce

Signed-off-by: Shikugawa <[email protected]>

exclude crates.bzl

ab0917d

Signed-off-by: Shikugawa <[email protected]>

PiotrSikora reviewed Aug 10, 2020

View reviewed changes

jplevyak requested a review from PiotrSikora August 10, 2020 20:27

jplevyak mentioned this pull request Aug 11, 2020

Upstream envoyproxy/envoy-wasm. envoyproxy/envoy#12546

Merged

htuch reviewed Aug 12, 2020

View reviewed changes

Shikugawa added 6 commits August 14, 2020 16:09

fix

3b7147a

Signed-off-by: Shikugawa <[email protected]>

Merge branch 'master' of github.com:envoyproxy/envoy-wasm into rust-t…

e177d1a

…est-flakes-bazel Signed-off-by: Shikugawa <[email protected]>

fix

7cd18e2

Signed-off-by: Shikugawa <[email protected]>

fix error

e4be6ad

Signed-off-by: Shikugawa <[email protected]>

Kick CI

bd3c13c

Signed-off-by: Shikugawa <[email protected]>

Kick CI

3920e7f

Signed-off-by: Shikugawa <[email protected]>

PiotrSikora reviewed Aug 14, 2020

View reviewed changes

Shikugawa added 4 commits August 15, 2020 10:05

fix

9d5b435

Signed-off-by: Shikugawa <[email protected]>

Kick CI

b1da231

Signed-off-by: Shikugawa <[email protected]>

Kick CI

57723bd

Signed-off-by: Shikugawa <[email protected]>

Kick CI

c78bd2e

Signed-off-by: Shikugawa <[email protected]>

PiotrSikora approved these changes Aug 15, 2020

View reviewed changes

Shikugawa added 4 commits August 16, 2020 09:00

Kick CI

f8d0dc7

Signed-off-by: Shikugawa <[email protected]>

Kick CI

dae4a91

Signed-off-by: Shikugawa <[email protected]>

Kick CI

ee04c91

Signed-off-by: Shikugawa <[email protected]>

Kick CI

545cb49

Signed-off-by: Shikugawa <[email protected]>

htuch reviewed Aug 18, 2020

View reviewed changes

remove unused stack size limitation

dab96a6

Signed-off-by: Shikugawa <[email protected]>

Shikugawa added 4 commits August 19, 2020 10:02

fix

2b92a8d

Signed-off-by: Shikugawa <[email protected]>

fix

cff866a

Signed-off-by: Shikugawa <[email protected]>

Kick CI

513262e

Signed-off-by: Shikugawa <[email protected]>

Kick CI

43f8f7c

Signed-off-by: Shikugawa <[email protected]>

PiotrSikora merged commit 057ab1f into envoyproxy:master Aug 25, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Porting to build Rust wasm test datas with Bazel #613

Porting to build Rust wasm test datas with Bazel #613

Shikugawa commented Aug 10, 2020

PiotrSikora Aug 10, 2020

Shikugawa Aug 10, 2020

PiotrSikora Aug 12, 2020

Shikugawa Aug 13, 2020 •

edited

Loading

htuch Aug 13, 2020

htuch Aug 13, 2020

Shikugawa Aug 14, 2020

jplevyak commented Aug 11, 2020

jplevyak commented Aug 11, 2020

htuch Aug 12, 2020

Shikugawa Aug 12, 2020

Shikugawa Aug 12, 2020

lizan Aug 12, 2020

htuch Aug 18, 2020

PiotrSikora left a comment

PiotrSikora Aug 14, 2020

Shikugawa Aug 15, 2020

PiotrSikora Aug 15, 2020

PiotrSikora left a comment

htuch left a comment

htuch Aug 18, 2020

PiotrSikora Aug 18, 2020

htuch Aug 18, 2020

htuch Aug 18, 2020

PiotrSikora Aug 18, 2020

Shikugawa Aug 19, 2020

Porting to build Rust wasm test datas with Bazel #613

Porting to build Rust wasm test datas with Bazel #613

Conversation

Shikugawa commented Aug 10, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Shikugawa Aug 13, 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

jplevyak commented Aug 11, 2020

jplevyak commented Aug 11, 2020

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

PiotrSikora 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

PiotrSikora left a comment

Choose a reason for hiding this comment

htuch 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

Shikugawa Aug 13, 2020 •

edited

Loading