Start a high-level architecture document for Wasmtime #3019
Conversation
This commit cleands up some existing documentation by removing a number of "noop README files" and starting a high-level overview of the architecture of Wasmtime. I've placed this documentation under the contributing section of the book since it seems most useful for possible contributors. I've surely left some things out in this pass, and am happy to add more!
github-actions
bot
added
the
wasmtime:docs
docs/contributing-architecture.md
Outdated
| "private" dependencies. While we strive to make the internal crates have proper | ||
| safe/unsafe annotations for their APIs in reality almost everything they do is | ||
| `unsafe` and much of it is not tagged as such. It's an eventual goal though to | ||
| have `unsafe` be properly annotated for all of the internal crates of Wasmtime. |
There was a problem hiding this comment.
I kinda feel like this last bit about safety in the private deps can be removed.
It isn't clear to me who this is written for, and who would benefit from reading this. On the one hand, users of Wasmtime don't really need to know about internals and where the safe vs unsafe line is drawn. On the other hand, contributors should (perhaps with guidance from reviewers) always strive for "proper" safety design/annotations, even if the current code doesn't fully live up to that in some area. I think we could expand / make explicit this second point, but what is written here seems like it will
- unnecessarily scare users of wasmtime, and
- let new contributors think that it is "okay" to be lax about safety boundaries because they've picked up the impression that everyone else is being lax too.
I don't think we intend for either of those things.
There was a problem hiding this comment.
My goal with this was to document the state of reality. The reality is that every time I look at wastime-runtime I see more bugs of "oh well this isn't marked unsafe but it should be". I fully agree this isn't a great reality but I also don't want to lure anyone into a false sense of security with a false reality.
Perhaps I could reword this to try to indicate this better?
| @@ -1,4 0,0 @@ | |||
| # `wasmtime-cranelift` | |||
There was a problem hiding this comment.
A note on this and other README.md deletions: there was a recent PR #2987 that added READMEs where missing so that a downstream packager could satisfy distro requirements. I think we should probably keep these around, even if they are somewhat trivial one-sentence descriptions, as it's fairly low-cost. (If the concern is that the docs could distract from the "real" architecture doc, perhaps we could include a pointer?)
There was a problem hiding this comment.
From that PR it's not clear to me at least if the requirement was that README.md must be present or "the one in the source isn't present in cargo package". Do you know if it's one of those two?
I personally find no use for one-liner readmes that explain nothing beyond the title of the crate, and I personally find it to be a worse experience not seeing a README in the first place and getting my hopes up that one exists. It's similar to the feeling of a function's documentation not explaining anything beyond the name of the function.
I also find that these sorts of README.md files which aren't at the root level of a repository are perennially neglected. If we really want to add documentation to these crates I think we should do it in a centralized location (the book) and more technical details would go in the source itself (e.g. a large crate doc comment)
There was a problem hiding this comment.
Ah! No, I think you're right, the issue looks like it was actually a metadata problem, i.e. already existing README not included in the package. So if that's the case, and packagers don't see an issue, I'm totally happy to get rid of them. (cc @olivierlemasle to confirm?)
There was a problem hiding this comment.
👍 Yes, my objective was to include the existing READMEs, but no README is not an issue for me.
|
Ok I'm gonna go ahead and merge this. We can of course continue to iterate in-tree, though! |
This commit cleands up some existing documentation by removing a number
of "noop README files" and starting a high-level overview of the
architecture of Wasmtime. I've placed this documentation under the
contributing section of the book since it seems most useful for possible
contributors.
I've surely left some things out in this pass, and am happy to add more!