Skip to content

c-api: Compile a component #10566

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

Conversation

MangoPeachGrape
Copy link
Contributor

First little bit of #9812, "compile a component", with some changes suggested in the original pr:

Based on the efforts of @lanfeust69 and @rockwotj.

@MangoPeachGrape MangoPeachGrape requested a review from a team as a code owner April 11, 2025 14:05
@MangoPeachGrape MangoPeachGrape requested review from fitzgen and removed request for a team April 11, 2025 14:05
Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@alexcrichton
Copy link
Member

Also if you're up for it feel free to fill out this header to match the module.h header, e.g. with deserializing and such. Reflection on imports/exports though is going to be a "big deal" so deferring that is probably best.

@MangoPeachGrape MangoPeachGrape force-pushed the c-api/component-model/compile branch from 8b5f1ce to 4c318eb Compare April 11, 2025 16:18
@MangoPeachGrape
Copy link
Contributor Author

MangoPeachGrape commented Apr 11, 2025

Also if you're up for it feel free to fill out this header to match the module.h header, e.g. with deserializing and such. Reflection on imports/exports though is going to be a "big deal" so deferring that is probably best.

Added wasmtime_component_serialize() and wasmtime_component_deserialize(). Should I do wasmtime_module_deserialize_file() as well, or any other functions?

For the documentation in the header file I did s/module/component/, so it might not be accurate. In general, do you want documentation on everything in the header files in these early commits, or is that something that can be added in PRs down the line?

@alexcrichton
Copy link
Member

wasmtime_component_deserialize_file sounds good to add yeah, and maybe wasmtime_component_clone as well, but otherwise it looks like other functions aren't as applicable yet so that's probably good for now.

For docs I find it best to add it when the functions come in, but I'll admit during review I generally gloss over docs. I can give them a once-over to confirm everything is right after deserialize_file is added

@MangoPeachGrape MangoPeachGrape force-pushed the c-api/component-model/compile branch from 4c318eb to dd1dcfa Compare April 11, 2025 17:53
@MangoPeachGrape
Copy link
Contributor Author

I'm unsure if all the consts are correct, for example in wasmtime_component_clone(const wasmtime_component_t *component), because wasmtime_module_clone(wasmtime_module_t *m) doesn't have const?

Should the rust file also be split up, similar to the header files?

@github-actions github-actions bot added the wasmtime:c-api Issues pertaining to the C API. label Apr 11, 2025
@alexcrichton
Copy link
Member

Ah yeah the const there is ok, and it's one where we aren't necessarily the best-principled, but adding const when it maps to &T in Rust doesn't hurt for sure.

For file organization, yeah if you're up for it I think it would be a good idea. We'll be filling this out more in the future so I think it makes sense to separate everything out by file and starting here feels a bit odd as it's so small but it's predicted to get larger and be more justified in the future.

@MangoPeachGrape MangoPeachGrape force-pushed the c-api/component-model/compile branch from dd1dcfa to c9d7ecf Compare April 11, 2025 20:17
@MangoPeachGrape
Copy link
Contributor Author

For file organization, yeah if you're up for it I think it would be a good idea. We'll be filling this out more in the future so I think it makes sense to separate everything out by file and starting here feels a bit odd as it's so small but it's predicted to get larger and be more justified in the future.

Done!

What should the scope be for the next PR?

Co-authored-by: Tyler Rockwood <[email protected]>
Co-authored-by: Jean-Jacques Lafay <[email protected]>
@MangoPeachGrape MangoPeachGrape force-pushed the c-api/component-model/compile branch from c9d7ecf to 9e2a5ba Compare April 12, 2025 21:41
@alexcrichton alexcrichton added this pull request to the merge queue Apr 14, 2025
@alexcrichton
Copy link
Member

Personally I think one major missing piece of foundation for the C API is a good testing story. We don't have many tests for the C API and right now they're all classified as examples rather than unit tests. They're also awkward to write because C is pretty awkward to write.

To solve this one thing we've wanted to do was to fold the wasmtime-cpp repository (C++ bindings build on the C API) into this repository. That would make it much easier to write tests and then we could whip up some cmake as well to get tests.

I know writing tests isn't the most glamorous thing but it's a major thing I'd personally like to see before much of the rest of the C API becomes load bearing because I think the conversion of values is going to be significantly trickier than the wasm C API and warrant more thorough testing.

