search

bytecodealliance / wasmtime-dotnet

.NET embedding of Wasmtime https://bytecodealliance.github.io/wasmtime-dotnet/
Apache License 2.0
409 stars 52 forks source link

Fix Some C-API calls #316

Closed martindevans closed 2 months ago

martindevans commented 2 months ago

Fixed breakage due to changes introduced in https://github.com/bytecodealliance/wasmtime/commit/e55fa3cc928547a88738597cb6236d8571c42501

Obviously I can't run all the tests, due to the other changes mentioned here https://github.com/bytecodealliance/wasmtime-dotnet/issues/315#issuecomment-2182893613. Here's what I could run:

devenv_2024-06-21_17-05-33

The WasiTests cover the changed code.

jsturtevant commented 2 months ago

Thanks! I've opened #317 which should help some. Maybe we need both?

jsturtevant commented 2 months ago

Looking alittle closer this, seem to only be a change in the wasmtime 0.22.0 versions. We will also need https://github.com/bytecodealliance/wasmtime/commit/77405cc8fdfb12e5a6022a338ad371d7cf357687.

I am guessing we are missing other API changes between 0.19 and 0.22 which is why when pulling this into #317 didn't work

martindevans commented 2 months ago

Yeah this is only fixing the first of the two things you linked. We'll need another PR to fix https://github.com/bytecodealliance/wasmtime/commit/77405cc8fdfb12e5a6022a338ad371d7cf357687.

At the end I guess we'll need to merge all 3 PRs together to test it it finally fixes all of the tests.

jsturtevant commented 2 months ago

At the end I guess we'll need to merge all 3 PRs together to test it it finally fixes all of the tests.

Yes, I would like to group the changes to get CI passing in order to merge. Without that I am not confident enough in the changes. I won't be able to get to https://github.com/bytecodealliance/wasmtime/commit/77405cc8fdfb12e5a6022a338ad371d7cf357687 today if you are interested in picking that up too. Thanks for the help!

martindevans commented 2 months ago

Unfortunately I don't think I'll have time today or tomorrow. It looks like a pretty complex set of changes!

jsturtevant commented 2 months ago

@martindevans could you rebase? then we can get this in for 1.22 release

martindevans commented 2 months ago

@jsturtevant I'm on holiday at the moment. I'll be able to look at it on Sunday at the soonest.

If you want to take these changes and incorporate them yourself before then go ahead :)

martindevans commented 2 months ago

I just discovered GitHub itself has a button to automatically do the rebase, so I don't need access to my PC after all!

jsturtevant commented 2 months ago

@jsturtevant I'm on holiday at the moment. I'll be able to look at it on Sunday at the soonest.

no worries. Thanks and I hope you I didn't interrupt your personal time, that always takes precedent