-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Move Wasmtime for .NET to the Wasmtime repo. #624
Move Wasmtime for .NET to the Wasmtime repo. #624
Conversation
This moves the Wasmtime for .NET implementation to the Wasmtime repo. Wasmtime for .NET is a binding of the Wasmtime API for use in .NET.
5a59617
to
623ac87
Compare
623ac87
to
ecd0183
Compare
When we merge this, I'll retire the "wasmtime.net" repo and update the docs so that everything points to |
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.
Nice! I won't really pretend to know anything about .NET though or claim any ability to review C# code. Some notes I'd have though are:
-
This differs from the Python extension quite a lot, in that this one is extremely fleshed out and well tested. The Python extension also currently only supports "integration through the module system" whereas this extension is more about exposing the API of the
wasmtime
crate itself to C#. (not sure if it makes sense to have a "module system integration" hook for C#). We should make sure to file an issue to getting the Python extension on par with this C# one (exposing API surface area and all that). I think this is my main note about this PR, which of course is not a blocker, just something for us to get to :) -
Does the .NET extension work by using the cdylib output of the wasmtime api crate? Or does it build its own custom cdylib?
-
It'd be awesome if we could avoid checking in lots of
*.wasm
files, would it be possible to tweak the wasmtime API to either take text or binary? (I'm not sure if this was part of the C# extension or baked into the API itself) -
Out of curiosity, and this is equally applicable to the Python extension, do you think that this'll cause much review churn/burden? I suspect most of us are pretty unfamiliar with C#, but I also suspect that the APIs used here are pretty stable, so I doubt it'll be a problem. Curious what you think though!
Unfortunately C# is a statically-typed compiled language that has no analogous concept for hooking up the underlying machinery as an "imported module" the way the Python extension works. The closest C# has is the I'd love to make this easier for .NET users, so I'm totally open to changing the API in any way that would help.
The former. The .NET API works via "P/Invoke" (which is .NET's term for FFI) and it loads the wasmtime library to invoke the C API directly.
Right now we're using the standard C API which operates just on Wasm blobs, so if we want to continue to stick to the API, we'd have to get that approved (I wager there might be some push back as not all runtime implementations may want to have to also compile wats).
I don't think there will be a lot of churn here yet. There's still some feature work to do (namely supporting tables, reference types [when they place nice with a host GC], and interface types), though. Some of that work will likely require changes to the C API to implement. @yurydelendik mentioned that he has at least some familiarity with C#, so when there are smaller PRs to review that aren't shuffling around lots of files he might be able to assist. The benefit of having this code live near the Wasmtime library is that it's much easier to test with local Wasmtime changes and gives additional test coverage of the C API in CI. |
Ok thanks for all the info! To be clear I'd personally be fine with this landing at any time. This does mean though that the Python extension as-is is significantly different from the .NET extension (and not in great ways, .NET seems much better I think). We'll want to resolve these differences, like:
For |
I don't think I agree with this: I think the support for "just using" a Wasm module as if it were a Python module is a strong feature, and should be the default. Would something similar to the proc-macro based Rust API work in C#, by any chance? Having a more explicit API, available through something like |
Unfortunately C# does not have macros or any similar compile-time code generation feature (although I do know the compiler devs have considered such a proposal). Roslyn, which is the C# compiler infrastructure, can be used to build up C# ASTs to generate source code that could be included in a build, but there would need to be build tooling around making that work. However, .NET does have runtime binding (and code generation) via reflection. I could envision a similar interface like: interface WasmMarkdown
{
string Render(string input);
}
void RenderHello()
{
using (var markdown = Wasmtime.LoadFile<WasmMarkdown>("markdown.wasm"))
{
Console.WriteLine($"{markdown.Render("# Hello, C#!")}");
}
} Of course, this would require interface types to be implemented, but I could see potentially hiding the C API machinery behind the scenes with an interface like this. |
Ah, that does seem quite promising!
Right, I certainly didn't mean this to be a short-term thing. We should probably have this discussion in another forum :) |
@tschneidereit oh I'd be down for that as well! I do think it's worthwhile to provide the nitty-gritty Rust-like internals API for all extensions, but I also agree that having a "super nice and easy to import" way of instantiating as the default ergonomic way (macros in Rust, I do think though we should probably rewrite the Python extension to be basically a blob of Python instead a blob of Rust. That blob of python would call into another blob of python we write which wraps |
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.
The c# code is read. Easy to follow. Thanks.
Right, I certainly didn't mean this to be a short-term thing. We should probably have this discussion in another forum :)
Agree
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.
Very cool! I'm also not the best person to review most of the code here, but the PR here looks reasonable to merge and continue development in tree.
This is by no means criticism in any shape or form, but wondering out loud, do we need to keep the |
I'm fine with removing any unnecessary wasm files we feel don't need to be there, but these are stripped files that total maybe a few kilobytes so far. |
This moves the Wasmtime for .NET implementation to the Wasmtime repo.
Wasmtime for .NET is a binding of the Wasmtime API for use in .NET.
The previous location for the implementation was here.