Stream: git-wasmtime

Topic: wasmtime / PR #4369 Add cmake compatibility to c-api

Wasmtime GitHub notifications bot (Jul 02 2022 at 23:30):

TheGreatRambler opened PR #4369 from cmake-c-api to main:

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [ ] This has been discussed in issue #..., or if not, please tell us why
  here.

• [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.

• [ ] This PR contains test cases, if meaningful.

• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the code of conduct.
-->
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.

Wasmtime GitHub notifications bot (Jul 02 2022 at 23:47):

TheGreatRambler updated PR #4369 from cmake-c-api to main.

Wasmtime GitHub notifications bot (Jul 03 2022 at 00:06):

TheGreatRambler updated PR #4369 from cmake-c-api to main.

Wasmtime GitHub notifications bot (Jul 03 2022 at 00:46):

TheGreatRambler updated PR #4369 from cmake-c-api to main.

Wasmtime GitHub notifications bot (Jul 03 2022 at 00:49):

TheGreatRambler updated PR #4369 from cmake-c-api to main.

Wasmtime GitHub notifications bot (Jul 03 2022 at 01:01):

TheGreatRambler updated PR #4369 from cmake-c-api to main.

Wasmtime GitHub notifications bot (Jul 03 2022 at 14:34):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Jul 03 2022 at 14:34):

bjorn3 created PR review comment:

This wrapping makes it harder to copy the full list.

Wasmtime GitHub notifications bot (Jul 03 2022 at 14:35):

bjorn3 created PR review comment:

Can you please revert this change? I am not sure doc tools will like it. In any case it makes it harder to read.

Wasmtime GitHub notifications bot (Jul 03 2022 at 14:35):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Jul 03 2022 at 14:36):

bjorn3 created PR review comment:

Can you please also revert this? It doesn't match the code style of the other header files.

Wasmtime GitHub notifications bot (Jul 03 2022 at 14:36):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Jul 03 2022 at 14:36):

bjorn3 created PR review comment:

nit: missing trailing newline

Wasmtime GitHub notifications bot (Jul 03 2022 at 14:36):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Jul 03 2022 at 14:39):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Jul 03 2022 at 14:39):

bjorn3 created PR review comment:

nit: I believe you have to use -pthread instead of -lpthread to be portable across UNIXes.

Wasmtime GitHub notifications bot (Jul 03 2022 at 14:39):

bjorn3 edited PR review comment.

Wasmtime GitHub notifications bot (Jul 04 2022 at 06:12):

TheGreatRambler submitted PR review.

Wasmtime GitHub notifications bot (Jul 04 2022 at 06:12):

TheGreatRambler created PR review comment:

Yeah issue with my formatter

Wasmtime GitHub notifications bot (Jul 04 2022 at 06:14):

TheGreatRambler submitted PR review.

Wasmtime GitHub notifications bot (Jul 04 2022 at 06:14):

TheGreatRambler created PR review 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

Wasmtime GitHub notifications bot (Jul 04 2022 at 06:17):

TheGreatRambler updated PR #4369 from cmake-c-api to main.

Wasmtime GitHub notifications bot (Jul 04 2022 at 07:17):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Jul 04 2022 at 23:12):

TheGreatRambler updated PR #4369 from cmake-c-api to main.

Wasmtime GitHub notifications bot (Jul 05 2022 at 17:52):

TheGreatRambler updated PR #4369 from cmake-c-api to main.

Wasmtime GitHub notifications bot (Jul 05 2022 at 23:50):

TheGreatRambler updated PR #4369 from cmake-c-api to main.

Wasmtime GitHub notifications bot (Jul 06 2022 at 01:57):

TheGreatRambler updated PR #4369 from cmake-c-api to main.

Wasmtime GitHub notifications bot (Jul 06 2022 at 02:00):

TheGreatRambler updated PR #4369 from cmake-c-api to main.

Wasmtime GitHub notifications bot (Jul 06 2022 at 03:57):

TheGreatRambler updated PR #4369 from cmake-c-api to main.

Wasmtime GitHub notifications bot (Jul 06 2022 at 05:47):

TheGreatRambler updated PR #4369 from cmake-c-api to main.

