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 16 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).
|
|
||
| [[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.
There was a problem hiding this comment.
Explicitly overrode semver for wasmtime-c-api-impl.
Done.
There was a problem hiding this comment.
Hm... For some reason all other Wasmtime crates are still at v24.0.6 and not v24.0.0, so maybe let's stick to v24.0.6.
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>
leonm1
left a comment
leonm1
left a comment
There was a problem hiding this comment.
Why? Looking at the changes, the benefits don't seem very obvious.
Two things:
- Updating wasmtime resulted in our rust_static_library in wasmtime.BUILD failing to build due to missing dependencies. With this current approach, the dependency tree is resolved by Cargo and we simply wrap it in rust_static_library. Updates which pull in new dependencies are trivial after this change and (in theory) will never require hunting down missing dependencies.
- For prefixing itself, using parts of the wasmtime_c_api (as opposed to the raw wasm_c_api in wasm.h / required for limits) involves a whole tree of c header files in the c-api crate. The approach we used previously, prefixing individual files via genrule, doesn't work with globs for all the required files (as bazel requires declaring all genrule outputs), so the toil and complexity of the old build would be increased significantly to support limits.
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?
Envoy build itself ran the multi runtime tests, so I don't think it required the non-prefixed version.
Using wasmtime-c-api-impl and 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).
Ack. I'm considering this a "change how we build wasmtime" PR, which makes sense semantically. While they are separable, due to higher priority tasks on my todo list, I simply don't have a couple hours to spare to split this.
|
|
||
| [[package]] | ||
| name = "wasmtime-c-api-impl" | ||
| version = "24.0.6" |
There was a problem hiding this comment.
Explicitly overrode semver for wasmtime-c-api-impl.
Done.
I don't believe that was ever the case, and I cannot find a code that would support that. Do you have a link? If not, could you open a draft PR against Envoy (you'll need to do that anyway at some point) and verify that it builds fine with Wasmtime - it can be either this PR or off the last PR in the stacked series. |
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