Skip to content

Add wasmtime-c-api-impl to the list of crates to publish#7837

Merged
alexcrichton merged 7 commits intobytecodealliance:mainfrom
maxbrunsfeld:publish-c-api
Feb 5, 2024
Merged

Add wasmtime-c-api-impl to the list of crates to publish#7837
alexcrichton merged 7 commits intobytecodealliance:mainfrom
maxbrunsfeld:publish-c-api

Conversation

@maxbrunsfeld
Copy link
Copy Markdown
Contributor

@maxbrunsfeld maxbrunsfeld commented Jan 29, 2024 •
edited
Loading

Prior discussion: #7735

Having the C API available on crates.io is useful for crates that use C code that use Wasmtime via the C API, allowing those crates themselves to be published to crates.io.

I'm not sure if there are drawbacks to adding this; I just thought I'd open this PR as a concrete suggestion, in case it's uncontroversial.

Currently, the package name is wasmtime-c-api-impl (since wasmtime-c-api is used by the cdylib). That seems fine to me, but I'm also open to some name change.

@maxbrunsfeld maxbrunsfeld requested a review from a team as a code owner January 29, 2024 01:30
@maxbrunsfeld maxbrunsfeld requested review from alexcrichton and removed request for a team January 29, 2024 01:30
@amaanq
Copy link
Copy Markdown

amaanq commented Jan 29, 2024

Wouldn't this line here need to be removed? I could be totally wrong

wasmtime/crates/c-api/Cargo.toml

Line 10 in 94b3e84

publish = false

@alexcrichton
Copy link
Copy Markdown
Member

alexcrichton commented Jan 29, 2024

Thanks! I don't have a great suggestion for how to fix the namig problem, and it's probably not worth holding this up for fixing that.

In addition to @amaanq's suggestion would you be up for helping spruce up the documentation a bit? For example I think it'd be good to have some basic crate documentation explaining what things are (as nothing has rustdoc internally otherwise) and ideally we'd also have a README.md explaining a bit about how to use it (as that'll get rendered on crates.io). I also think it'd be good to have a written log about how the crate is used so if we change things in the future we know what to keep working.

@maxbrunsfeld maxbrunsfeld requested a review from a team as a code owner January 30, 2024 00:40
@maxbrunsfeld maxbrunsfeld force-pushed the publish-c-api branch 3 times, most recently from 203eef4 to e69274e Compare January 30, 2024 01:55
@maxbrunsfeld
Copy link
Copy Markdown
Contributor Author

maxbrunsfeld commented Jan 30, 2024 •
edited
Loading

Ok, I updated the Cargo metadata, enabling publishing and documentation. In order to make it convenient for a downstream crate to include wasmtime's headers, I also added a small build.rs that provides links metadata. The resulting environment variables worked as expected for me in my downstream crate (tree-sitter) when I pull the crate from GitHub. I think that means that they will work when the source is pulled from crates.io, but it's possible I've made a mistake there.

I added some content to the c-api README about how to consume the crate from Rust, in the case where you're binding to a C library that uses wasmtime.

I also added a bit to the module-level doc comment for c-api.

Let me know if there were other forms of documentation that make sense for this.

@github-actions github-actions bot added the wasmtime:c-api Issues pertaining to the C API. label Jan 30, 2024
@github-actions
Copy link
Copy Markdown

github-actions bot commented Jan 30, 2024

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

Learn more.

@maxbrunsfeld maxbrunsfeld mentioned this pull request Jan 30, 2024
Merged
10 tasks
@amaanq
Copy link
Copy Markdown

amaanq commented Feb 2, 2024

ping @alexcrichton @peterhuene, this would be immensely helpful for tree-sitter to have the C API published to crates.io 😅 since this is a blocker for us to publish the latest release on crates.io

@maxbrunsfeld
Copy link
Copy Markdown
Contributor Author

maxbrunsfeld commented Feb 2, 2024

I just realized I could test this with dry-run publishing, and that the wasmtime-c-api-macros would need to be published as well. I think that the most straightforward way to do that would be to move it up to the crates folder and add it to the Cargo workspace. I'm going to proceed with doing that on this branch. Let me know if I'm missing something there, and there's a different solution to that macros thing.

