Remove the explicit gc feature from the wasmtime dependency in the c-api crate#12805
Remove the explicit gc feature from the wasmtime dependency in the c-api crate#12805tolumide-ng wants to merge 2 commits intobytecodealliance:mainfrom
Conversation
2569780 to
ba53a43
Compare
alexcrichton
left a comment
There was a problem hiding this comment.
Returning now from vacation, thanks for your patience @tolumide-ng. Also I'll apologize that the scope of this issue is broadening to be a bit larger than I had originally anticipated. I thought there would be some minor edits required to get the c-api building without GC, but there's a number of API design decisions to make here which affect how exactly this works. I'm happy to help guide through these, but if you feel this is getting too broad in scope that's also totally fine.
There was a problem hiding this comment.
This is an example file where when gc is disabled the entire module may wish to disappear vs having lots of #[cfg] internally.
There was a problem hiding this comment.
I'm not exactly sure what you mean here. There are some functions where the gc feature does not apply, and some other modules depend on the macros. I'd be happy to retry with more clarification.
There was a problem hiding this comment.
Ah what I mean is placing #![cfg(feature = "gc")] at the top of this file to avoid compiling the entire file when the gc feature is disabled. Everything in here looks GC-related so should be fine to omit when gc is disabled. What are the errors you're seeing though about other modules depending on this? Perhaps those dependencies should also be #[cfg]'d?
There was a problem hiding this comment.
Thank you for the clarification. The errors mostly emanate from:
- crates/c-api/src/table.rs:2:82 where
wasm_ref_tis used - crates/c-api/src/val.rs:3:43 where
wasm_val_unionhas*mut wasm_ref_tin its fields.
There was a problem hiding this comment.
For table.rs that's ok to exclude those functions when GC is disabled, and for val.rs the union will have fewer fields when GC is disabled and that's ok, too.
There was a problem hiding this comment.
I'd update this right away, thank you
Welcome back from your holiday @alexcrichton, and thank you for the review. |
395f1f3 to
75ac484
Compare
There was a problem hiding this comment.
Ah what I mean is placing #![cfg(feature = "gc")] at the top of this file to avoid compiling the entire file when the gc feature is disabled. Everything in here looks GC-related so should be fine to omit when gc is disabled. What are the errors you're seeing though about other modules depending on this? Perhaps those dependencies should also be #[cfg]'d?
2864340 to
a226ebf
Compare
alexcrichton
left a comment
There was a problem hiding this comment.
It looks like some merge conflicts have cropped up as well, I'd recommend rebasing and seeing how to resolve those as well.
crates/c-api/include/wasmtime/val.h
Outdated
| void *__private3; | ||
| } wasmtime_anyref_t; | ||
|
|
||
| #ifdef WASMTIME_FEATURE_GC |
There was a problem hiding this comment.
Would it be possible to make the definition of wasmtime_{any,extern}ref_t conditional based on WASMTIME_FEATURE_GC?
There was a problem hiding this comment.
For table.rs that's ok to exclude those functions when GC is disabled, and for val.rs the union will have fewer fields when GC is disabled and that's ok, too.
ae29880 to
344c5b9
Compare
89143a5 to
44bff11
Compare
44bff11 to
c575d98
Compare
You're welcome. Thank you for the reviews and your patience. |
|
I updated the title and description of this PR to reflect the changes this PR introduces |
The
c-apicrate currently enables thegcfeature on itswasmtimedependency, even though it exposes agcfeature to control this behaviour. This prevents downstream users from disablinggc, sinceCargo.tomldoesn't allow overriding it.Changes
gcfeature for thewasmtimedependency inc-api, thus enabling downstream users to specify this behaviour.c-apicrate without default features, and with thegcfeatureNote
This introduces a breaking change for downstream users who may not have explicitly enabled the
gcfeature on thec-apicrate but rely on it.Closes #12783