iawia002 edited PR #8939.
alexcrichton commented on PR #8939:
Could the WIT folder not be duplicated here? Since test-programs isn't published to crates.io it should be able to refer to the folder defined in the wasmtime-wasi or wasmtime-wasi-http crate
iawia002 commented on PR #8939:
Nope, it has to, as I mentioned before, this change is mainly for the upcoming addition of APIs like
wasi-runtime-config
. These APIs are not dependent on other APIs and are not depended on by other APIs, so their WIT files will not be included in thewasi
orwasi-http
crates. We will need thetest-programs
to include the WIT files of all APIs.
iawia002 commented on PR #8939:
And I plan to make some improvements to the
vendor-wit
script (pure bash script, no additional dependencies). Ultimately, each wasi API crate will only include the necessary WIT files. For example,wasi-runtime-config
will only include one WIT dependency, and the implementation ofwasi-cli
will no longer include the WIT files forwasi-http
.
alexcrichton commented on PR #8939:
It's a bit difficult to review this as it's setting up for future changes I at least personally have not yet seen or reviewed. In that sense it's difficult to review changes when only half the motivation is apparent, so I'd ask you bear with me in my questions and review feedback here.
Ideally we only have one copy of all WIT files in-tree. Even today it's a bummer we have one copy in wasmtime-wasi and one copy in wasmtime-wasi-http. How we manage WIT has changed over time but we've never settled on something everyone's happy with. The current status quo is the sort of "local maximum" where it's low-overhead to maintain and the management in CI is relatively simple.
In-tree there's also two main "styles" of sorts (not many data points to draw from) of managing WIT. One is wasmtime-wasi/wasmtime-wasi-http which are somewhat "tied at the hip". They're managed similarly and have the same WIT files. Another is wasi-nn which, like wasi-runtime-config, has no other WIT dependencies and is managed differently. Everything has a
wit_bindgen::generate!
call in thetest-programs
crate.With the eventual goal of adding wasi-runtime-config, I would recommend one of two routes:
- Follow the example of wasi-nn. This means that you'd add a few lines to the bash script to vendor wasi-runtime-config WIT files in a new directory. There'd be a new
wit_bindgen::generate!
call in test-programs as well. I don't think that it would require reorganizing things too much.- Refactor things before landing wasi-runtime-config to have a single
generate!
call in test-programs. That's sort of what this PR is doing but it's compromising on some of the initial design goals of our WIT management. I'll reiterate our WIT management is not great but we're also hoping to not make it worse (e.g. another copy of WIT in-tree).With the (1) route it should be possible to add everything today without changes. With (2) I would recommend first refactoring the
wit-bindgen
crate itself, for example it would probably be best to enable something like this:wit_bindgen::generate!({ path: ["../wasmtime-wasi/wit", "../wasmtime-wasi-http/wit", "../wasi-nn/wit"], inline: " include wasi:cli/imports; include wasi:http/imports; include wasi:nn/imports; ", });
I don't think that's fully supported in
wit-bindgen
today but it shouldn't be too too hard to add. That would enable not needing to copy around WIT files while still enabling a singlegenerate!
call. There's need to be a separategenerate!
call forwasi:http/proxy
and other worlds used by the test suite as well.
iawia002 commented on PR #8939:
Thanks for getting back to me, let's hold this for now.
My original intention for making these changes before implementing the
wasi-runtime-config
was to simplify the future PR and focus more on implementing thewasi-runtime-config
API itself.I was too fixated on getting these future tasks done beforehand. It seems I got the sequence of everything wrong. Let's go back to the beginning, after I have implemented the
wasi-runtime-config
, we can go back to those subsequent works.
alexcrichton closed without merge PR #8939.
Last updated: Dec 23 2024 at 12:05 UTC