Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#include <wasmtime/conf.h> fails since conf.h is not generated #8890

Closed
dundargoc opened this issue Jul 1, 2024 · 12 comments · Fixed by #9031
Closed

#include <wasmtime/conf.h> fails since conf.h is not generated #8890

dundargoc opened this issue Jul 1, 2024 · 12 comments · Fixed by #9031

Comments

@dundargoc
Copy link
Contributor

dundargoc commented Jul 1, 2024

When we try to bump to wasmtime v22.0.0 in tree-sitter/tree-sitter#3428 we get the following error:

cargo:warning=/home/runner/.cargo/registry/src/index.crates.io-6f17d22bba15001f/wasmtime-c-api-impl-22.0.0/include/wasi.h:12:10: fatal error: wasmtime/conf.h: No such file or directory
  cargo:warning=   12 | #include <wasmtime/conf.h>
  cargo:warning=      |          ^~~~~~~~~~~~~~~~~

When I checked this locally it seems that conf.h is not being generated when building the c-api via a crate as I could not find it in cargo:warning=/home/runner/.cargo/registry/src/index.crates.io-6f17d22bba15001f/wasmtime-c-api-impl-22.0.0/include/, which is where I assumed it would be as that's where the headers are looking for it.

@clason
Copy link

clason commented Jul 5, 2024

Related: Homebrew/homebrew-core#175234

@alexcrichton
Copy link
Member

This changed in #8642 where conf.h is only generated as part of the cmake-based build process at this time. If using via the Rust build script that'll need to have support as well for generating conf.h, probably in the build directory, and generating another cargo build directive to add another include path pointing to the generated file

@clason
Copy link

clason commented Jul 8, 2024

Just to be clear: These changes need to happen here in wasmtime-c-api, right?

@alexcrichton
Copy link
Member

I think there may be multiple issues in play here, so I'm not certain if a single fix in a single location will fix both issues here. One issue is that this line is no longer sufficient for the includes of Wasmtime because the conf.h file isn't generated anywhere as part of that build script. I think that's what tree-sitter is using and I think that's what tree-sitter is running into. Fixing that would involve updating the build script to do what CMake is doing which is to read conf.h.in and then postprocess it to generate a new conf.h. A new directive would then need to be printed to point to the location of this generated header file.

As for the Homebrew PR I don't know what's going on there. That looks like a possibly buggy installation of the C API because it looks like it was built from source and installed but conf.h is missing. That may indicate that CMake wasn't used to build/install but the PR looks like it was updated to use CMake. Given all that I don't know what's happening.

@clason
Copy link

clason commented Jul 8, 2024

One issue is that this line is no longer sufficient for the includes of Wasmtime because the conf.h file isn't generated anywhere as part of that build script. I think that's what tree-sitter is using and I think that's what tree-sitter is running into

Yes. The main question is whether that is considered a regression to be fixed, or whether the official policy is now "use the cmake crate to build the C API" (or hack it yourself in your own build.rs).

As for the Homebrew PR I don't know what's going on there.

Sorry, that was just meant to show that the effect is not limited to one consumer: Homebrew had the same issue of no longer being able to build wasmtime-c-api via (pure) cargo and had to switch to a CMake based build. (Which is less intrusive there since it's a standalone build that does not need to interface with a larger Rust project.)

@alexcrichton
Copy link
Member

If CMake can be used that'd be best yeah, primarily because that's what we're testing in CI and it's what's used to produce the artifacts. If that's not feasible this is minor enough I think it's ok to add custom logic to the build script too.

@osokin
Copy link

osokin commented Jul 17, 2024

It seems like cmake runs cargo to fetch all necessary crates during the build time (previously cargo did that by itself)...

@alexcrichton
Copy link
Member

Yes cmake runs cargo build which will fetch if necessary. Is that an issue for your integration? If so where should the network fetching happen?

@osokin
Copy link

osokin commented Jul 17, 2024

All necessary for a build process files need to be fetched before the build phase, where phases are: fetch -> patch -> configure -> build -> stage -> install.

Previously it was easy enough just because cargo did all dirty job. Now cargo is wrapped up with cmake, and as I said #8966 that adds complexity to the build process.

@alexcrichton
Copy link
Member

As a workaround for the time being could you continue to run cargo fetch during the fetch phase?

freebsd-git pushed a commit to freebsd/freebsd-ports that referenced this issue Jul 19, 2024
@CGamesPlay
Copy link
Contributor

Can I get some guidance on how to use this crate in its current state? As it stands, the wasmtime-c-api crate seems to be completely unusable.

It doesn't seem to be possible to use the cmake crate to call cmake from the build.rs script, because the inner cargo will deadlock trying to lock the package folder. I could try calling cmake from tree-sitter's build.rs, but this feels wrong because it would then require keeping tree-sitter's build script up to date with the build script for wasmtime.

Am I missing something obvious? Thanks for any pointers. Happy to do some tweaking of the build script and file a PR with the results once I get it working.

@alexcrichton
Copy link
Member

I think #9031 should work well, thanks for opening a PR!

I'll note though that one of the reasons this snuck in, which I think I was also mentioning on the original PR for this support, is that there are no tests in-tree so we can't guarantee that this will keep working. Despite that though assistance in maintaining it is much appreciated!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants