Use cmake to build wasmtime-c-api

[CGamesPlay]

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.

[alexcrichton]

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.

[CGamesPlay]

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.

[CGamesPlay]

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

[alexcrichton]

Thanks!

[clason]

Can this be backported to release-23, too?

[alexcrichton]

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)

[maxbrunsfeld]

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.

[alexcrichton]

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.

[maxbrunsfeld]

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.

[alexcrichton]

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.

[maxbrunsfeld]

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.

[CGamesPlay]

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?

[CGamesPlay]

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

[alexcrichton]

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

[CGamesPlay]

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).

[alexcrichton]

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.

[alexcrichton]

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.

[CGamesPlay]

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?

[clason]

Suggested change

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

[clason]

unrelated changes?

[alexcrichton]

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.

[clason]

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

[alexcrichton]

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.

[clason]

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).

[alexcrichton]

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.

[clason]

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

[CGamesPlay]

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

• [backport release-24.0.0] Use cmake to build wasmtime-c-api (#9031) #9101
• [backport release-23.0.0] Use cmake to build wasmtime-c-api (#9031) #9102