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.
CGamesPlay requested fitzgen for a review on PR #9031.
CGamesPlay requested wasmtime-core-reviewers for a review on PR #9031.
CGamesPlay requested wasmtime-default-reviewers for a review on PR #9031.
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 theconf.h
could report the wrong set of supported features.
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.
CGamesPlay updated PR #9031.
CGamesPlay updated PR #9031.
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.
CGamesPlay updated PR #9031.
alexcrichton submitted PR review:
Thanks!
alexcrichton updated PR #9031.
alexcrichton has enabled auto merge for PR #9031.
Can this be backported to release-23, too?
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)
maxbrunsfeld commented on PR #9031:
Is there any way we could avoid adding this
cmake
build dependency towasmtime-c-api-impl
?Could the
build.rs
just generateconf.h
, instead of using cmake to do it? It looks like thatbuild.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 aconf.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 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.
maxbrunsfeld commented on PR #9031:
I just anticipate that consumers of the
tree-sitter
crate won't necessarily havecmake
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.
maxbrunsfeld edited a comment on PR #9031:
I just anticipate that consumers of the
tree-sitter
crate won't necessarily havecmake
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 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.
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 thewasmtime-c-api-impl
build script. Is there a tested code path that doesn't run this build script?
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 thewasmtime-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.
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 thewasmtime-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 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
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?
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?
CGamesPlay updated PR #9031.
CGamesPlay commented on PR #9031:
I've added a workflow change to include the commands that I suspect fix the issue.
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.
alexcrichton updated PR #9031.
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
CGamesPlay updated PR #9031.
CGamesPlay updated PR #9031.
CGamesPlay updated PR #9031.
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 anartifact
directory in the source directory and it isn't possible to package that directory because cargo is hard-coded to exclude any directory withCargo.toml
in it.
- By the way, the
--no-verify
onwasi-nn
seems unnecessary (it worked fine on my machine).
alexcrichton submitted PR review:
Thanks! Given the possible fragility here with packaging and how you had to modify
include
, could verification viapublish.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 submitted PR review:
Thanks! Given the possible fragility here with packaging and how you had to modify
include
, could verification viapublish.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 created PR review comment:
Should the
BINARY_DIR
configuration be dropped entirely? That should probably be somewhere withinCMAKE_CURRENT_BINARY_DIR
if it's still being used to avoid modifying the source tree.
alexcrichton submitted PR review.
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
CGamesPlay updated PR #9031.
CGamesPlay updated PR #9031.
CGamesPlay updated PR #9031.
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.
alexcrichton updated PR #9031.
alexcrichton updated PR #9031.
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.
alexcrichton updated PR #9031.
alexcrichton updated PR #9031.
alexcrichton updated PR #9031.
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 justcmake
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 usecmake -P
instead. That skips the whole configure step and only requires a singlecmake
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.
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
tomake
and I got a successful run, but I am suspicious that its actually a nondeterministic failure.
CGamesPlay updated PR #9031.
CGamesPlay updated PR #9031.
CGamesPlay updated PR #9031.
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?
clason submitted PR review.
clason created PR review comment:
# When adding or removing a feature, make sure to keep the C API in sync by
clason submitted PR review.
clason created PR review comment:
unrelated changes?
clason submitted PR review.
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
grep
ping).
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.
alexcrichton submitted PR review.
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.
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.
alexcrichton submitted PR review.
alexcrichton merged PR #9031.
clason submitted PR review.
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).
And also backport (24.0 is probably good enough) ;)
CGamesPlay commented on PR #9031:
I'd definitely love to get this in 24 if possible. Link to @clason's backport PRs:
- #9101
- #9102
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: Dec 23 2024 at 12:05 UTC