alexcrichton
alexcrichton approved these changes Feb 2, 2024
View reviewed changes
Copy link
Copy Markdown
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.

Looks good to me, thanks! I suspect that the publish.rs may need reordering though as well, yeah. If you add prtest:full to a commit message in this PR it'll run the full CI if you'd like (before it goes to the merge queue)

Note that most of the Wasmtime team has been at the BA plumbers summit this week and is traveling home today.

@maxbrunsfeld
Copy link
Copy Markdown
Contributor Author

maxbrunsfeld commented Feb 2, 2024

Ah ok, I'll do that. Thanks for responding while you're all traveling.

@sebastiaanspeck sebastiaanspeck mentioned this pull request Feb 5, 2024
Merged
@alexcrichton
Copy link
Copy Markdown
Member

alexcrichton commented Feb 5, 2024

Ah apologies now that I've gotten back to this it now has a merge conflict. If you don't mind rebasing again I'll add it to the merge queue. Otherwise though I can do the rebase/merge later this week.

@alexcrichton alexcrichton added this pull request to the merge queue Feb 5, 2024
@maxbrunsfeld maxbrunsfeld mentioned this pull request Feb 5, 2024
Closed
20 tasks
Merged via the queue into bytecodealliance:main with commit 825494f Feb 5, 2024
@maxbrunsfeld maxbrunsfeld deleted the publish-c-api branch February 5, 2024 18:48
@maxbrunsfeld
Copy link
Copy Markdown
Contributor Author

maxbrunsfeld commented Feb 5, 2024 •
edited
Loading

Thanks so much @alexcrichton.

Just curious - Now that this is landed, would it be easy to publish to crates.io the wasmtime-c-api-impl corresponding to the latest published wasmtime version, or do we need to wait until the next Wasmtime release?

@alexcrichton
Copy link
Copy Markdown
Member

alexcrichton commented Feb 5, 2024

Doing this for the current release would require a backport and we typically try not to backport features and instead only backport bugfixes. Not impossible but if you're ok I think this would be best to wait.

That being said there's three active branches at the moment. release-17.0.0 has what was released in January and just today release-18.0.0 was created to be released in two weeks. This PR landed on main which will become release-19.0.0 which will be released in March. If you'd like I think it's ok to backport this to the 18.0.0 release branch if you'd prefer to have this in a release sooner rather than later.

@maxbrunsfeld
Copy link
Copy Markdown
Contributor Author

maxbrunsfeld commented Feb 5, 2024

Got it. I would love to backport to 18.0 if possible. Is that something I can help with, via a PR against that branch?

@maxbrunsfeld maxbrunsfeld mentioned this pull request Feb 5, 2024
Closed
@alexcrichton
Copy link
Copy Markdown
Member

alexcrichton commented Feb 5, 2024

Indeed that's all that's required, and please feel free!

@maxbrunsfeld maxbrunsfeld mentioned this pull request Feb 5, 2024
Merged
@maxbrunsfeld
Copy link
Copy Markdown
Contributor Author

maxbrunsfeld commented Feb 5, 2024

Ok, PR up: #7872

elliottt pushed a commit that referenced this pull request Feb 6, 2024
@maxbrunsfeld
Add wasmtime-c-api-impl to the list of crates to publish (#7837) (#7872)
57444d2 
* Add wasmtime-c-api-impl to the list of crates to publish

* Enable rustdoc and publishing for c-api crate

* Provide paths to c-api headers as cargo links metadata

* Add a README section about using wasm-c-api in a rust crate

* In C API doc comment, mention use case for crates w/ C bindings

* Enable publishing for wasmtime-c-api-macros (prtest:full)

* Move c-api crates later in the publishing sequence (prtest:full)
@0323pin 0323pin mentioned this pull request Feb 22, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alexcrichton alexcrichton alexcrichton approved these changes

Assignees

No one assigned

Labels

wasmtime:c-api Issues pertaining to the C API.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@maxbrunsfeld @amaanq @alexcrichton