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

Fix Some C-API calls #316

Merged

Conversation

martindevans
Copy link
Contributor

Fixed breakage due to changes introduced in bytecodealliance/wasmtime@e55fa3c

Obviously I can't run all the tests, due to the other changes mentioned here #315 (comment). Here's what I could run:

devenv_2024-06-21_17-05-33

The WasiTests cover the changed code.

@jsturtevant
Copy link
Contributor

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

@jsturtevant
Copy link
Contributor

Looking alittle closer this, seem to only be a change in the wasmtime 0.22.0 versions. We will also need bytecodealliance/wasmtime@77405cc.

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
Copy link
Contributor Author

Yeah this is only fixing the first of the two things you linked. We'll need another PR to fix bytecodealliance/wasmtime@77405cc.

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
Copy link
Contributor

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 bytecodealliance/wasmtime@77405cc today if you are interested in picking that up too. Thanks for the help!

@martindevans
Copy link
Contributor Author

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

Copy link
Contributor

@kpreisser kpreisser left a comment

Choose a reason for hiding this comment

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

Thanks for updating this!

It looks like the MarshalAs(UnmanagedType.I1) attribute is missing from the function delcaration, as without this, .NET would marshal the bool value as 4-byte BOOL, rather than as 1 byte value.

Otherwise, this looks good to me.

src/WasiConfiguration.cs Show resolved Hide resolved
src/WasiConfiguration.cs Show resolved Hide resolved
@jsturtevant
Copy link
Contributor

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

@martindevans
Copy link
Contributor Author

@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 and others added 3 commits June 29, 2024 13:00
Co-authored-by: Konstantin Preißer <kpreisser@users.noreply.github.com>
Co-authored-by: Konstantin Preißer <kpreisser@users.noreply.github.com>
@martindevans martindevans force-pushed the fix/c-api-changes-wasi-builder branch from c166963 to e0066c3 Compare June 29, 2024 12:00
@martindevans
Copy link
Contributor Author

martindevans commented Jun 29, 2024

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
Copy link
Contributor

@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

@jsturtevant jsturtevant merged commit 5e1b0e8 into bytecodealliance:main Jul 1, 2024
6 checks passed
@martindevans martindevans deleted the fix/c-api-changes-wasi-builder branch July 1, 2024 17:34
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.

3 participants