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

Add a runtime test for multiple versions #787

Merged

Conversation

jsturtevant
Copy link
Collaborator

When working on #781 I noticed there wasn't a runtime test that had versions in the wit files. This adds test for a wit file that has multiple versions for rust and csharp.

@jsturtevant
Copy link
Collaborator Author

the C# failure the following, looks like we might be missing the versioning info on the import.

EXEC : error : WebAssembly function imports must be unique [D:\a\wit-bindgen\wit-bindgen\target\runtime-tests\versions\csharp-foo\Foo.csproj]
  System.Exception: WebAssembly function imports must be unique
     at ILCompiler.ConfigurableWasmImportPolicy.AddWasmImport(String) + 0x117
     at ILCompiler.ConfigurableWasmImportPolicy..ctor(IReadOnlyList`1, IReadOnlyList`1) + 0x2d4
     at ILCompiler.Program.Run() + 0x161d

@jsturtevant
Copy link
Collaborator Author

This is waiting on release with bytecodealliance/wasmtime#7656 for rust tests. The c# should work after #791

@alexcrichton
Copy link
Member

Oh if you'd like it's ok to use a git dependency on wasmtime to pull in fixes like that

@jsturtevant
Copy link
Collaborator Author

Oh if you'd like it's ok to use a git dependency on wasmtime to pull in fixes like that

sounds good, I will do that and drop the c# test until we get the other PR merged

@jsturtevant jsturtevant force-pushed the versions-runtime-test branch 2 times, most recently from d7f9f21 to 3d42dba Compare December 15, 2023 21:27
@jsturtevant jsturtevant marked this pull request as ready for review December 15, 2023 21:30
@jsturtevant
Copy link
Collaborator Author

@alexcrichton updated, I had to move a few dependencies, a couple type changes and regenerate the adapter module

@jsturtevant
Copy link
Collaborator Author

opened #802 to update the dependencies and will rebase on top of that

@jsturtevant jsturtevant marked this pull request as draft January 2, 2024 21:59
Signed-off-by: James Sturtevant <jsturtevant@gmail.com>
@jsturtevant jsturtevant force-pushed the versions-runtime-test branch 2 times, most recently from 3bac690 to ec48cd9 Compare January 3, 2024 01:07
@jsturtevant
Copy link
Collaborator Author

opened #802 to update the dependencies and will rebase on top of that

ends up that wasmtime 0.16.0 didn't have the required commit. I saw the date of the release which was after my commit went in but I guess it was from a branch a little further back in time.

wasmtime is only used as a dev dependency so I've moved it and made the required changes to the API.

@jsturtevant jsturtevant marked this pull request as ready for review January 3, 2024 01:10
@jsturtevant
Copy link
Collaborator Author

@alexcrichton up to you if you want to wait for a release for this or use the revision

Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

Looks good to me! Mind dropping the rev though in the dependency? I think the Cargo.lock entry should suffice for that

@jsturtevant
Copy link
Collaborator Author

Without the rev it will mean we will pull the tip of the main branch. I just tested it and this works but could cause issues down the line as the branch progresses?

@alexcrichton
Copy link
Member

WIth a Cargo.lock that won't happen unless a cargo update is issued which by the time that happens Wasmtime 17 will be released and it should be good to use anywya

Signed-off-by: James Sturtevant <jsturtevant@gmail.com>
@alexcrichton alexcrichton merged commit 8825a3b into bytecodealliance:main Jan 3, 2024
9 checks passed
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