Stream: git-wasmtime

Topic: wasmtime / PR #8939 test-programs: reorganize wasi conten...

Wasmtime GitHub notifications bot (Jul 12 2024 at 01:35):

iawia002 edited PR #8939.

Wasmtime GitHub notifications bot (Jul 12 2024 at 15:03):

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

Wasmtime GitHub notifications bot (Jul 12 2024 at 15:34):

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 the wasi or wasi-http crates. We will need the test-programs to include the WIT files of all APIs.

Wasmtime GitHub notifications bot (Jul 12 2024 at 15:39):

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 of wasi-cli will no longer include the WIT files for wasi-http.

Wasmtime GitHub notifications bot (Jul 12 2024 at 16:16):

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 the test-programs crate.

With the eventual goal of adding wasi-runtime-config, I would recommend one of two routes:

1. 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.
2. 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 single generate! call. There's need to be a separate generate! call for wasi:http/proxy and other worlds used by the test suite as well.

Wasmtime GitHub notifications bot (Jul 12 2024 at 17:08):

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 the wasi-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.

Wasmtime GitHub notifications bot (Jul 17 2024 at 15:17):

alexcrichton closed without merge PR #8939.

Last updated: Nov 22 2024 at 17:03 UTC