Add cmake compatibility to c-api #4369
Conversation
github-actions
bot
added
wasmtime:c-api
Subscribe to Label Actioncc @peterhuene
This issue or pull request has been labeled: "wasmtime:c-api", "wasmtime:docs"
Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
|
|
|
|
| -I crates/c-api/include \ | ||
| -I crates/c-api/wasm-c-api/include \ | ||
| target/release/libwasmtime.a \ | ||
| -lpthread -ldl -lm \ |
There was a problem hiding this comment.
Pre-existing: I believe you have to use -pthread instead of -lpthread to be portable across UNIXes.
There was a problem hiding this comment.
That was a copy and paste from serialize.c, which uses -lpthread, copied so cmake could be mentioned, I could change the rest to be consistent if you'd like
There was a problem hiding this comment.
Personally, I don't think it's worth changing -lpthread to -pthread until somebody reports that it doesn't work for them.
|
Thanks for this! This seems reasonable enough to me. Could this be used to replace and/or modify the existing |
|
I've moved compilation to the |
|
CI failed due to a lack of disk space. If it becomes a problem in the future I found this script from Apache that could help, it just removes unnecessary packages from the runner. |
|
I don't even know what to do to fix this CI error. I'm just going to give it a break for now |
|
Can anyone provide any tips for why the externref example works perfectly fine on my machine but fails in the CI? I clean the build folder to save disk space, but that should only impact the |
|
Still running out of storage? I... I don't even know, I'm going to revert the tests and test from there |
|
Could someone (if this is possible) manually restart the CI and/or give any tips on how to fix this final issue? It has to do with |
|
I don't know CMake on Windows really at all, but that looks like some form of a legitimate loop of some kind. |
There was a problem hiding this comment.
I like this a lot. I have a few fixes to request but I'd like to see this merged.
I spent a few minutes staring at the CI failure on Windows-2019 but not much jumped out at me. The one thing I noticed is that the timestamps are surprising: the output from the example, starting at "Initializing...", shows up all within the same second as the message "error: process didn't exit successfully". But that's hours after the last message from the linker. So I'm wondering if the output is getting buffered somewhere, and the buffer is flushed on ctrl-c.
I've triggered a re-run of the failing job with debug logging enabled, so maybe that will tell us something new.
| -I crates/c-api/include \ | ||
| -I crates/c-api/wasm-c-api/include \ | ||
| target/release/libwasmtime.a \ | ||
| -lpthread -ldl -lm \ |
There was a problem hiding this comment.
Personally, I don't think it's worth changing -lpthread to -pthread until somebody reports that it doesn't work for them.
There was a problem hiding this comment.
To confirm this isn't building more in CI now than we currently are? This PR's linux run is 10 minutes longer than main last linux run which could be variance but that seems pretty large.
crates/misc/run-examples/src/main.rs
Outdated
| @@ -1,36 +1,74 @@ | |||
| use anyhow::Context; | |||
There was a problem hiding this comment.
Is it possible to remove this crate entirely? It seems unfortunate to have both this runner script for CMake plus all the CMake support. I was hoping that this run-examples could be entirely deleted in favor of some CMake scripting. I don't know much about CMake myself though.
There was a problem hiding this comment.
Setting aside the C examples for a second, what would that look like for the Rust examples? Some of the examples have nested Rust subprojects that need to be built first to get a .wasm blob, and I couldn't find a way to make cargo run --example trigger that build first. I also don't see a way to ask cargo to run all examples, just a specific one by name.
My guess is that there's still value in this run-examples crate, so I'd like to merge this PR. If we figure out how to replace what it does with a tiny shell script or something, that'd be great as another PR.
There was a problem hiding this comment.
Cmake would be able to build, it would look similar to the below
include(ExternalProject)
ExternalProject_Add(
wasmtime-crate
DOWNLOAD_COMMAND ""
CONFIGURE_COMMAND ""
INSTALL_COMMAND "${WASMTIME_INSTALL_COMMAND}"
BUILD_COMMAND cargo build ${WASMTIME_BUILD_TYPE_FLAG}
BINARY_DIR ${CMAKE_CURRENT_SOURCE_DIR}
BUILD_ALWAYS ON)
add_library(wasmtime INTERFACE)
add_dependencies(wasmtime wasmtime-crate)
And actually it appears you can test with cmake, I've never done it but it can be done
There was a problem hiding this comment.
I had glanced at CTest while I was reviewing this PR earlier and at a very brief glance I didn't think it was meant for this kind of use: it seemed like it was more about running a test suite, with the usual facilities for reporting number of passing/failing tests and so on. I assumed that would be a poor fit, but I could be wrong.
I think it'd be unfortunate to require people to use CMake if they just want to try the Rust examples, so I'm skeptical about building the wasm blobs that way.
There was a problem hiding this comment.
I've just added integration with ctest
There was a problem hiding this comment.
Alright, that was simpler than I expected. 😆 Nice!
I see this version fails on Windows because -j4 isn't supported there. It's cool that you can run the examples in parallel but I'd also be happy without that optimization.
|
A little over 2 minutes of that difference was the That said, I would also like to know if removing the |
|
|
|
As for the Windows builds passing this time, it could be dumb luck but I actually reduced the warnings msvc reports to |
|
The new problem seems to be that the examples which rely on wasm blobs can't find them when run under CTest. The macos build is the one that failed but I think it just got to that point before any of the other job runners did, since I don't see any reason why it should be Mac-specific. Do you need to change how you set the current working directory for each test, perhaps? |
|
It has to do with the order in which I build the tests, since wasm modules are built for some c tests to run. Now the new problem is msbuild not taking the ctest arguments, and it sort of needs to accept them, without |
|
Of COURSE it got stuck again, this PR is taking so much of my time lol (I wish Github let me see the command line output so I can debug better) |
|
I've cancelled and restarted the windows-2019 job; 4+ hours in, it clearly wasn't going to work. I wish I had any good guesses about what's going on; I'd assumed there might be something weird in the run-examples crate, but that's gone now and we're still seeing this hang in CI. You have my sympathy for how painful this has been! I don't know much about cmake but I see there's a TIMEOUT property that you can set on a test. Maybe you can set a timeout on all the tests and at least get it to fail faster? I think 30 seconds ought to be more than enough time when things are working okay. Ooh, I see you have a new patch on the branch while I was typing this. I'll be curious to see if that fixes things. |
|
Hey, windows-2019 got past the point where it had been stuck! Although I see my suggestion of a 30-second timeout would have been too short for |
|
It was getting stuck because both |
|
Alex and I are dismayed that our CI didn't catch that bug sooner; apparently we should start using |
This PR adds CMake compatibility to the c-api, similar to that in wasm-tools. The examples are also modified to both demonstrate usage of CMake and are built by CI to verify the c-api is compiling properly.