CI has been failing since April #315
Comments
|
I can reproduce the failing test consistently: Also the one from CI is failing consistently locally too: |
|
I seems the results look random in CI because some tests hang, and when others fail it crashes rest of the test suite. Here is a list of failing tests:
|
|
Both the passing and failing builds from above are running the same .net version: |
|
The project downloads the wasmtime c api: wasmtime-dotnet/src/Wasmtime.csproj Lines 151 to 156 in b2b0480 This changes regularly, and in ci the size of the download changed between the two runs. My current guess is that the Dev release is broken for this. |
|
Looks like Dev builds are being enabled even for pr runs: wasmtime-dotnet/.github/workflows/main.yml Lines 48 to 52 in b2b0480 Here is an example of a PR that should be using the matching c-library but is using DEV: https://github.com/bytecodealliance/wasmtime-dotnet/pull/313/checks |
|
Switching the main build to pull the matching version locally (19.0.1) made many more of the tests pass, so might be on to something. |
|
Hi, There have been some changes in the C API recently, but For example, the following commits changed C API functions that However, it seems the project is currently missing a maintainer, see: #313 (comment) |
This makes sense to have an early signal. Currently it looks like it is running on the PR's too which is blocking any updates for simple things like https://github.com/bytecodealliance/wasmtime-dotnet/pull/313/files.
bytecodealliance/wasmtime@e55fa3c Thanks for the pointers! Do you know enough to be able to update the project for these fixes?
I can help on this front but new to the project. If you know enough to update to the API changes, I could help review and get it in. Otherwise I'll start digging in. |
|
@jsturtevant I've fixed the first of those two things you linked in #316. |
|
I dug into this a bit more, it was bothering me that I couldn't get the So what looks to have happen is:
So we are in a strange state, the version in the code is 19.0.1 but we are closer to using the c-API of version 21.0 and the nightly dev version is way ahead. We have a few options:
|
|
There is a third option which is to bump to 1.20. The branch was cut on April 11th and got the backports from mid april, then was but right before april 24th! |
|
I took approach 3 in #317 and PR CI is passing! |
|
Hi @jsturtevant, thanks for your efforts in improving this!
AFAIK, previously, the workflow in this repo regarding updates from upstream wasmtime was:
Therefore, it was expected that the With #317, PRs against the For example, PR #245 updated the Therefore, I'm currently not so sure if we really should change the CI behavior to no longer use the Wasmtime You are right that if upstream What do you think? |
|
I was struggling with this too. I generally agree that is a good approach but comes with a few down sides, such as the one we are in now where we are far behind and it's pretty confusing what needs to be fixed. I think we can take a hybrid approach atleast temporary to get unstuck in an approach that let's us merge changes with ci being green with out a huge pr
I would propose we document the approach once we are back to "normal". How does that sound for an approach? |
This includes updates for: - bytecodealliance/wasmtime#8451 - bytecodealliance/wasmtime#8461 - bytecodealliance/wasmtime#8011 TODOs: - Allocating an `externref` can now fail (by `wasmtime_externref_new` returning `false`). Currently, we throw a `WasmtimeException` in that case. We need to check where that exception can be thrown, and whether we need to do any additional clean-up (e.g. when converting arguments for a function call). - Check whether it's ok to compare the `__private` field of externs (which has been remaned in the C API, previously it was `index`). - `anyref` type is not yet supported, but I'm not sure what exactly it is and whether we need to add it. Fixes bytecodealliance#315
|
Hi @jsturtevant, Yes, I think taking that hybrid approach might be a good idea. Regarding the C API changes, I have opened #318 to incorporate the changes in the Wasmtime C API (except for WASI, which is done in #316), so that with both PRs merged, CI should succeed again with the current However, #318 is still a draft as some details are still missing (e.g. clean-up in case Thanks! |
|
Awesome! Thanks for your help on this. Is there any value in having release 1.20 and 1.21, if not with your work on #318 we can skip ahead and cut a release for 1.22. |
|
Hi,
I don't know Wasmtime well enough to determine this, but it would probably be easier to skip these versions and proceed e.g. with a v22.0.0 release. |
|
Hi @jsturtevant, However, regarding the releases of wasmtime-dotnet Consider e.g. the WASM_API_EXTERN void wasmtime_val_delete(wasmtime_val_t *val);In Wasmtime v20.0.2, this has been changed to the following: WASM_API_EXTERN void wasmtime_val_delete(wasmtime_context_t *context,
wasmtime_val_t *val);Then, in Wasmtime v21.0.1, it has been changed to the following: WASM_API_EXTERN void wasmtime_val_unroot(wasmtime_context_t *context,
wasmtime_val_t *val);However, in wasmtime-dotnet tag Lines 397 to 398 in ed1e61f Notice that it has only one parameter ( (This is why I mentioned that it would probably be easier to skip versions 20.x and 21.x and proceed with a v22.0 release 😉) Similarly, for the released wasmtime-dotnet wasmtime-dotnet/src/WasiConfiguration.cs Lines 452 to 453 in 704b155 However, Wasmtime v21.0.1 already contains the changes to that function (bytecodealliance/wasmtime#8578): WASI_API_EXTERN bool wasi_config_set_argv(wasi_config_t *config, size_t argc,
const char *argv[]);What do you think about this? Maybe we should deprecate the releases, by unlisting/deprecate the NuGet packages for Once #316 is merged, I think we can then create a wasmtime-dotnet Thanks! |
I guess I incorrectly assumed if CI, was passing we would be mostly in good shape. Thanks for point this out to me.
Yes, lets get it into a good state and go from there |
|
side note: Any thoughts on what can be done to improve CI so these types of things are caught? |
Hi @jsturtevant, I'm not sure if there's an easy way to detect mismatching interop function signatures, as interop with native functions requires correctly declaring the function signature; otherwise, it depends on various factors (such as the CPU architecture, stack layout, parameter types etc.) what will happen. To detect such issues before creating a new Maybe there exists some build action that could detect whether two commits/refs of a specific repo ( Thanks! |
|
Thanks for the feedback and input. Will look into this a bit more since it would be nice to not have to manually do this to avoid mistakes. |
|
I asked a few folks and they suggested looking at I tried it out and it worked for the scenario where function name changed. The tests failed and I was able to catch the function name change to |
|
Doing a little more digging, and it seems we could use a tool like https://github.com/dotnet/clangsharp?tab=readme-ov-file#generating-bindings to point at header files and generate c#. This would require a bit of changes to the project but would help catch the changes to files. |
Opening this issue to start tracking down why it is failing.
The last good run was https://github.com/bytecodealliance/wasmtime-dotnet/actions/runs/8808954841 on commit b2b0480
Started failing the next day with https://github.com/bytecodealliance/wasmtime-dotnet/actions/runs/8825026578 on commit b2b0480
Some dependency that isn't pinned in CI might be a culprit.
I am able to repo locally:
The text was updated successfully, but these errors were encountered: