Refactor wasmtime build to use crates_universe-provided wasmtime-c-api-impl crate#501
Refactor wasmtime build to use crates_universe-provided wasmtime-c-api-impl crate#501leonm1 wants to merge 10 commits intoproxy-wasm:mainfrom
Conversation
leonm1
force-pushed
the
build/wasmtime
branch
3 times, most recently
from
6bcf94e to
144d005
Compare
bazel/cargo/wasmtime/Cargo.toml
Outdated
| wasmtime-c-api-macros = {git = "https://github.com/bytecodealliance/wasmtime", tag = "v24.0.0"} | ||
| # Fixes testdata build error due to missing crate_features = ["std"] | ||
| log = {version = "0.4.29", default-features = false, features = ['std']} | ||
| wasmtime-c-api-impl = {version = "24.0.0", default-features = false, features = ['cranelift', 'wasi', 'wat']} |
There was a problem hiding this comment.
This is definitely a wrong set of features.
At the very least, we need "runtime" feature to execute Wasm modules.
Also, we don't use "their" version of WASI, so "wasi" should be removed.
We didn't support "wat" before, so we probably shouldn't be adding it here either, since it will result in mismatch for WAT support across different Wasm engines.
There was a problem hiding this comment.
The wasmtime runtime, gc, and std features are enabled by the c-api Cargo.toml here: https://github.com/bytecodealliance/wasmtime/blob/f18f06e6dea00a78c06913061d952b26ed700b92/crates/c-api/Cargo.toml#L25
Removed wat and wasi.
There was a problem hiding this comment.
Ah, right. Those are features of wasmtime-c-api-impl and not wasmtime itself...
With that in mind, we don't need default-features = false, since that crates doesn't have any.
Signed-off-by: Matt Leon <mattleon@google.com>
Signed-off-by: Matt Leon <mattleon@google.com>
Signed-off-by: Matt Leon <mattleon@google.com>
Signed-off-by: Matt Leon <mattleon@google.com>
Signed-off-by: Matt Leon <mattleon@google.com>
Signed-off-by: Matt Leon <mattleon@google.com>
This reverts commit c19e738. Signed-off-by: Matt Leon <mattleon@google.com>
Signed-off-by: Matt Leon <mattleon@google.com>
Always uses prefixed wasmtime-c-api-impl, otherwise the prefixed implementation bitrots. Prefixes the headers in the repo rule to allow for globbing, which is not possible in a genrule. The crates_vendor-provided wasmtime-c-api-impl provided build allows us to upgrade wasmtime solely by changing the version number in the Cargo.toml file (and the corresponding repo in bazel/repositories.bzl for the C headers). Signed-off-by: Matt Leon <mattleon@google.com>
Signed-off-by: Matt Leon <mattleon@google.com>
PiotrSikora
left a comment
PiotrSikora
left a comment
There was a problem hiding this comment.
A few high-level comments:
- Why? Looking at the changes, the benefits don't seem very obvious.
- Always using prefixed variant seems fine (although, I think there was a reason why it wasn't the default - perhaps something with Envoy build?), but is that something that's at all relevant with the move to Wasmtime C++ API in #503?
- Using
wasmtime-c-api-impland always using prefixed variant are pretty separate changes, so might be good to split them off (but if they are too intertwined and would result in a throwaway work, then we can keep this as-is).
| wasmtime-c-api-macros = {git = "https://github.com/bytecodealliance/wasmtime", tag = "v24.0.0"} | ||
| # Fixes testdata build error due to missing crate_features = ["std"] | ||
| log = {version = "0.4.29", default-features = false, features = ['std']} | ||
| wasmtime-c-api-impl = {version = "24.0.0", default-features = false, features = ['cranelift']} |
There was a problem hiding this comment.
You should run cargo install cargo-cargofmt && cargo cargofmt to format this correctly. Perhaps hook it into CI as well.
| -e 's/\\[wasm_/\\[wasmtime_wasm_/g' \ | ||
| -e 's/wasmtime_config_wasm_/wasmtime_config_wasmtime_wasm_/g' \ | ||
| -e 's/(wasm_/(wasmtime_wasm_/g' {} \\; | ||
| cat >lib.rs <<EOF |
There was a problem hiding this comment.
Nit: use src/lib.rs to match convention.
| # This should be OBJCOPY, but bazel-zig-cc doesn't define it. | ||
| objcopy --redefine-syms=prefixed $(<) $@ | ||
| """, | ||
| """ + | ||
| # MacOS runners for GitHub do not ship with objcopy. Use the version bundled with the vendored toolchain. | ||
| select({ | ||
| "@platforms//os:macos": "$(location @llvm_toolchain_llvm//:objcopy) --redefine-syms=prefixed $(<) $@ ", | ||
| "//conditions:default": "objcopy --redefine-syms=prefixed $(<) $@ ", | ||
| }), | ||
| toolchains = ["@bazel_tools//tools/cpp:current_cc_toolchain"], | ||
| tools = select({ | ||
| "@platforms//os:macos": ["@llvm_toolchain_llvm//:objcopy"], | ||
| "//conditions:default": [], | ||
| }), |
There was a problem hiding this comment.
Did you try using $(OBJCOPY) as per comment? macOS should have llvm-objcopy available.
|
|
||
| [[package]] | ||
| name = "wasmtime-c-api-impl" | ||
| version = "24.0.6" |
There was a problem hiding this comment.
This doesn't match the version in Cargo.toml.
2 participants
Always uses prefixed wasmtime-c-api-impl, otherwise the prefixed implementation bitrots. Prefixes the headers in the repo rule to allow for globbing, which is not possible in a genrule.
The crates_vendor-provided wasmtime-c-api-impl provided build allows us to upgrade wasmtime solely by changing the version number in the repositories.bzl and Cargo.toml files.
Build off of #496