Skip to content
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

feat: Implement .NET Host SDK #119

Merged
merged 20 commits into from Dec 8, 2022
Merged

Conversation

mhmd-azeez
Copy link
Collaborator

@mhmd-azeez mhmd-azeez commented Dec 1, 2022

I am working on a .NET host for extism. My plan is to do the following:

  • Implement a proof of concept to make sure things are possible
  • Write docs for the C# API so that the users get a nice IDE experience
  • Create a github action to publish the NuGet packages
    • Edit ci.yml to include .NET Sdk
    • Create release-dotnet.yml to release Extism.Sdk nuget package
    • Maybe Create release-dotnet-native.yml to release Extism.runtime.win nuget package
  • Test on Linux (Help needed)
  • Test on Mac (Help needed)
  • Expose all of the Extism functions
  • Write automated tests
  • Edit README show that the there is a .NET SDK. Probably we should not do this until we have a docs page.
  • Use the Extism.runtime.win-x64 package in the sample project

Out of scope for this PR:

  • Json Serialization/Desererialization support

@alistairjevans
Copy link
Contributor

Hey @mhmd-azeez; external observer, thanks for raising this PR! Let me know if you want any assistance, I've got use-cases in mind...

Have you considered generating a native-assembly nuget package that contains a pre-built extism binary for various platforms? Similar to how (https://github.com/jedisct1/libsodium/actions/runs/3560505523)[libsodium does it]? That would give a nice out-of-the-box experience for .NET devs. Your may choose not to include Android and iOS builds in there by default out of the pain of setting up those builds, but desktop and server platforms should be fairly straightforward?

@mhmd-azeez
Copy link
Collaborator Author

Hello @alistairjevans I definitely plan to generate native assembly nuget packages. And if you're interested, we can work on this PR together.

@nilslice mentioned that they can drop the binaries in a folder in GitHub actions

@alistairjevans
Copy link
Contributor

Sure, I'm up for helping out; how you want to do it? I can fork your repo and merge into there to make this work? Or collaborate on your repo directly? Your choice.

@mhmd-azeez
Copy link
Collaborator Author

mhmd-azeez commented Dec 2, 2022

Fantastic! @alistairjevans I just invited you to my repo, do you agree with the scope in the PR description? My idea is to get something up and running and then in subsequent PRs add more layers and nice-to-haves and even a .NET PDK

@alistairjevans
Copy link
Contributor

alistairjevans commented Dec 2, 2022

Yeah, I think the scope makes sense; to my mind, step 1 is to build a solid (but thin) interop layer over the imported methods, and get that CI and tests in place. There's always going to be people who want to be able to reach directly for the methods that work with raw ReadOnlySpan<byte>. That's more or less what you already have, and what this PR is for.

Once that's in place, a higher-level API that helps with serialising data to/from the plugins, a POCO representation of the manifest, and so on, will help most users pick this up more easily. I haven't seen that there's a fixed standard for passing structured types to/from the PDK; JSON seems to be included in the PDK by default, but it's a relatively inefficient format for passing arguments, so I can imagine guidance (codegen?) in future for defining interop data structures.

I wonder if @nilslice has considered anything like a protobuf-style spec for defining parameters to plugin methods; this would allow you to arrive at a standard structure definition for all the supported PDKs from a single source.

The use-case I'm thinking about for this is plugins for processing network packets in near-real-time, so would want to keep the method argument passing overheads low.

It looks like you can only pass a single data pointer to each plugin method, and I'm assuming that the data is copied into the WebAssembly context rather than providing direct access to the provided buffer.

@bhelx
Copy link
Contributor

bhelx commented Dec 2, 2022

Hey @mhmd-azeez I'd be happy to take some of these tasks if you don't want to do them:

  • Test on Linux (Help needed)
  • Test on Mac (Help needed)
  • Create a github action to publish the NuGet packages (we can share this one or I can do it for you. I'll get an account and secret setup at least)

@bhelx
Copy link
Contributor

bhelx commented Dec 2, 2022

Okay we now have a Nuget API key stored as github secret: NUGET_API_KEY
Should be available from actions. Nuget user is extism-bot like our other distributions.

@mhmd-azeez
Copy link
Collaborator Author

@bhelx Thank you very much! Testing is something your help will speed things up and as for the pipeline I'll need your help in fetching the native binaries. Will let you know once I get there

@bhelx
Copy link
Contributor

bhelx commented Dec 2, 2022 via email

@mhmd-azeez
Copy link
Collaborator Author

mhmd-azeez commented Dec 2, 2022

@bhelx how can I make sure that logging works? currently I set log level to Trace and the extism_log_file returns true, but the log file is always empty, even when I pass in an incomplete wasm byte array.

Edit: I just pushed some code that I used to test the logging, I haven't been able to force it to write anything to the log file so far.

@bhelx
Copy link
Contributor

bhelx commented Dec 2, 2022

Edit: I just pushed some code that I used to test the logging, I haven't been able to force it to write anything to the log file so far.

I seem to recall @nilslice or @zshipko may have something to say on this. I noticed this issue in another host library. May be a bug that we should track and fix. If it's returning true though I say keep going and don't worry about it! It should error out if you are doing it wrong

@zshipko
Copy link
Contributor

zshipko commented Dec 2, 2022

Looks like the logging issue is a bug - I just made a PR with a fix: #123

@mhmd-azeez
Copy link
Collaborator Author

mhmd-azeez commented Dec 2, 2022

For the nuget packages, I think OpenCVSharp's approach is a good fit because Extism's binaries are pretty big (10 - 30 MB per platform). So we shouldn't force people to copy all of these binaries with their apps. Here is what I have in mind:

  1. ExtismSharp: Contains the .NET host, targets .NET Standard 2.1. Later on if there is demand we can lower it to .NET Standard 2.0. But for now, I want to support Span and don't want to litter the codebase with #ifs.
  2. ExtismSharp.runtime.win: Contains the win-x64 binary. @bhelx @nilslice Do you think we should include the MSVC binary or the gnu one? Or should we offer both choices?
  3. ExtismSharp.runtime.linux: Contains both aarch64 and x86_64 linux binaries.
  4. ExtismSharp.runtime.osx: Contains both aarch64 and x86_64 apple binaries.

This gives the user freedom to use the systemwide installations or include specific binaries with their apps. These are the OpenCVSharp packages: https://www.nuget.org/profiles/schimatk

@alistairjevans what do you think?

Note 1: I think if we go through that route, we should publish the runtime nuget packages in the https://github.com/extism/extism/actions/workflows/release.yml pipeline since these packages deployments don't have anything to do with the C# code. We can create a new pipeline that only publishes the ExtismSharp nuget package. @bhelx @nilslice

Note 2: Including the binaries is mostly important for Windows, because extism already supports systemwide installation for macOS and linux. So we can even skip them. @bhelx @nilslice

Note 3: The runtime names are based on .NET's runtime identifiers.

@nilslice
Copy link
Member

nilslice commented Dec 2, 2022

  1. ExtismSharp.runtime.win: Contains the win-x64 binary. @bhelx @nilslice Do you think we should include the MSVC binary or the gnu one? Or should we offer both choices?

I am not sure I really know the impact of deciding either one. Are there many Windows users who use the gnu toolchain vs. MSVC? Sorry for my naivety on the subject :)

Note 2: Including the binaries is mostly important for Windows, because extism already supports systemwide installation for macOS and linux. So we can even skip them. @bhelx @nilslice

I think we can skip these for macos and linux. This is assuming that dotnet doesn't expect any special treatment on those platforms with regard to how it locates shared libraries. I think it would use the system's linker anyway?

@alistairjevans
Copy link
Contributor

Do you think there's a reason not to lay claim to the Extism prefix rather than ExtismSharp? There's unlikely to be a "non-sharp" nuget package separate to this one.

I would then be tempted to ship Extism.Host for the host library, to give you space for Extism.Pdk.

I think storing the runtime packages separately is a good idea. Would suggest a couple of things:

  • The win package should probably include an arm64 build of extism as well if possible, there are a selection of Windows arm64 devices out there. Will extism build for Windows arm64? Since it's rust, should be fairly straightforward?

  • Using the MSVC compiler may give better windows support, and is available out-of-the-box on GitHub runners I believe. Documentation will need to indicate the VC++ redist package is needed though.

  • Would propose an additional, optional, Extism.runtime.all which pulls in all supported runtime packages for those unlucky few (like myself), that deploy an app onto all of the above. It allows updates of the package to pull in any new supported platforms without needing to explicitly add them. Useful for devs building apps for user devices.

  • I'd personally say that it would be good to keep that Linux runtime package, specifically to support the single-file publishing model; makes it easy to distribute apps to client Linux devices that way. Are we expecting a generic 'linux' package to be sufficient? Or are we going to need distro-specific ones?

@nilslice
Copy link
Member

nilslice commented Dec 2, 2022

+1 to naming Extism over ExtismSharp, but if this is a pattern that is more welcome and familiar to dotnet ecosystem, I don't mind it.

@mhmd-azeez
Copy link
Collaborator Author

mhmd-azeez commented Dec 2, 2022

@nilslice @alistairjevans

I'd personally say that it would be good to keep that Linux runtime package, specifically to support the single-file publishing model; makes it easy to distribute apps to client Linux devices that way. Are we expecting a generic 'linux' package to be sufficient? Or are we going to need distro-specific ones?

Since publishing these packages is fairly straightforward, I think we should just publish for all 3 platforms.

The win package should probably include an arm64 build of extism as well if possible, there are a selection of Windows arm64 devices out there. Will extism build for Windows arm64? Since it's rust, should be fairly straightforward?

I didn't include it because release artifacts don't have it. If @nilslice @bhelx think it's possible, then we should definitely add it.

@alistairjevans I agree with your naming suggestions, I will rename the projects accordingly

Would propose an additional, optional, Extism.runtime.all which pulls in all supported runtime packages

Great idea

And as for the win binary, let's go with the MSVC one just because it's smaller (however as @alistairjevans mentioned, requires the VC++ redist runtime)

@nilslice
Copy link
Member

nilslice commented Dec 2, 2022

I would then be tempted to ship Extism.Host for the host library, to give you space for Extism.Pdk.

I don't have strong feelings against using the Host name here, but we typically have referred to these as "SDKs" in other places.Extism.Sdk and Extism.Pdk (when we have one :)) may align better in the bigger picture. If folks think Host is more clear though I'm on board!

Will extism build for Windows arm64?

Rust can target that platform, but I don't see a release for it for one of our dependencies wasmtime:

https://github.com/bytecodealliance/wasmtime/releases/tag/v3.0.1

I haven't tested to see if it wouldn't build, but I'd imagine wasmtime would release a build for it if it could target that platform too.

@mhmd-azeez
Copy link
Collaborator Author

mhmd-azeez commented Dec 2, 2022

I don't have strong feelings against using the Host name here, but we typically have referred to these as "SDKs" in other places.Extism.Sdk and Extism.Pdk (when we have one :)) may align better in the bigger picture. If folks think Host is more clear though I'm on board!

My vote is for Extism.Host

On second thought, maybe we should keep things consistent with the other platforms, @alistairjevans @bhelx what do you think?

@bhelx
Copy link
Contributor

bhelx commented Dec 2, 2022

Contains the win-x64 binary. @bhelx @nilslice Do you think we should include the MSVC binary or the gnu one? Or should we offer both choices?

Sorry catching up here. I understand the reason we may want to put the dll in the package for windows users, since they expect that. but for everything else we should probably follow the pattern we have with the rest of the libraries. Let the FFI library search out the dylib or so on the load path. Ideally we don't have to build for every potential architecture to and OS just to ship out the csharp library. It could get very big and we won't easily be able to future proof it.

@bhelx
Copy link
Contributor

bhelx commented Dec 2, 2022

On second thought, maybe we should keep things consistent with the other platforms, @alistairjevans @bhelx what do you think?

Yeah I think consistency is key unless there is a strong idiomatic (dotnet) reason to break it.

@bhelx
Copy link
Contributor

bhelx commented Dec 2, 2022

Rust can target that platform, but I don't see a release for it for one of our dependencies wasmtime:

man one other thing i need to do, make a table of architectures and OSs we support.

@alistairjevans
Copy link
Contributor

Happy with that argument @bhelx. Extism.Sdk it is then.

Re: distributing the binaries; on non-windows system, what's the deployment approach? Build from source on-device and put the built binary in the load path?

I guess to keep things simple and consistent we can start with that, but dotnet users are generally less familiar with build-from-source-libraries on any platform, so I can see some challenges on that front in the future.

@bhelx
Copy link
Contributor

bhelx commented Dec 2, 2022

Re: distributing the binaries; on non-windows system, what's the deployment approach? Build from source on-device and put the built binary in the load path?

For linux and mac, we have an installer that manages the dynamic library. it puts it on your load path: https://extism.org/docs/install#macos-1

The SDKs find this library at runtime. For windows I'd defer to your judgement. We probably want to have some code that just loads the packaged version for windows and finds the installed version for other platforms.

@mhmd-azeez
Copy link
Collaborator Author

mhmd-azeez commented Dec 2, 2022

Ideally we don't have to build for every potential architecture to and OS just to ship out the csharp library. It could get very big and we won't easily be able to future proof it.

I don't mind going with the approach of only releasing the windows runtime nuget package. But I just wanted to clarify the process a bit so that both @bhelx and @nilslice have a clearer understanding of the future maintenance work.

So .NET has its own search path in addition to the OS search path, it's something like this: runtimes\[runtime identifier]\native\ [native library file]. So the nuget runtime packages will have a runtime folder that matches the search path. For example:

The Extism.runtime.win package will have this structure:

runtimes/
├─ win-x64/
│  ├─ native/
│  │  ├─ extism.dll

The Extism.runtime.osx package will have this structure:

runtimes/
├─ osx-x64/
│  ├─ native/
│  │  ├─ extism.dylib
├─ osx-arm64/
│  ├─ native/
│  │  ├─ extism.dylib

The Extism.runtime.linux package will have this structure:

runtimes/
├─ linux-x64/
│  ├─ native/
│  │  ├─ extism.so

Now, we can either add all of these to a single nuget package, or separate them into different nuget pacakages. The important part is, as you might have noticed, these nuget packages contain nothing .NET specific, so they can be published whenever a new version of extism is released, i.e. in the Release or ci pipeline.

We will have two other nuget packages: Extism.Sdk and Extism.Pdk that contain the .NET specific code and the release cycles can be decoupled from the runtime packages.

Now, whichever path you decide is totally fine with me because I understand your concerns as the library maintainers.

@nilslice
Copy link
Member

nilslice commented Dec 3, 2022

Also I agree, probably best to fetch the DLL from latest release rather than build on the fly.

It will certainly be faster - and I think we always want to ship an SDK to target the latest release anyway. The only tricky thing I can think of is when we make a runtime release, we have to also re-release the .NET SDK so that it bundles the latest runtime. This might turn out to be true for most runtime releases anyways (especially for runtime feature additions/changes) but worth calling out and thinking through now.

Awesome work @mhmd-azeez!!

@mhmd-azeez
Copy link
Collaborator Author

@bhelx @nilslice thanks!

The only tricky thing I can think of is when we make a runtime release, we have to also re-release the .NET SDK so that it bundles the latest runtime

Just wanted to clarify one thing, the .NET SDK doesn't bundle the runtime. The Extism.Sdk nuget package doesn't actually depend on Extism.runtime.win-x64 at all and can be used separately. The Extism.runtime.win-x64 is just the runtime without any knowledge of the SDK. Whenever a project references Extism.runtime.win-x64 it just copies the win-x64 dll to the app's output directory.

So the release cycle of Extism.runtime.win-x64 is coupled with the release cycle of the native extism library. But that's not necessarily true for Extism.Sdk. I'll write some notes about both packages later on to make it clear when they should be published and what are the steps (mainly changing the version in the .csproj)

@alistairjevans
Copy link
Contributor

I'll take a look on Monday @mhmd-azeez; I'm away-from-keyboard until then I'm afraid.

… Context and Plugin classes provide comprehensive wrappers for the underlying library.

Also implement thread-safe dispose semantics and verify Context and Plugin have not been disposed when invoking methods.
Make interop calls internal, plus some other minor bits.
@nilslice
Copy link
Member

nilslice commented Dec 5, 2022

@mhmd-azeez regarding:

Test on more wasm plugins

I have some pre-compiled wasm modules in this repo, which I use as an example.

https://github.com/extism/extism-fsnotify/tree/main/apps

Both plugins use these datatypes as input/ouput in JSON:
https://github.com/extism/extism-fsnotify/blob/main/main.go#L17-L26

It may be useful to re-use them!

@bhelx
Copy link
Contributor

bhelx commented Dec 6, 2022

Just wanted to clarify one thing, the .NET SDK doesn't bundle the runtime. The Extism.Sdk nuget package doesn't actually depend on Extism.runtime.win-x64 at all and can be used separately. The Extism.runtime.win-x64 is just the runtime without any knowledge of the SDK. Whenever a project references Extism.runtime.win-x64 it just copies the win-x64 dll to the app's output directory.

@mhmd-azeez trying to understand what is needed from us to get this published. Are you saying this is how it works now? Or do you need us to work on the CI task to get the dll in the right place?

As for testing. I think it's in a good spot. If you want I can add some more unit tests tomorrow. As for testing different plugins, so much of that logic happens in the runtime itself so i think testing different plugins has diminishing returns. We just need to make sure we're mapping the types correctly back and forth. Unit tests can address this.

Let me know what you think. We're already got a few people stoked to try this!

@mhmd-azeez
Copy link
Collaborator Author

mhmd-azeez commented Dec 6, 2022

Hello @bhelx

@mhmd-azeez trying to understand what is needed from us to get this published. Are you saying this is how it works now? Or do you need us to work on the CI task to get the dll in the right place?

Unfortunately it doesn't work like that right now, so you'll need to work on the CI task to get the dll in the right place. If you can put the MSVC dll in dotnet/nuget/runtimes/win-x64.dll, everything should work as expected. If you can also publish a version for the runtime and SDK package it would be great so that I can reference them in the Sample project. If not, we can do this in another PR. After that I think this PR is ready to be merged.

Edit: I edited the PR description to cleanup the tasks

@mhmd-azeez
Copy link
Collaborator Author

After this PR is merged, I will try to work on adding a page on the extism docs about the .NET SDK

@bhelx
Copy link
Contributor

bhelx commented Dec 7, 2022

Hey @mhmd-azeez I have some notes on the interface: mhmd-azeez#3

In the rest of the libraries, we hide the implementation details of libextism. no need to expose length and error for example. I changed the interface of call to be bytes in and bytes out. The error code triggers an exception. This is more inline with how our other hosts do it. But if you think there is a more idiomatic way to do it let me know. Also feel free to merge and change it up or implement it yourself if you think there are better ways to accomplish this.

@bhelx
Copy link
Contributor

bhelx commented Dec 7, 2022

After this PR is merged, I will try to work on adding a page on the extism docs about the .NET SDK

our docs repo isn't public yet. I'm happy to do that for you. I can send a preview link here for you to approve.

@bhelx bhelx changed the title Implement a basic .NET host feat: Implement .NET Host SDK Dec 7, 2022
@bhelx
Copy link
Contributor

bhelx commented Dec 7, 2022

Heads Up: Github does not seem to want to allow us to run these release workflows until we merge. So we can merge this when the code is ready and follow up to fix the workflows.

@bhelx
Copy link
Contributor

bhelx commented Dec 7, 2022

The branch with the doc changes is here: https://dotnet-sdk-doc.extism-docs.pages.dev/docs/integrate-into-your-codebase/dotnet-host-sdk/

Will update this after we merge, publish, and test it. Once we verify the instructions are working we'll go live with it.

nilslice
nilslice previously approved these changes Dec 7, 2022
Copy link
Member

@nilslice nilslice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, great work on this!

@mhmd-azeez
Copy link
Collaborator Author

mhmd-azeez commented Dec 7, 2022

The branch with the doc changes is here: https://dotnet-sdk-doc.extism-docs.pages.dev/docs/integrate-into-your-codebase/dotnet-host-sdk/

Will update this after we merge, publish, and test it. Once we verify the instructions are working we'll go live with it.

@bhelx thank you very much. This makes me so happy :'D

A couple of notes:

  1. Change dotnet add package Extism.SDK --version 0.1.0 to dotnet add package Extism.Sdk --prerelease. The convention for package names is to be PascalCase and specifying --prelease allows people to install the package without us having to update the docs every time we push a new version.
  2. I think we have to document that they will need the Extism library in the bin directory of the app on Windows or maybe Improve windows support #141 makes this better? Or after merging this PR and publishing the win runtime nuget package we can just ask them to add it to the project
  3. Please update the code snippet to the one we have in the other branch so that it doesn't cause build errors

After mhmd-azeez#3 is merged we should merge this PR too I think

@bhelx
Copy link
Contributor

bhelx commented Dec 8, 2022

After mhmd-azeez#3 is merged we should merge this PR too I thinkAfter mhmd-azeez#3 is merged we should merge this PR too I think

Yes, i think it's time to merge it and let's do some smaller, more focused, follow up PRs. Great work getting it this far! Ping me when you want to merge mhmd-azeez#3 . Is there anything else we need to do for it?

I'll update the docs site with your suggestions too, thank you!

…interface-changes

feat: hide some of the implementation details of libextism
@bhelx
Copy link
Contributor

bhelx commented Dec 8, 2022

I just realized i can merge it. Let's get all this merged up now! Just gonna wait on CI one more time.

@bhelx bhelx merged commit c3ffb25 into extism:main Dec 8, 2022
@bhelx
Copy link
Contributor

bhelx commented Dec 8, 2022

Let's go!

@mhmd-azeez
Copy link
Collaborator Author

Wohooo! 🎉🎉🎉

@mhmd-azeez mhmd-azeez deleted the feature/dotnet-host branch March 16, 2023 19:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants