wasmtime / PR #9031 Use cmake to build wasmtime-c-api · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / PR #9031 Use cmake to build wasmtime-c-api

Wasmtime GitHub notifications bot (Jul 29 2024 at 04:34):

CGamesPlay opened PR #9031 from CGamesPlay:main to bytecodealliance:main:

This resolves #8890, and allows a rust project to use the wasmtime-c-api crate again.

In #8642, the build process was changed but the rust build script was not changed to match. In addition, because the CMake configuration invokes cargo, it isn't possible for cargo to then invoke CMake (cylic dependency). To resolve this, we introduce a new CMake target that only produces the necessary files for the build to succeed, and then allow cargo to build the API as normal.

I've tested that this works in my local copy of tree-sitter, which depends on the wasmtime-c-api-impl crate.

Wasmtime GitHub notifications bot (Jul 29 2024 at 04:34):

CGamesPlay requested fitzgen for a review on PR #9031.

Wasmtime GitHub notifications bot (Jul 29 2024 at 04:34):

CGamesPlay requested wasmtime-core-reviewers for a review on PR #9031.

Wasmtime GitHub notifications bot (Jul 29 2024 at 04:34):

CGamesPlay requested wasmtime-default-reviewers for a review on PR #9031.

Wasmtime GitHub notifications bot (Jul 29 2024 at 14:27):

alexcrichton submitted PR review:

Thanks! Do you know if it's possible to have an install target which only installs header files? That way there wouldn't need to be two includes and only one would be necessary.

Additionally can you forward along the -D flags for the features of Wasmtime? Otherwise the conf.h could report the wrong set of supported features.

Wasmtime GitHub notifications bot (Jul 30 2024 at 00:34):

CGamesPlay commented on PR #9031:

Sure, we just need to use COMPONENT Headers, see docs. I think only having one header would be ideal, especially if they are in different directories.

Are you suggesting that instead of just generating the header, I install all the headers to the cargo output directory, and then report that as the sole include directory? That makes sense, I just want to confirm.

Wasmtime GitHub notifications bot (Jul 30 2024 at 01:01):

CGamesPlay updated PR #9031.

Wasmtime GitHub notifications bot (Jul 30 2024 at 02:50):

CGamesPlay updated PR #9031.

Wasmtime GitHub notifications bot (Jul 30 2024 at 02:51):

CGamesPlay commented on PR #9031:

Updated commits properly configure the headers, and install all headers to the right place, so we can get rid of the second metadata variable.

Wasmtime GitHub notifications bot (Jul 30 2024 at 02:52):

CGamesPlay updated PR #9031.

Wasmtime GitHub notifications bot (Jul 30 2024 at 14:21):

alexcrichton submitted PR review:

Thanks!

Wasmtime GitHub notifications bot (Jul 30 2024 at 14:23):

alexcrichton updated PR #9031.

Wasmtime GitHub notifications bot (Jul 30 2024 at 14:23):

alexcrichton has enabled auto merge for PR #9031.

Wasmtime GitHub notifications bot (Jul 30 2024 at 16:10):

clason commented on PR #9031:

Can this be backported to release-23, too?

Wasmtime GitHub notifications bot (Jul 30 2024 at 16:49):

alexcrichton commented on PR #9031:

I think that would be reasonable yeah, just needs to land on main first and then the backport is a cherry-pick + new PR.

I believe CI is failing here on MinGW for cmake-related reasons? (compiler detection not working presumbably because cl.exe is used when mingw flags are passed)

Wasmtime GitHub notifications bot (Jul 30 2024 at 17:58):

maxbrunsfeld commented on PR #9031:

Is there any way we could avoid adding this cmake build dependency to wasmtime-c-api-impl?

Could the build.rs just generate conf.h, instead of using cmake to do it? It looks like that build.rs file already has to explicitly list out a bunch of cargo features and convert them to C macro names. Could it just write that to a conf.h file directly, instead of passing them as arguments to cmake?

Maybe I'm missing something, and the CMake build is doing something too significant to duplicate, but if not, it would be great to avoid the build-time dependency.

Wasmtime GitHub notifications bot (Jul 30 2024 at 19:37):

alexcrichton commented on PR #9031:

Is the cmake dependency too heavy for end users to arrange? I do think it's nice to have the same logic not duplicated. It's already broken once now so keeping it synchronized seems like the best bet.

Wasmtime GitHub notifications bot (Jul 30 2024 at 20:22):

maxbrunsfeld commented on PR #9031:

I just anticipate that consumers of the tree-sitter crate won't necessarily have cmake installed on their CI / dev machines, if they are mainly set up for Rust development.

Just to double check, I think that if we wanted to avoid requiring Cmake, we'd change this:

    let dir = PathBuf::from(env::var_os("CARGO_MANIFEST_DIR").unwrap());
    let mut config = cmake::Config::new(&dir);
    config
        .define("WASMTIME_DISABLE_ALL_FEATURES", "ON")
        .always_configure(true)
        .build_target("headers");
    for f in FEATURES {
        if env::var_os(format!("CARGO_FEATURE_{}", f)).is_some() {
            config.define(format!("WASMTIME_FEATURE_{}", f), "ON");
        }
    }
    let dst = config.build();

into this (or something similar).

    let prefix = "#ifndef WASMTIME_CONF_H\n#define WASMTIME_CONF_H\n";
    let suffix = "#endif\n";

    let mut header = prefix.to_string();
    for f in FEATURES {
        if std::env::var_os(format!("CARGO_FEATURE_{}", f)).is_some() {
            writeln!(&mut header, "WASMTIME_FEATURE_{}", f);
        }
    }
    header += suffix;

And you wouldn't need to add the headers target to the cmake file. It doesn't seem any more brittle to me. Either way, the build script needs to know the set of features, and how they map to C macro names.

Wasmtime GitHub notifications bot (Jul 30 2024 at 20:22):

maxbrunsfeld edited a comment on PR #9031:

I just anticipate that consumers of the tree-sitter crate won't necessarily have cmake installed on their CI / dev machines, if they are mainly set up for Rust development. It's not the end of the world, but it adds one more gotcha and potentially dependency issue for users of the crate.

Just to double check, I think that if we wanted to avoid requiring Cmake, we'd change this:
    let dir = PathBuf::from(env::var_os("CARGO_MANIFEST_DIR").unwrap());
    let mut config = cmake::Config::new(&dir);
    config
        .define("WASMTIME_DISABLE_ALL_FEATURES", "ON")
        .always_configure(true)
        .build_target("headers");
    for f in FEATURES {
        if env::var_os(format!("CARGO_FEATURE_{}", f)).is_some() {
            config.define(format!("WASMTIME_FEATURE_{}", f), "ON");
        }
    }
    let dst = config.build();
into this (or something similar).
    let prefix = "#ifndef WASMTIME_CONF_H\n#define WASMTIME_CONF_H\n";
    let suffix = "#endif\n";

    let mut header = prefix.to_string();
    for f in FEATURES {
        if std::env::var_os(format!("CARGO_FEATURE_{}", f)).is_some() {
            writeln!(&mut header, "WASMTIME_FEATURE_{}", f);
        }
    }
    header += suffix;
And you wouldn't need to add the headers target to the cmake file. It doesn't seem any more brittle to me. Either way, the build script needs to know the set of features, and how they map to C macro names.

Wasmtime GitHub notifications bot (Jul 30 2024 at 21:40):

alexcrichton commented on PR #9031:

The brittle part is that it's a duplication of copying over headers and there is no longer a single source of truth for how headers are created. Additionally there are no tests in this repository testing the header as-generated by the build script and without that there's also no tests that the duplicated logic for the header file generation is correct. What you're describing is correct as-is today but that may not hold true indefinitely into the future.

Wasmtime GitHub notifications bot (Jul 31 2024 at 00:18):

maxbrunsfeld commented on PR #9031:

Oh, I would have thought that if there are tests for wasmtime-c-api, then they would cover the correctness of the wasmtime-c-api-impl build script. Is there a tested code path that doesn't run this build script?

Wasmtime GitHub notifications bot (Jul 31 2024 at 00:22):

maxbrunsfeld edited a comment on PR #9031:

Oh, I would have thought that if there are tests for wasmtime-c-api, then they would cover the correctness of the wasmtime-c-api-impl build script. Is there a tested code path that doesn't run this build script?

Edit - Sorry never mind; I don't want to hold this up.

Wasmtime GitHub notifications bot (Jul 31 2024 at 00:23):

maxbrunsfeld edited a comment on PR #9031:

~Oh, I would have thought that if there are tests for wasmtime-c-api, then they would cover the correctness of the wasmtime-c-api-impl build script. Is there a tested code path that doesn't run this build script?~

Edit - Sorry never mind, it doesn't matter that much, and I don't want to hold this up.

Wasmtime GitHub notifications bot (Jul 31 2024 at 00:59):

CGamesPlay commented on PR #9031:

Is there are test that mingw compilation is working at all presently? This invocation of cmake looks correct, but it detects MSVC anyways:

    running: "cmake" "D:\\a\\wasmtime\\wasmtime\\crates\\c-api"
        "-G" "MSYS Makefiles"
        "-DWASMTIME_DISABLE_ALL_FEATURES=ON"
        "-DWASMTIME_FEATURE_DISABLE_LOGGING=ON"
        "-DCMAKE_SYSTEM_NAME=Windows"
        "-DCMAKE_SYSTEM_PROCESSOR=AMD64"
        "-DCMAKE_INSTALL_PREFIX=D:\\a\\wasmtime\\wasmtime\\target\\x86_64-pc-windows-gnu\\release\\build\\wasmtime-c-api-impl-dfbdb5120f123319\\out"
        "-DCMAKE_C_FLAGS= -ffunction-sections -fdata-sections -m64"
        "-DCMAKE_CXX_FLAGS= -ffunction-sections -fdata-sections -m64"
        "-DCMAKE_ASM_FLAGS= -ffunction-sections -fdata-sections -m64"
        "-DCMAKE_BUILD_TYPE=MinSizeRel"
    -- The C compiler identification is MSVC 19.40.33812.0

Wasmtime GitHub notifications bot (Jul 31 2024 at 01:08):

CGamesPlay edited a comment on PR #9031:

Is there are test that mingw compilation is working at all presently? This invocation of cmake looks correct, but it detects MSVC anyways:

    running: "cmake" "D:\\a\\wasmtime\\wasmtime\\crates\\c-api"
        "-G" "MSYS Makefiles"
        "-DWASMTIME_DISABLE_ALL_FEATURES=ON"
        "-DWASMTIME_FEATURE_DISABLE_LOGGING=ON"
        "-DCMAKE_SYSTEM_NAME=Windows"
        "-DCMAKE_SYSTEM_PROCESSOR=AMD64"
        "-DCMAKE_INSTALL_PREFIX=D:\\a\\wasmtime\\wasmtime\\target\\x86_64-pc-windows-gnu\\release\\build\\wasmtime-c-api-impl-dfbdb5120f123319\\out"
        "-DCMAKE_C_FLAGS= -ffunction-sections -fdata-sections -m64"
        "-DCMAKE_CXX_FLAGS= -ffunction-sections -fdata-sections -m64"
        "-DCMAKE_ASM_FLAGS= -ffunction-sections -fdata-sections -m64"
        "-DCMAKE_BUILD_TYPE=MinSizeRel"
    -- The C compiler identification is MSVC 19.40.33812.0

Looking at another mingw job I see that C:/msys64/usr/bin/pacman.exe -S --needed mingw-w64-x86_64-gcc --noconfirm and possibly a few others are likely needed?

Wasmtime GitHub notifications bot (Jul 31 2024 at 01:09):

CGamesPlay edited a comment on PR #9031:

Is there are test that mingw compilation is working at all presently? This invocation of cmake looks correct, but it detects MSVC anyways:

    running: "cmake" "D:\\a\\wasmtime\\wasmtime\\crates\\c-api"
        "-G" "MSYS Makefiles"
        "-DWASMTIME_DISABLE_ALL_FEATURES=ON"
        "-DWASMTIME_FEATURE_DISABLE_LOGGING=ON"
        "-DCMAKE_SYSTEM_NAME=Windows"
        "-DCMAKE_SYSTEM_PROCESSOR=AMD64"
        "-DCMAKE_INSTALL_PREFIX=D:\\a\\wasmtime\\wasmtime\\target\\x86_64-pc-windows-gnu\\release\\build\\wasmtime-c-api-impl-dfbdb5120f123319\\out"
        "-DCMAKE_C_FLAGS= -ffunction-sections -fdata-sections -m64"
        "-DCMAKE_CXX_FLAGS= -ffunction-sections -fdata-sections -m64"
        "-DCMAKE_ASM_FLAGS= -ffunction-sections -fdata-sections -m64"
        "-DCMAKE_BUILD_TYPE=MinSizeRel"
    -- The C compiler identification is MSVC 19.40.33812.0

Updated

Looking at another mingw job I see that C:/msys64/usr/bin/pacman.exe -S --needed mingw-w64-x86_64-gcc --noconfirm and possibly a few others are likely needed?

Wasmtime GitHub notifications bot (Jul 31 2024 at 01:45):

CGamesPlay updated PR #9031.

Wasmtime GitHub notifications bot (Jul 31 2024 at 01:45):

CGamesPlay commented on PR #9031:

I've added a workflow change to include the commands that I suspect fix the issue.

Wasmtime GitHub notifications bot (Jul 31 2024 at 01:46):

CGamesPlay edited a comment on PR #9031:

I've added a workflow change to include the commands that I suspect fix the issue. And I rebased.

Wasmtime GitHub notifications bot (Jul 31 2024 at 14:49):

alexcrichton updated PR #9031.

Wasmtime GitHub notifications bot (Jul 31 2024 at 15:05):

alexcrichton commented on PR #9031:

I've added an extra commit to this PR which runs the full tests suite on this PR instead of waiting for the merge queue, and it looks like there may still be some CI issues

Wasmtime GitHub notifications bot (Aug 01 2024 at 02:25):

CGamesPlay updated PR #9031.

Wasmtime GitHub notifications bot (Aug 01 2024 at 02:41):

CGamesPlay updated PR #9031.

Wasmtime GitHub notifications bot (Aug 01 2024 at 05:27):

CGamesPlay updated PR #9031.

Wasmtime GitHub notifications bot (Aug 01 2024 at 05:41):

CGamesPlay commented on PR #9031:

The one remaining CI failure is a network failure. Changes made to make CI happy:

Force CC and CXX in mingw. Turns out the other mingw build configurations don't build the C API anyways.

Use cargo package --no-verify on the C API since it is hard-coded to create an artifact directory in the source directory and it isn't possible to package that directory because cargo is hard-coded to exclude any directory with Cargo.toml in it.

By the way, the --no-verify on wasi-nn seems unnecessary (it worked fine on my machine).

Wasmtime GitHub notifications bot (Aug 01 2024 at 19:42):

alexcrichton submitted PR review:

Thanks! Given the possible fragility here with packaging and how you had to modify include, could verification via publish.rs be made to work? Basically could this be updated to undo the addition of --no-verify?

As for wasi-nn, if you'd like I think it's reasonable to try dropping that here too (and verify it), but it's ok to defer that to a different PR as well.

Wasmtime GitHub notifications bot (Aug 01 2024 at 19:42):

alexcrichton submitted PR review:

Thanks! Given the possible fragility here with packaging and how you had to modify include, could verification via publish.rs be made to work? Basically could this be updated to undo the addition of --no-verify?

As for wasi-nn, if you'd like I think it's reasonable to try dropping that here too (and verify it), but it's ok to defer that to a different PR as well.

Wasmtime GitHub notifications bot (Aug 01 2024 at 19:42):

alexcrichton created PR review comment:

Should the BINARY_DIR configuration be dropped entirely? That should probably be somewhere within CMAKE_CURRENT_BINARY_DIR if it's still being used to avoid modifying the source tree.

Wasmtime GitHub notifications bot (Aug 01 2024 at 19:43):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Aug 01 2024 at 19:43):

alexcrichton created PR review comment:

Mind leaving a comment as to why these steps are necessary? (this doesn't seem to directly follow from rust-lang/rust#112368 by my read

Wasmtime GitHub notifications bot (Aug 02 2024 at 00:18):

CGamesPlay updated PR #9031.

Wasmtime GitHub notifications bot (Aug 02 2024 at 00:36):

CGamesPlay updated PR #9031.

Wasmtime GitHub notifications bot (Aug 02 2024 at 01:17):

CGamesPlay updated PR #9031.

Wasmtime GitHub notifications bot (Aug 02 2024 at 01:44):

CGamesPlay commented on PR #9031:

Given the possible fragility here with packaging and how you had to modify include, could verification via publish.rs be made to work? Basically could this be updated to undo the addition of --no-verify?

Short answer: no. Longer answer: yes, but I probably can't do it in this PR. Any change I make here will at best be "works on my machine", and will likely involve knowing the details of all supported configurations and what went into https://github.com/bytecodealliance/wasmtime/pull/8642. Plus, I'm not a core developer of this project, I'm trying to get access to the 1-line method I added in https://github.com/bytecodealliance/wasmtime/pull/8907 so that I can update tree-sitter so that I can use wasm modules in my project. Basically, I'm already 2 projects deep in a rabbit hole :smile:

Hopefully you'll appreciate that I've taken the rust c-api crate from definitely-not-working to working-but-not-thoroughly-tested, and can add a follow-up task to the backlog to improve the testing.

Wasmtime GitHub notifications bot (Aug 02 2024 at 14:12):

alexcrichton updated PR #9031.

Wasmtime GitHub notifications bot (Aug 02 2024 at 14:21):

alexcrichton updated PR #9031.

Wasmtime GitHub notifications bot (Aug 02 2024 at 14:27):

alexcrichton commented on PR #9031:

That's fine yeah, I can try to help out here too.

It's worth nothing though the other side of the coin here. Most of us working on Wasmtime know very little about cmake and don't expect the C API to be used through Rust like this. #6765 has some more discussion of the addition of this feature about how things might break without tests in the repository. I tried to make it clear at least that this current situation was a risk of happening.

I understand you're short on time and don't want to maintain this here but at the same time as maintainers responsible for Wasmtime we need to make sure that introduction of features we don't fully understand don't hinder the development elsewhere. Here specifically our crate publication process is fully automated and only runs once a month so is at a very high risk of going wrong if we don't have protections in place. Explicitly passing --no-verify to new crates raises that risk even further. Failing publication due to errors with the c-api can then be difficult to understand without precise knowledge of everything working here.

Basically what I'm trying to say is that I'm not trying to get this PR to a picture-perfect state but rather get it to a point where it (a) passes tests and (b) doesn't increase maintenance burden. Dealing with CMake isn't always fun and easy and I understand it can be a slog, but that's part of the reality of having a cross-platform project sometimes.

Wasmtime GitHub notifications bot (Aug 02 2024 at 14:37):

alexcrichton updated PR #9031.

Wasmtime GitHub notifications bot (Aug 02 2024 at 15:55):

alexcrichton updated PR #9031.

Wasmtime GitHub notifications bot (Aug 02 2024 at 16:11):

alexcrichton updated PR #9031.

Wasmtime GitHub notifications bot (Aug 02 2024 at 16:19):

alexcrichton commented on PR #9031:

I'm losing steam on this and I don't know how best to proceed. Running a full cmake configure step has issues with needing extra CI configuration and new build dependencies (not just cmake but the build system like make/ninja as well). It additionally resurfaced android errors in https://github.com/bytecodealliance/wasmtime/actions/runs/10217416984/job/28271193012 which I don't know what's going on.

I tried an alternate approach of installing headers with a *.cmake script to use cmake -P instead. That skips the whole configure step and only requires a single cmake invocation and should be pretty portable and more lightweight. That is causing all sorts of issues of having to install headers in a few places as part of the build for things like docs and examples. I don't know how to best handle that.

Overall I don't know the best way forward on this. It might be to stop relying on cmake for features and use a rust-based script that can be shared between the normal cmake build and the build script. Unsure.

Wasmtime GitHub notifications bot (Aug 02 2024 at 16:24):

CGamesPlay commented on PR #9031:

I’m not at my computer right now but about the Android failure, one thing I noticed is that the NDK is v27 on -min and v25 on the regular one, with a different rust toolchain and a different runner image version. I set CMAKE_MAKE_PROGRAM to make and I got a successful run, but I am suspicious that its actually a nondeterministic failure.

Wasmtime GitHub notifications bot (Aug 08 2024 at 01:36):

CGamesPlay updated PR #9031.

Wasmtime GitHub notifications bot (Aug 08 2024 at 02:22):

CGamesPlay updated PR #9031.

Wasmtime GitHub notifications bot (Aug 08 2024 at 02:43):

CGamesPlay updated PR #9031.

Wasmtime GitHub notifications bot (Aug 08 2024 at 03:00):

CGamesPlay commented on PR #9031:

The remaining 2 build failures were pretty simple tweaks, so I went ahead and made them. The CI is passing now, and I confirmed that it still works when referenced from a rust crate. There was a network failure int he build, but I rebased to trigger a test rerun.

Do you think this is good to merge in its latest state?

Wasmtime GitHub notifications bot (Aug 09 2024 at 08:59):

clason submitted PR review.

Wasmtime GitHub notifications bot (Aug 09 2024 at 08:59):

clason created PR review comment:

# When adding or removing a feature, make sure to keep the C API in sync by

Wasmtime GitHub notifications bot (Aug 09 2024 at 09:00):

clason submitted PR review.

Wasmtime GitHub notifications bot (Aug 09 2024 at 09:00):

clason created PR review comment:

unrelated changes?

Wasmtime GitHub notifications bot (Aug 09 2024 at 09:02):

clason submitted PR review.

Wasmtime GitHub notifications bot (Aug 09 2024 at 09:02):

clason created PR review comment:

If you move this comment _above_ the list (here and elsewhere), then you can avoid the duplicate nonce (less noise when grepping).

Wasmtime GitHub notifications bot (Aug 09 2024 at 18:36):

alexcrichton submitted PR review:

Ah thanks for debugging that! This looks good to me yeah, so I'm going to go ahead and merge. I'm going to follow up with some responses to @clason as well and there can always be follow-ups too to handle things.

Wasmtime GitHub notifications bot (Aug 09 2024 at 18:37):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Aug 09 2024 at 18:37):

