github-actions[bot] commented on issue #6765:
Subscribe to Label Action
cc @peterhuene
<details>
This issue or pull request has been labeled: "wasmtime:c-api"Thus the following users have been cc'd because of the following labels:
- peterhuene: wasmtime:c-api
To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.
Learn more.
</details>
alexcrichton commented on issue #6765:
Thanks for the PR! I'm a bit hesitant personally about this, however, because if you're able to use
Cargo.toml
dependencies is there any reason you couldn't use thewasmtime
crate itself? The C API isn't designed to be used as a Rust crate dependency and probably exposes some functions as safe which are actuallyunsafe
for example (which doesn't matter on a C API bounary but matters to Rust).There's also other niche concerns where the Rust compiler has some odd gotchas about crates that are compiled as both a
cdylib
and anrlib
and I think it would be best to avoid that.
bjorn3 commented on issue #6765:
There's also other niche concerns where the Rust compiler has some odd gotchas about crates that are compiled as both a cdylib and an rlib and I think it would be best to avoid that.
That is fixed as of https://github.com/rust-lang/rust/pull/113695
RubixDev commented on issue #6765:
Thanks for the PR! I'm a bit hesitant personally about this, however, because if you're able to use
Cargo.toml
dependencies is there any reason you couldn't use thewasmtime
crate itself?Yes, I guess it didn't really become clear. Tree-sitter is written in C and provides Rust bindings. The C part uses wasmtime internally. The build script for the Rust bindings compiles the C code. For this, the wasmtime C library must be available because the tree-sitter C code uses it. The easiest way to have it available is to add it as a cargo dependency.
The C API isn't designed to be used as a Rust crate dependency and probably exposes some functions as safe which are actually
unsafe
for example (which doesn't matter on a C API bounary but matters to Rust).So the Rust crate does _not_ interface with wasmtime directly (that could be done with the normal
wasmtime
crate), but the C code it provides bindings to _does_. The safe/unsafe functions from thewasmtime-c-api
crate still only get called from C, not from Rust.I hope this time it's clearer
RubixDev edited a comment on issue #6765:
Thanks for the PR! I'm a bit hesitant personally about this, however, because if you're able to use
Cargo.toml
dependencies is there any reason you couldn't use thewasmtime
crate itself?Yes, I guess it didn't really become clear. Tree-sitter is written in C and provides Rust bindings. The C part uses wasmtime internally. The build script for the Rust bindings compiles the C code. For this, the wasmtime C library must be available because the tree-sitter C code uses it. The easiest way to have it available is to add it as a cargo dependency.
The C API isn't designed to be used as a Rust crate dependency and probably exposes some functions as safe which are actually
unsafe
for example (which doesn't matter on a C API bounary but matters to Rust).So the Rust crate _does not_ interface with wasmtime directly (that could be done with the normal
wasmtime
crate), but the C code it provides bindings to _does_. The safe/unsafe functions from thewasmtime-c-api
crate still only get called from C, not from Rust.I hope this time it's clearer
RubixDev commented on issue #6765:
Ok and to address the failing CI, this is because the C library is expected to just be called
wasmtime
(orlibwasmtime.a
), whereas with this change it is calledwasmtime_c_api
instead. That's obviously not desired, but necessary for tree-sitter to additionally depend on the normal Rustwasmtime
crate. I think the best alternative would be to revert the name change and re-export the Rustwasmtime
API from thewasmtime-c-api
crate. This way, tree-sitter only needs to depend onwasmtime-c-api
for both the normal Rust API and the C API.Before I push this though, I would like feedback on this idea.
RubixDev edited a comment on issue #6765:
Ok and to address the failing CI, this is because the C library is expected to just be called
wasmtime
(orlibwasmtime.a
), whereas with this change it is calledwasmtime_c_api
instead. That's obviously not desired, but necessary for tree-sitter to additionally depend on the normal Rustwasmtime
crate. I think the best alternative would be to revert the name change and re-export the Rustwasmtime
API from thewasmtime-c-api
crate. This way, tree-sitter only needs to depend onwasmtime-c-api
for both the normal Rust API and the C API.Before I push this though, I would like feedback on this idea.
<details>
<summary>The final diff would then look something like this</summary>diff --git a/crates/c-api/Cargo.toml b/crates/c-api/Cargo.toml index a464c0dbd..eb08a548a 100644 --- a/crates/c-api/Cargo.toml +++ b/crates/c-api/Cargo.toml @@ -11,7 +11,7 @@ publish = false [lib] name = "wasmtime" -crate-type = ["staticlib", "cdylib"] +crate-type = ["staticlib", "cdylib", "rlib"] doc = false test = false doctest = false diff --git a/crates/c-api/src/lib.rs b/crates/c-api/src/lib.rs index 4bc111b5d..3ef450731 100644 --- a/crates/c-api/src/lib.rs +++ b/crates/c-api/src/lib.rs @@ -11,6 +11,8 @@ #![allow(non_snake_case, non_camel_case_types, non_upper_case_globals)] +pub use wasmtime::*; + mod config; mod engine; mod error;
</details>
alexcrichton commented on issue #6765:
That is fixed as of https://github.com/rust-lang/rust/pull/113695
Oh nice! I was also thinking of issues though with LTO where rlib+cdylib has at least caused problems there where LTO settings effectively don't apply. That may have been fixed, though, I'm not sure.
Tree-sitter is written in C and provides Rust bindings
Hm ok it might be good to better understand your use case first then. This is a recipe for things going wrong where a Rust crate depends on a C library which depends on a Rust library. There's two versions of the rust standard library in that final program and that may or may not be expected (and may also cause things like linking issues).
I'm also still not sure how this PR is expected to be integrated successfully? Why is reexporting
wasmtime::*
necessary? Are you using both Wasmtime's C API and the native Rust API? Or are the Rust bindings for tree-sitter exclusively using tree-sitter's own C API?
At least for me I've never found a way to strike a right balance providing a C API from a Rust crate. I've gotten requests like this historically to support the C API both when used from Rust and C and I don't know how best to organize and enable that. In the end though if this works for you (it'd be good to end-to-end test to confirm) then my concerns are pretty niche and we can go ahead and merge it.
bjorn3 commented on issue #6765:
Oh nice! I was also thinking of issues though with LTO where rlib+cdylib has at least caused problems there where LTO settings effectively don't apply. That may have been fixed, though, I'm not sure.
Seems not.
echo | rustc - --crate-type rlib --crate-type cdylib -Clto=thin
giveserror: lto can only be run for executables, cdylibs and static library outputs
RubixDev commented on issue #6765:
Hm ok it might be good to better understand your use case first then. This is a recipe for things going wrong where a Rust crate depends on a C library which depends on a Rust library. There's two versions of the rust standard library in that final program and that may or may not be expected (and may also cause things like linking issues).
Well, the proposed solution only has one Rust and one C part which interoperate. The tree-sitter Rust bindings call functions from the C part, and the C part calls the wasmtime functions from the Rust part. Note that the wasmtime functions are in the same Rust binary as the tree-sitter bindings (because the wasmtime C API is used as a dependency) so there will only be one Rust stdlib and no linking issues. The use of the C API is required as tree-sitter should also be usable from C, just that there users will have to manually provide wasmtime.
I'm also still not sure how this PR is expected to be integrated successfully? Why is reexporting
wasmtime::*
necessary? Are you using both Wasmtime's C API and the native Rust API? Or are the Rust bindings for tree-sitter exclusively using tree-sitter's own C API?The Rust bindings don't interface with the wasmtime C API directly, but the C code called from the Rust bindings uses the wasmtime C API. Additionally, the Rust bindings use a small part of the native Rust API, actually just
Engine
. Due to the lib name clash betweenwasmtime
andwasmtime-c-api
, a re-export is required for the usage of both crates simultaneously.In the end though if this works for you (it'd be good to end-to-end test to confirm) then my concerns are pretty niche and we can go ahead and merge it.
I'm not sure what exactly you mean by "end-to-end test", but I've successfully used the fork locally.
alexcrichton commented on issue #6765:
Can you explain more about why
pub use
is required? That's venturing more into the territory of this seems to situation-solve a specific use case which isn't documented and won't be guarnateed to continue working. For example that's a very easy thing to overlook and accidentally remove during a refactoring or something like that.Again though I don't know if there are any widely used idioms about defining C APIs in Rust and using them from Rust as well. This is not a use case I've ever seen before work successfully, so I don't know how best to organize this.
Note that the wasmtime functions are in the same Rust binary as the tree-sitter bindings (because the wasmtime C API is used as a dependency)
I'll note as well that what you're describing here is a circular dependency between libraries which is not guaranteed to work with all native linkers. To get circular dependencies to always work native linkers have
--start-group
and--end-group
options (or similar) which are required to indicate that there are circular references within that set of arguments passed. The Rust compiler would not pass these for arbitrary libraries and there's no means of doing so in general. While some platforms may work (e.g. the ones you've tested) it's unlikely to work on all platforms or into the future (e.g. in the face of future refactorings)
Sorry if I may be repeating myself, I've been doing a bit of travel and lost context on this...
maxbrunsfeld commented on issue #6765:
:wave: I wrote the Tree-sitter PR that gave rise to this change. It'd be great to land this functionality, so that Tree-sitter can provide the WASM plugin functionality that's implemented in https://github.com/tree-sitter/tree-sitter/pull/1864, without having to depend on a fork of wasmtime.
I'll note as well that what you're describing here is a circular dependency between libraries which is not guaranteed to work with all native linkers
I don't think there's a dependency cycle here. The dependencies between the different compilation units are as follows:
(1. Tree-sitter Rust crate) | +---> (2. Tree-sitter C library) | | +------+----> (3. Wasmtime C API) | | +------+----> (4. Wasmtime Rust Crate)
All of the edges in this graph are expressed directly to Cargo, except for the dependency between 2 and 3 (because 2 is a C library compiled in a build script). When compiling with Cargo, the linking works fine though, I imagine because 1 and 2 are part of the same crate, and 1 has a Cargo dependency on 3.
It seems to work without issue on all of the platforms I've tried it on, and it's useful.
maxbrunsfeld edited a comment on issue #6765:
:wave: I wrote the Tree-sitter PR that gave rise to this change. It'd be great to land this functionality, so that Tree-sitter can provide the WASM plugin functionality that's implemented in https://github.com/tree-sitter/tree-sitter/pull/1864, without having to depend on a fork of wasmtime.
I'll note as well that what you're describing here is a circular dependency between libraries which is not guaranteed to work with all native linkers
I don't think there's a dependency cycle here. The dependencies between the different compilation units are as follows:
(1. Tree-sitter Rust crate) | +---> (2. Tree-sitter C library) | | +------+------> (3. Wasmtime C API) | | +----------------+------> (4. Wasmtime Rust Crate)
All of the edges in this graph are expressed directly to Cargo, except for the dependency between 2 and 3 (because 2 is a C library compiled in a build script). When compiling with Cargo, the linking works fine though, I imagine because 1 and 2 are part of the same crate, and 1 has a Cargo dependency on 3.
It seems to work without issue on all of the platforms I've tried it on, and it's useful.
maxbrunsfeld commented on issue #6765:
Can you explain more about why
pub use
is required?The
pub use
isn't strictly required, but we do need some way to access the items in thewasmtime
crate, while depending on thewasmtime-c-api
crate. Right now, we're prevented from importing anything fromwasmtime
while depending onwasmtime-c-api
, becausewasmtime-c-api
does this in its Cargo.toml:https://github.com/bytecodealliance/wasmtime/blob/main/crates/c-api/Cargo.toml#L12-L13
[lib] name = "wasmtime"
alexcrichton commented on issue #6765:
Ok thanks for coming back to this, and given how useful it is for y'all I think this is ok to land. If issues come up we can try to work with you to figure out how to fix it (if any), and otherwise can take care of things in the meantime.
Testing locally I don't think the
pub use
is required with a bit of a Cargo workaround:[dependencies] wasmtime = "*" this-is-your-name = { version = "*", package = 'wasmtime-c-api' }
The
package
key tricks cargo into passing--extern this_is_your_name=...
. Note thatthis-is-your-name
could even bewasmtime-c-api
which would allow Rust code to usewasmtime_c_api::...
.I'm going to go ahead and queue this up for merge and I'll send a follow-up to remove the
pub use
under the assumption that it's best to go ahead and land this somewhat-old PR rather than ask for further changes.Thank you again for following up, it's appreciated!
maxbrunsfeld commented on issue #6765:
Thanks so much @alexcrichton, and I really appreciate that solution of using a custom dependency name. I didn't know you could do that!
Last updated: Dec 23 2024 at 13:07 UTC