Wasmtime GitHub notifications bot (Jul 08 2022 at 16:58):

TheGreatRambler updated PR #4369 from cmake-c-api to main.

Wasmtime GitHub notifications bot (Jul 08 2022 at 17:43):

TheGreatRambler updated PR #4369 from cmake-c-api to main.

Wasmtime GitHub notifications bot (Jul 08 2022 at 18:09):

TheGreatRambler updated PR #4369 from cmake-c-api to main.

Wasmtime GitHub notifications bot (Jul 08 2022 at 18:37):

TheGreatRambler updated PR #4369 from cmake-c-api to main.

Wasmtime GitHub notifications bot (Jul 08 2022 at 18:47):

TheGreatRambler updated PR #4369 from cmake-c-api to main.

Wasmtime GitHub notifications bot (Jul 08 2022 at 19:09):

TheGreatRambler updated PR #4369 from cmake-c-api to main.

Wasmtime GitHub notifications bot (Jul 08 2022 at 19:16):

TheGreatRambler updated PR #4369 from cmake-c-api to main.

Wasmtime GitHub notifications bot (Jul 08 2022 at 19:21):

TheGreatRambler updated PR #4369 from cmake-c-api to main.

Wasmtime GitHub notifications bot (Jul 08 2022 at 19:37):

TheGreatRambler updated PR #4369 from cmake-c-api to main.

Wasmtime GitHub notifications bot (Jul 08 2022 at 21:02):

TheGreatRambler updated PR #4369 from cmake-c-api to main.

Wasmtime GitHub notifications bot (Jul 08 2022 at 21:09):

TheGreatRambler updated PR #4369 from cmake-c-api to main.

Wasmtime GitHub notifications bot (Jul 08 2022 at 21:14):

TheGreatRambler updated PR #4369 from cmake-c-api to main.

Wasmtime GitHub notifications bot (Jul 08 2022 at 21:22):

TheGreatRambler updated PR #4369 from cmake-c-api to main.

Wasmtime GitHub notifications bot (Jul 09 2022 at 02:53):

TheGreatRambler updated PR #4369 from cmake-c-api to main.

Wasmtime GitHub notifications bot (Jul 11 2022 at 01:03):

TheGreatRambler updated PR #4369 from cmake-c-api to main.

Wasmtime GitHub notifications bot (Jul 11 2022 at 01:12):

TheGreatRambler updated PR #4369 from cmake-c-api to main.

Wasmtime GitHub notifications bot (Jul 11 2022 at 01:30):

TheGreatRambler updated PR #4369 from cmake-c-api to main.

Wasmtime GitHub notifications bot (Jul 11 2022 at 03:46):

TheGreatRambler updated PR #4369 from cmake-c-api to main.

Wasmtime GitHub notifications bot (Jul 11 2022 at 04:59):

TheGreatRambler updated PR #4369 from cmake-c-api to main.

Wasmtime GitHub notifications bot (Jul 14 2022 at 18:53):

TheGreatRambler updated PR #4369 from cmake-c-api to main.

Wasmtime GitHub notifications bot (Jul 14 2022 at 19:59):

TheGreatRambler updated PR #4369 from cmake-c-api to main.

Wasmtime GitHub notifications bot (Jul 14 2022 at 20:32):

TheGreatRambler updated PR #4369 from cmake-c-api to main.

Wasmtime GitHub notifications bot (Jul 14 2022 at 20:41):

TheGreatRambler updated PR #4369 from cmake-c-api to main.

Wasmtime GitHub notifications bot (Jul 14 2022 at 21:21):

TheGreatRambler updated PR #4369 from cmake-c-api to main.

Wasmtime GitHub notifications bot (Jul 15 2022 at 06:43):

TheGreatRambler updated PR #4369 from cmake-c-api to main.

Wasmtime GitHub notifications bot (Jul 15 2022 at 07:02):

TheGreatRambler updated PR #4369 from cmake-c-api to main.

Wasmtime GitHub notifications bot (Jul 15 2022 at 07:06):

TheGreatRambler updated PR #4369 from cmake-c-api to main.

Wasmtime GitHub notifications bot (Jul 15 2022 at 07:07):

TheGreatRambler updated PR #4369 from cmake-c-api to main.

Wasmtime GitHub notifications bot (Jul 15 2022 at 07:09):

TheGreatRambler updated PR #4369 from cmake-c-api to main.

Wasmtime GitHub notifications bot (Jul 15 2022 at 07:17):

TheGreatRambler updated PR #4369 from cmake-c-api to main.

Wasmtime GitHub notifications bot (Jul 19 2022 at 08:18):

TheGreatRambler updated PR #4369 from cmake-c-api to main.

Wasmtime GitHub notifications bot (Jul 19 2022 at 17:23):

jameysharp created PR review comment:

Because you're removing the only uses of env!("TARGET"), please also remove run-examples/build.rs, which only exists to set that environment variable.

This is also the only use of the cc crate, so please remove that from run-examples/Cargo.toml and commit the updated top-level Cargo.lock.

Wasmtime GitHub notifications bot (Jul 19 2022 at 17:23):

jameysharp created PR review comment:

I'm not excited about having a blocklist here, but I don't think that should stop us from merging this PR. Still, if you wanted to do this better, I think you should be able to get the right list of examples from cargo and cmake.

For cargo, I believe you'd use cargo --read-manifest, parse the JSON output, then look under the "targets" key for anything with "kind": ["example"]. You can also get the required-features key for a target and pass those to cargo with --features, instead of hard-coding that the tokio example needs the "wasmtime-wasi/tokio" feature.

For cmake, it looks like you should be able to use the file API to do a "codemodel" query and look for targets with "type": "EXECUTABLE". (You might need to filter out anything with a isGeneratorProvided key?)

Wasmtime GitHub notifications bot (Jul 19 2022 at 17:23):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Jul 19 2022 at 17:23):

jameysharp created PR review comment:

I'd rather not have tokio in this list, but I see that the run-examples crate will try to build it because tokio/main.c exists. I think we should just delete tokio/main.c as well. Although most examples have both C and Rust implementations, epochs is Rust-only, and we should just acknowledge that not all examples make sense in C.

Wasmtime GitHub notifications bot (Jul 19 2022 at 17:23):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Jul 19 2022 at 17:23):

jameysharp created PR review comment:

Personally, I don't think it's worth changing -lpthread to -pthread until somebody reports that it doesn't work for them.

Wasmtime GitHub notifications bot (Jul 19 2022 at 17:23):

jameysharp created PR review comment:

This is missing the check for examples/{example}/main.c, which is what it should look for when is_dir is true.

Wasmtime GitHub notifications bot (Jul 20 2022 at 23:19):

TheGreatRambler updated PR #4369 from cmake-c-api to main.

Wasmtime GitHub notifications bot (Jul 20 2022 at 23:22):

TheGreatRambler submitted PR review.

Wasmtime GitHub notifications bot (Jul 20 2022 at 23:22):

TheGreatRambler created PR review comment:

I pushed a solution involving cargo read-manifest and cmake --build examples/build --target help, which can be parsed to get all targets. I still need to add --features handling but let me know how it is

Wasmtime GitHub notifications bot (Jul 20 2022 at 23:42):

jameysharp created PR review comment:

I like it! I had tried to find an option like cmake --target help but didn't search for the right things, I guess.

I see you removed the example_to_run command-line argument. I don't know whether anyone was using that but it seems pretty easy to keep supporting it so if you don't mind putting it back that'd be cool. It's weird that it runs two examples (the C and the Rust version) most of the time, but that seems harmless.

I think the big question now is, what will CI do with this PR this time?

Wasmtime GitHub notifications bot (Jul 20 2022 at 23:42):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Jul 20 2022 at 23:43):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Jul 21 2022 at 00:17):

TheGreatRambler updated PR #4369 from cmake-c-api to main.

Wasmtime GitHub notifications bot (Jul 21 2022 at 00:19):

TheGreatRambler submitted PR review.

Wasmtime GitHub notifications bot (Jul 21 2022 at 00:19):

TheGreatRambler created PR review comment:

Oh whoops, added it back it. It will still check if that is a valid example before it runs, however, like the old code. I'm also interested in seeing if the CI works for once.

