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

Move to .NET 8 WASI support #587

Merged
merged 16 commits into from
Nov 30, 2023
Merged

Move to .NET 8 WASI support #587

merged 16 commits into from
Nov 30, 2023

Conversation

RReverser
Copy link
Member

@RReverser RReverser commented Nov 21, 2023

Description of Changes

.NET 8 shipped last week with built-in WASI support. It's still marked as experimental because some things - like better import/export handling that we could use - are still being worked on, but it's already better than using the Wasi.Sdk package which is now soft-deprecated and has a number of bugs that nobody's going to fix anymore.

This change is pretty big because it tries to move from custom Mono C APIs to native C# marshalling as much as possible, because it's now supported, it's safer, and it's more future-proof. We should be able to eventually remove the bindings.c layer altogether once UnmanagedCallersOnly and DllImport are handled automatically and generate Wasm imports/exports by .NET's PInvoke, which is already available in .NET alpha channel. It also provides better foundation for NativeAOT support where Mono APIs don't exist and where otherwise we'd have to maintain two very different paths for AOT and non-AOT bindings.

The performance and instruction cost of the resulting bundle is more or less the same as before, because it's still using an interpreter bundled together with .NET bytecode, but thanks to various trimming options the Wasm itself is now ~3 times smaller.

API and ABI breaking changes

I don't think we have the right category for this, but after this change users will need to install .NET 8 to build new SpacetimeDB projects, but we'll automate the rest, e.g. installing the WASI SDK (Clang compiler & C stdlib) as well as Wasi workload for .NET itself.

I'll tentatively mark it as ABI change to draw attention to breakage even though ABI of the generated module is still compatible.

Expected complexity level and risk

3

This complexity rating applies not only to the complexity apparent in the diff,
but also to its interactions with existing and future code.

If you answered more than a 2, explain what is complex about the PR,
and what other components it interacts with in potentially concerning ways.

@RReverser RReverser added release-0.8 abi-break A PR that makes an ABI breaking change labels Nov 21, 2023
Copy link
Contributor

@kulakowski kulakowski left a comment

Choose a reason for hiding this comment

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

Small code comments but looks really great!

The only thing I want is some approval from I guess @jdetter or @cloutiertyler that the download strategy is ok from a product perspective.

crates/bindings-csharp/Runtime/bindings.c Show resolved Hide resolved
crates/bindings-csharp/Runtime/bindings.c Show resolved Hide resolved
crates/bindings-csharp/Runtime/Runtime.cs Show resolved Hide resolved
crates/bindings-csharp/Runtime/RawBindings.cs Outdated Show resolved Hide resolved
@RReverser
Copy link
Member Author

that the download strategy is ok from a product perspective.

FWIW this is the same download strategy the previously used Wasi.Sdk package uses. I'm open to better suggestions, just pointing out that functionally it's not something new.

@RReverser RReverser changed the title Move to .NET 8 Wasi support Move to .NET 8 WASI support Nov 28, 2023
Copy link
Collaborator

@jdetter jdetter left a comment

Choose a reason for hiding this comment

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

Tested, working with a few hiccups. Ingvar knows what the issues are and will fix them before merging. Approving so that this isn't blocked. We're trying to get this into the 0.8 release which we're cutting later today.

@kulakowski
Copy link
Contributor

@RReverser Let me know if there's anything I can do to help test or land this.

@RReverser
Copy link
Member Author

RReverser commented Nov 29, 2023

@jdetter wanted to do some more testing after my fixes - we ran into some issues, but they were mostly with unclear error messages e.g. due to .NET 8 not being installed. There was also an issue with dotnet workload install requiring sudo on macOS even if workload is already installed, so I had to wrap it into conditional check.

I fixed those so in principle nothing prevents this from getting merged now, but I'd prefer to wait for @jdetter to double-check that it's looking good.

@RReverser
Copy link
Member Author

RReverser commented Nov 29, 2023

The main problem is actually that I don't have macOS to test that sudo issue is gone now. @kulakowski if you want to test this, you can try to check out, do cargo run -- build -p .\modules\spacetimedb-quickstart-cs locally and see if error messages you get on each step make sense and whether you can eventually build this project (w/o sudo).

@kulakowski
Copy link
Contributor

cargo run -- build -p .\modules\spacetimedb-quickstart-cs

Trying now.

@RReverser
Copy link
Member Author

Trying now.

@kulakowski Any luck?

@RReverser
Copy link
Member Author

Ok I'm merging this as-is to make sure it gets into release. If there are more things to improve, we can do that separately.

@RReverser RReverser merged commit 4f74a5c into master Nov 30, 2023
5 checks passed
@kulakowski
Copy link
Contributor

It worked fine, just took a while to download a new xcode etc.

@RReverser RReverser deleted the csharp-wasi-net8 branch November 30, 2023 14:54
@RReverser
Copy link
Member Author

It worked fine, just took a while to download a new xcode etc.

Thanks. I'm confused though - why would it need Xcode? O_o

@kulakowski
Copy link
Contributor

Xcode has a couple levels, the lowest is essentially just a toolchain, and the new dotnet needed a newer version of the toolchain.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
abi-break A PR that makes an ABI breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants