Upstream envoyproxy/envoy-wasm. #12546
Conversation
|
The Linux-x64 release seems to be a flake. I don't know that this code has ever been tested on Windows or arm64. Any suggestions on how to debug these things locally and/or disable the Wasm extensions for these environments? |
|
@jplevyak we can skip arm / windows, just modify build_setup / do_ci script to add bazel flags. |
|
@jplevyak how far does this get us to Wasm upstreaming? Is this mission complete? Would be good to document the support status after this PR in the commit message and version history. |
api/wasm/README.md
Outdated
| @@ -0,0 +1,42 @@ | |||
| Envoy hooks: | |||
There was a problem hiding this comment.
The api/ tree is for xDS APIs, why is there Wasm docs and build files here?
There was a problem hiding this comment.
This is all going to be removed. Pending envoyproxy/envoy-wasm#613 which I will merge in here and ping this thread.
| "ASMFLAGS": "-UDEBUG", | ||
| }, | ||
| lib_source = "@org_llvm_llvm//:all", | ||
| static_libraries = select({ |
There was a problem hiding this comment.
@lizan thoughts on relationship to https://github.com/envoyproxy/envoy/blob/master/tools/clang_tools/support/BUILD.prebuilt?
|
@htuch with this we will be 95% complete. There are a couple things which are still under development by interns and there is one thing that istio needs, but for all intents and purposes this will be enough for most users. |
Which flags do you mean? I don't think we have a flag to remove Wasm support i.e. we have --define wasm=enabled and --define wsam=wavm, but I don't think we have --define wasm=none. |
We used to have |
There was a problem hiding this comment.
Please toggle the default value of whether wasm is compiled here, so it is tested in CI:
Line 235 in 374dca7
Please add a flag for |
|
@lizan I have made v8 support optional. We now need to add --define wasm=enabled to make it enabled. Which C builds should it default to being enabled? |
In general, most/all things default to enabled/included. Also, please the settings here: https://github.com/envoyproxy/envoy/blob/master/bazel/README.md#disabling-optional-features |
| "src/**/*.cc", | ||
| ], | ||
| exclude = ["src/**/wavm*"], | ||
| # Note that the select cannot appear in the glob. |
There was a problem hiding this comment.
Wavm has a cmake build system, why can't we build it under rules_foreign_cc rather than this handcrafted blob thing?
There was a problem hiding this comment.
WAVM is built using rules_foreign_cc, this here is a BUILD file for Proxy-Wasm C++ host implementation, which optionally includes WAVM support.
There was a problem hiding this comment.
Is there any possibility here of not checking in this long glob file?
There was a problem hiding this comment.
Do you want all the files called out separately for each case? It will be longer, but perhaps that is OK?
There was a problem hiding this comment.
It looks like you folks are contributors to https://github.com/proxy-wasm/proxy-wasm-cpp-host. Can you add some Bazel BUILD files there?
So what's the default that we want to implement? V8 enabled by default, WAVM (only?) enabled with |
I don't know the tradeoffs or considerations for V8 vs WAVM. Someone else can chime on that. My request is just that there be a way to not compile either one in (which it looks like is now provided), and that we try to test as many variations as possible in CI. |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Only issue now is the MacOS CI |
|
OSX is messed up right now. We can force merge assuming this is ready to go and passes otherwise. Are there any remaining issues? cc @lizan |
|
@lizan should be back from vacation and will get this merged. Assigning over to him. |
PiotrSikora
force-pushed
the
upstream-wasm
branch
from
7e8c4ca
to
b841d8f
Compare
|
@mattklein123 @htuch all tests pass now, and this PR is synced with the current |
PiotrSikora
force-pushed
the
upstream-wasm
branch
from
ed20aaa
to
b390020
Compare
|
@PiotrSikora DCO is now broken. Can you just squash it all into a single commit and force push? |
|
Note that we are going to merge this without docs but we need to do a doc pass before we do any major PR about this feature. cc @louiscryan |
Signed-off-by: John Plevyak <jplevyak@gmail.com> Signed-off-by: Piotr Sikora <piotrsikora@google.com>
PiotrSikora
force-pushed
the
upstream-wasm
branch
from
b390020
to
a74e733
Compare
Signed-off-by: Piotr Sikora <piotrsikora@google.com>
|
huzzah! |
|
Cool! |
* use envoy master instead of envoy-wasm envoy-wasm has been merged to envoy envoyproxy/envoy#12546 * use istio/envoy release-1.8 as requested by @bianpengyuan #3055 (comment)
For an explanation of how to fill out the fields, please see the relevant section
in PULL_REQUESTS.md
Commit Message: Upstream envoyproxy/envoy-wasm.
Additional Description: This PR represents the proxy-wasm work which has been going on over the last 18 months in the envoyproxy/envoy-wasm repository. The vast majority of the changes herein are confined to extensions which are optional. Changes outside of the extensions are the minimal set required in order to support those extensions.
Risk Level: Low (if the extensions are not included).
Testing: Unit tests in envoyproxy/envoy and integration tests in istio/proxy.
Docs Changes: Docs will be provided in subsequent PRs.
Release Notes: Wasm plugins available optionally.
[Optional Runtime guard:]
[Optional Fixes #Issue] #4272
[Optional Deprecated:]