Wasmtime GitHub notifications bot (Jul 21 2022 at 00:21):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Jul 21 2022 at 00:21):

jameysharp created PR review comment:

Yup, that looks right to me. And I'm interested too! :crossed_fingers:

Wasmtime GitHub notifications bot (Jul 21 2022 at 14:52):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jul 21 2022 at 14:52):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jul 21 2022 at 14:52):

alexcrichton created PR review 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.

Wasmtime GitHub notifications bot (Jul 21 2022 at 14:52):

alexcrichton created PR review comment:

Is this github action still needed? If so I don't think we should bring in some external action for this but instead work on sharing artifacts instead of producing new ones.

Wasmtime GitHub notifications bot (Jul 21 2022 at 16:46):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Jul 21 2022 at 16:46):

jameysharp created PR review 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.

Wasmtime GitHub notifications bot (Jul 21 2022 at 19:52):

TheGreatRambler created PR review comment:

Yeah I removed it, it was an attempt to reduce disk space usage, but I introduced a bug that had caused that disk usage and fixing it solved the problem

Wasmtime GitHub notifications bot (Jul 21 2022 at 19:52):

TheGreatRambler submitted PR review.

Wasmtime GitHub notifications bot (Jul 21 2022 at 20:03):

TheGreatRambler submitted PR review.

Wasmtime GitHub notifications bot (Jul 21 2022 at 20:03):

TheGreatRambler created PR review 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

Wasmtime GitHub notifications bot (Jul 21 2022 at 20:08):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Jul 21 2022 at 20:08):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Jul 21 2022 at 20:08):

jameysharp created PR review comment:

A minor style nit: Markdown in this project seems to be pretty consistently word-wrapped around 75-80 columns. Could you word-wrap this paragraph?

Wasmtime GitHub notifications bot (Jul 21 2022 at 20:16):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Jul 21 2022 at 20:16):

jameysharp created PR review 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.

Wasmtime GitHub notifications bot (Jul 21 2022 at 20:43):

TheGreatRambler updated PR #4369 from cmake-c-api to main.

Wasmtime GitHub notifications bot (Jul 21 2022 at 20:44):

TheGreatRambler submitted PR review.

Wasmtime GitHub notifications bot (Jul 21 2022 at 20:44):

TheGreatRambler created PR review comment:

I've just added integration with ctest

Wasmtime GitHub notifications bot (Jul 21 2022 at 20:45):

TheGreatRambler created PR review comment:

Oh wow that's really noticeable my bad lol

Wasmtime GitHub notifications bot (Jul 21 2022 at 20:45):

TheGreatRambler submitted PR review.

Wasmtime GitHub notifications bot (Jul 21 2022 at 20:48):

TheGreatRambler updated PR #4369 from cmake-c-api to main.

Wasmtime GitHub notifications bot (Jul 21 2022 at 21:09):

jameysharp created PR review comment:

Alright, that was simpler than I expected. :laughing: 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.

Wasmtime GitHub notifications bot (Jul 21 2022 at 21:09):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Jul 21 2022 at 21:10):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Jul 21 2022 at 23:35):

TheGreatRambler updated PR #4369 from cmake-c-api to main.

Wasmtime GitHub notifications bot (Jul 22 2022 at 00:15):

TheGreatRambler updated PR #4369 from cmake-c-api to main.

Wasmtime GitHub notifications bot (Jul 22 2022 at 00:47):

TheGreatRambler updated PR #4369 from cmake-c-api to main.

Wasmtime GitHub notifications bot (Jul 22 2022 at 01:05):

TheGreatRambler updated PR #4369 from cmake-c-api to main.

Wasmtime GitHub notifications bot (Jul 22 2022 at 01:53):

TheGreatRambler updated PR #4369 from cmake-c-api to main.

Wasmtime GitHub notifications bot (Jul 22 2022 at 07:01):

TheGreatRambler updated PR #4369 from cmake-c-api to main.

Wasmtime GitHub notifications bot (Jul 22 2022 at 17:22):

jameysharp merged PR #4369.

Last updated: Nov 22 2024 at 17:03 UTC