-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Replace WASI WITs with binary form #7929
Replace WASI WITs with binary form #7929
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome!
What is the advantage of this? It would make it harder to review changes (as git will refuse to show any diff) Also I believe distros generally require the original source for binary blobs to be included anyway and recompiled as part of the build process. https://www.debian.org/doc/debian-policy/ch-source.html#missing-sources-debian-missing-sources and https://docs.fedoraproject.org/en-US/packaging-guidelines/what-can-be-packaged/#prebuilt-binaries-or-libraries |
Just thought of this - this will eliminate the rust docs generated from the wit comments on the generated code, right? Those are pretty useful and so losing them is a significant regression... |
Good questions! The main benefit of this to me is that it more heavily discourages local modifications to WIT files and reaffirms that the source of truth of the WIT files is upstream in the WASI proposals themeslves, not here in Wasmtime (as has sort of historically ad-hoc-wise been the case but we're trying to shift now). In the limit with bytecodealliance/wit-bindgen#837 I hope to have You're right, however, that reviewing diffs won't be easy and if sources require non-binary artifacts won't work. One possible solution to that would be to check in the wasm text format instead of the wasm binary format as that would still pretty heavily discourage modifications but it would be more easily diff-able and wouldn't be a binary. Still a bit of a "generated wad of text", however, so I'm not sure how distros would feel about that. On the diff side it's also a property that any modifications will almost surely require changes on the Wasmtime side of things, but those are definitely more difficult to review than a change in the WIT itself. I think in general I'm not sure how best we can manage depending on the WASI spec here at this time, but this felt better to me than copying all the WITs in all the places. We're also somewhat "setting the trend" as well where if other projects or embeddings want to reference WASI this is an example of how they don't have to copy WITs but can instead copy artifacts at this time (ideally a package-manager-style-solution would work but that isn't fully mature yet AFAIK). As for documentation, that should actually be solved where the WASI proposals have a custom section with documentation inside of it so when |
5868023
to
c4f6f44
Compare
I've pushed up an update which is now what I think the longer-term vision for WITs-in-Wasmtime should look like. CI will fail until I publish bytecodealliance/wit-bindgen#838 but the idea is that theres a single wasm for (this doesn't move the needle on the binary-vs-not problem, just wanted to point out I made an orthogonal update to that) |
OK. I'm ok with the docs regression as long as we work on it soon :) |
Oh to clarify, the docs problem I believe is solved. Docs are encoded in the wasm binaries in a custom section, so the only issue I think with this is the binary-vs-not problem |
aaa2aab
to
a0c1082
Compare
This commit replaces the textual `*.wit` files in the Wasmtime tree with the binary form of a WIT package. The new WIT structure now looks like: * `crates/wasi/[email protected]` - serialized version of the `wasi-cli` proposal at the v0.2.0 tag. Created with `wasm-tools component wit --wasm` * `crates/wasi-http/[email protected]` - same as the above, but for `wasi-http`. * `crates/test-programs/wit` - WIT files used for tests such as a custom reactor. * `crates/test-programs/wit/deps/*.wasm` - WIT bindings to generate tests with. This is a duplicate of the `wasi-{cli,http}@0.2.0.wasm` files from above but additionally enables testing multiple versions in the future. The end state of this PR is that whenever WASI APIs are updated we'll need a tagged release to pull into Wasmtime to officially support.
a0c1082
to
f369df3
Compare
I've added this to the next Wasmtime meeting agenda in case a resolution is not met before then. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, this fell off my queue. This change looks good to me, are we waiting to resolve the concerns @bjorn3 brought up?
Yeah, @bjorn3 do you have thoughts on |
If you want to discourage edits, one option would be to store hashes of them separately and check that they match in a build script. That way you have to modify the source and then separately recompute the hash (or patch out the hash check, but you can leave a big comment near the hash check about why it exists). Or maybe you could gzip the wat files? That would also still allow extracting the original sources, but would still look like a binary blob if you open it in a text editor without extracting them first. |
I had some thoughts I was saving for the Wasmtime agenda item you had scheduled on Feb 29 (are we still discussing it then?) but to add some here:
|
Ah I put this on the agenda for Feb 29 to make sure it's not forgotten about, but I figure that if it's merged sooner I can take it off the agenda. I'm not sure how best to thread this needle myself. I don't know how to balance the conerns y'all have with how these WIT files are developed. For example I understand distros don't like binary blobs but a packager can always check out a versioned tag of a WASI proposal/repo and generate the exact same wasm binary. We could check in To me I feel that the binary distribution of WIT packages is a perfect use case here for integrating with downstream repos using WIT packages. Ideally we wouldn't even check in binary files and we'd simply have As I think more about this one other thing I like about the binary solution is that it makes it much easier to write down and execute instructions about updating these files. Currently it's copying a bunch of WIT files but you have to get that from multiple upstream WASI repositories and make sure you copy them all to the right places with the right names. You also need to make sure all the repos are checked out at the right version. As a reviewer I'd have to theoretically redo all that work to confirm it's all matching. With a binary file there's a single file in a single location under GitHub Releases for a single repo to go pick out. As a reviewer I could verify it's the expected file and move on from there. Well, in any case, I'm happy to discuss further on this PR, but I'm also ok deferring to next week's meeting too. |
Discussion from last week's Wasmtime meeting concluded that this isn't the way we'd like to go, and instead pursing WebAssembly/component-model#313 to have a single |
This is a follow-up to bytecodealliance#7929 where it was concluded that we want to retain textual WIT files in this repository. Currently today we have no automated checks for vendoring and re-vendoring requires hand-editing. To help incrementally improve this situation this commit adds a script to do all the vendoring for us. This way CI can verify that everything is generated with respect to the output of this script.
* Add a script to vendor WIT files This is a follow-up to #7929 where it was concluded that we want to retain textual WIT files in this repository. Currently today we have no automated checks for vendoring and re-vendoring requires hand-editing. To help incrementally improve this situation this commit adds a script to do all the vendoring for us. This way CI can verify that everything is generated with respect to the output of this script. * Review comments
This commit replaces the textual
*.wit
files in the Wasmtime tree withthe binary form of a WIT package. The new WIT structure now looks like:
crates/wasi/[email protected]
- serialized version of thewasi-cli
proposal at the v0.2.0 tag. Created with
wasm-tools component wit --wasm
crates/wasi-http/[email protected]
- same as the above, but forwasi-http
.crates/test-programs/wit
- WIT files used for tests such as a customreactor.
crates/test-programs/wit/deps/*.wasm
- WIT bindings to generatetests with. This is a duplicate of the
wasi-{cli,http}@0.2.0.wasm
files from above but additionally enables testing multiple versions in
the future.
The end state of this PR is that whenever WASI APIs are updated we'll
need a tagged release to pull into Wasmtime to officially support.