-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
c-api: Compile a component #10566
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Also if you're up for it feel free to fill out this header to match the |
8b5f1ce
to
4c318eb
Compare
Added For the documentation in the header file I did |
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 |
4c318eb
to
dd1dcfa
Compare
I'm unsure if all the Should the rust file also be split up, similar to the header files? |
Ah yeah the const there is ok, and it's one where we aren't necessarily the best-principled, but adding 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. |
dd1dcfa
to
c9d7ecf
Compare
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]>
c9d7ecf
to
9e2a5ba
Compare
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. |
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. |
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 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 |
@alexcrichton, Some questions that that came up while moving the C++ api into this repository:
|
Yeah let's put examples in 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 |
`wasmtime-cpp/examples/*` -> `/examples/` `wasmtime-cpp/tests/*` -> `/crates/c-api/tests/` See discussion in bytecodealliance#10566 for more context.
`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.
`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.
* 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()`
First little bit of #9812, "compile a component", with some changes suggested in the original pr:
WASMTIME_FEATURE_COMPILER
.Based on the efforts of @lanfeust69 and @rockwotj.