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

c-api: Update reexport of wasmtime crate crate #7112

Merged
merged 2 commits into from
Oct 18, 2023

Conversation

alexcrichton
Copy link
Member

@alexcrichton alexcrichton commented Sep 29, 2023

This is a follow-up to #6765 to update this reexport since it was originally added to use both the C API and the wasmtime crate in the same downstream crate, but this should be possible through Cargo with:

[dependencies]
wasmtime = "13"
wasmtime-c-api = { version = "13", package = "wasmtime-c-api" }

and that way wasmtime::* is available as well as wasmtime_c_api::*. For convenience though the name wasmtime is still exported to allow depending on just this crate.

This is a follow-up to bytecodealliance#6765 to remove this reexport since it was
originally added to use both the C API and the `wasmtime` crate in the
same downstream crate, but this should be possible through Cargo with:

    [dependencies]
    wasmtime = "13"
    wasmtime-c-api = { version = "13", package = "wasmtime-c-api" }

and that way `wasmtime::*` is available as well as `wasmtime_c_api::*`
@alexcrichton
Copy link
Member Author

cc @RubixDev and @maxbrunsfeld on this. I'd want to confirm that your embedding use case still works if this is removed. Additionally this is mostly just pointing out that it's not necessary to have this directive to get things working. That being said I can imagine it's still convenient to have because you'd only need one Cargo manifest directive instead of two.

If y'all would prefer I think it would make sense to keep this but as pub use wasmtime instead of pub use wasmtime::* so you could access all of the internals but it wouldn't clash namespaces ideally. In any case I'd like to leave main as-is until y'all have weighed in over here.

@github-actions github-actions bot added the wasmtime:c-api Issues pertaining to the C API. label Sep 29, 2023
@github-actions
Copy link

Subscribe to Label Action

cc @peterhuene

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 .github/subscribe-to-label.json configuration file.

Learn more.

@maxbrunsfeld
Copy link
Contributor

I like the idea of pub use wasmtime, but either solution is ok with me. Our use case is pretty niche.

@alexcrichton alexcrichton changed the title c-api: Remove reexport of wasmtime crate c-api: Update reexport of wasmtime crate crate Sep 29, 2023
@alexcrichton alexcrichton marked this pull request as ready for review September 29, 2023 22:24
@alexcrichton alexcrichton requested a review from a team as a code owner September 29, 2023 22:24
@alexcrichton alexcrichton requested review from pchickey and removed request for a team September 29, 2023 22:24
@alexcrichton
Copy link
Member Author

Ok sounds good to me, I've updated to remove just the ::* aspect but this will still do pub use wasmtime; for downstream users to use.

@alexcrichton alexcrichton requested review from fitzgen and removed request for pchickey October 3, 2023 14:34
@alexcrichton
Copy link
Member Author

ping @fitzgen, mind taking a look at this?

@fitzgen fitzgen added this pull request to the merge queue Oct 18, 2023
Merged via the queue into bytecodealliance:main with commit f7004c1 Oct 18, 2023
18 checks passed
@alexcrichton alexcrichton deleted the no-reexport-in-c-api branch October 25, 2023 19:12
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.

3 participants