alexcrichton created PR review comment:

Personally I like having this at the end because features are often added at the end and a comment only at the top might get missed. The amount of duplication here though is unfortunate across the codebase but that would probably be best fixed with a "tidy" script or something like that rather than relying on us humans to handle everything.

Wasmtime GitHub notifications bot (Aug 09 2024 at 18:37):

alexcrichton created PR review comment:

I found this was necessary at one stage of testing to appease doxygen. That being said I barely understanding doxygen and we could very well be holding doxygen wrong where this should work when it doesn't.

Wasmtime GitHub notifications bot (Aug 09 2024 at 18:37):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Aug 09 2024 at 18:51):

alexcrichton merged PR #9031.

Wasmtime GitHub notifications bot (Aug 09 2024 at 19:17):

clason submitted PR review.

Wasmtime GitHub notifications bot (Aug 09 2024 at 19:17):

clason created PR review comment:

I just mean as a trivial way to halve the number of hits: no need to have the nonce both before _and_ after the list; once is enough (no matter where).

Wasmtime GitHub notifications bot (Aug 09 2024 at 19:17):

clason commented on PR #9031:

And also backport (24.0 is probably good enough) ;)

Wasmtime GitHub notifications bot (Aug 12 2024 at 01:32):

CGamesPlay commented on PR #9031:

I'd definitely love to get this in 24 if possible. Link to @clason's backport PRs:

#9101

#9102

Wasmtime GitHub notifications bot (Aug 12 2024 at 01:33):

CGamesPlay edited a comment on PR #9031:

I'd definitely love to get this in 24, and 23 would be nice if possible. Link to @clason's backport PRs:

#9101

#9102

Last updated: Oct 23 2024 at 20:03 UTC