@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 14, 2025
@alexcrichton alexcrichton added this pull request to the merge queue Apr 14, 2025
@MangoPeachGrape
Copy link
Contributor Author

Yes, tests would be great!

All the tests would use GoogleTest, would you still seperate the tests for the C api and the C++ api, or would you have those combined?

Is this something you would want me to take a jab at? I'm not that familiar with the codebase, so I would need some guiding where to put everything.

@alexcrichton
Copy link
Member

GoogleTest would be fine yeah, and I think it'd be reasonable to test everything together. I'd expected we'd test C++ APIs by deafult (by moving wasmtime.hh to this repository) and then we could write one-off tests in C if necessary.

If you're willing to work on this that'd be great! Most of us in this repo aren't really all that well-versed in C/C++ repository management so if you know of practices/idioms around organization/testing/CI/etc that'd be very helpful. Most of the source would probably want to live in crates/c-api or some sub-folder, and I'm happy to help out with CI integration as well.

Merged via the queue into bytecodealliance:main with commit 6ba6e13 Apr 14, 2025
43 checks passed
@MangoPeachGrape MangoPeachGrape deleted the c-api/component-model/compile branch April 14, 2025 16:56
@MangoPeachGrape
Copy link
Contributor Author

@alexcrichton, Some questions that that came up while moving the C++ api into this repository:

  • Should the C++ examples go to examples/, and the tests go to crates/c-api/tests/?
  • Should we guard the C++ api to be only built if the consumer wants it (some consumers might not be able to build C++)?
  • Should C/C++ tests be built by default, or only be enabled by a CMake option when desired?

@alexcrichton
Copy link
Member

Yeah let's put examples in examples/{example}.{rs,c,cpp} and in theory we can fill out that matrix over time. For tests I think crates/c-api/tests is a good spot too.

The C++ API build step from an end consumable artifact point of view should just be a header file so I think it's fine to always enable/copy that around. For the tests I think it's also reasonable to require C++ to build tests since that's just for developers which should be easier in general. Given that I'd hope we can skip CMake options to conditionalize the C++ usage and basically just always use it

MangoPeachGrape added a commit to MangoPeachGrape/wasmtime that referenced this pull request Apr 14, 2025
`wasmtime-cpp/examples/*` -> `/examples/`
`wasmtime-cpp/tests/*` -> `/crates/c-api/tests/`

See discussion in bytecodealliance#10566 for more context.
MangoPeachGrape added a commit to MangoPeachGrape/wasmtime that referenced this pull request Apr 14, 2025
`wasmtime-cpp/include/wasmtime.hh` -> `/crates/c-api/include/`
`wasmtime-cpp/tests/*` -> `/crates/c-api/tests/`
`wasmtime-cpp/examples/*` -> `/examples/`

See discussion in bytecodealliance#10566 for more context.
MangoPeachGrape added a commit to MangoPeachGrape/wasmtime that referenced this pull request Apr 14, 2025
`wasmtime-cpp/include/wasmtime.hh` -> `/crates/c-api/include/`
`wasmtime-cpp/tests/*` -> `/crates/c-api/tests/`
`wasmtime-cpp/examples/*` -> `/examples/`

See discussion in bytecodealliance#10566 for more context.
github-merge-queue bot pushed a commit that referenced this pull request Apr 16, 2025
* c-api: Bring wasmtime-cpp into this repository.

`wasmtime-cpp/include/wasmtime.hh` -> `/crates/c-api/include/`
`wasmtime-cpp/tests/*` -> `/crates/c-api/tests/`
`wasmtime-cpp/examples/*` -> `/examples/`

See discussion in #10566 for more context.

* Make tests disabled by default prtest:full

* Mention C++ API in the c-api README

* Enable C++ only for tests

* Add *.hh files to doxygen

This requires disabling warnings on undocumented, as `wasm.hh` has
undocumeted structures

Also fix urls in `wasmtime.hh`

* Re-enable `WARN_IF_UNDOCUMENTED`

This required excluding `wasm.hh`, `wasmtime::detail` namespace, and some
macros.

Also documented two overloads to `Func::call()`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wasmtime:c-api Issues pertaining to the C API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants