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

Host resources wit-bindgen code generation #6886

Merged
merged 29 commits into from
Aug 30, 2023

Conversation

silesmo
Copy link
Member

@silesmo silesmo commented Aug 22, 2023

Implements host export resource generation #6722

After a discussion with @alexcrichton, I have broken out basic resource generation for host resource exports into it's own PR,
Host import resource generation will follow, as well as more ergonomic resource code generation that abstracts away the raw resource handles.

Example implementation: https://github.com/Nor2-io/wasmtime-resources-example/blob/main/example-host-export-index/src/main.rs

Example code generation output: https://github.com/Nor2-io/wasmtime-resources-example/blob/main/example-host-export-index/main%20copy%202.rs

Sneak peak of future additions that abstract away resources can be seen in the link below. (It doesn't work with nested resources yet.)
https://github.com/Nor2-io/wasmtime-resources-example/blob/main/example-host-import/src/main.rs
https://github.com/Nor2-io/wasmtime-resources-example/blob/main/example-host-export/src/main.rs

Added temporary exception for cargo deny on multiple versions until memfd has been updated: #62

@silesmo silesmo requested review from a team as code owners August 22, 2023 20:40
@silesmo silesmo requested review from alexcrichton and removed request for a team August 22, 2023 20:40
Copy link
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.

Thanks again for this! I'm running out of time today but wanted to give some quick feedback on this. There's a few comments below and I have more thoughts on the generated code but I'll try to flesh that out more tomorrow (I'll probably send a PR-to-your-PR in the morning)

In the meantime though this'll need to have some tests as well, both at crates/component-macro/tests/codegen/*.wit and additionally one in tests/all/component_model/*. You'll find some other bindgen-related tests in that directory too.

Cargo.lock Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
crates/wasmtime/src/component/linker.rs Show resolved Hide resolved
crates/wasi-http/Cargo.toml Outdated Show resolved Hide resolved
@alexcrichton alexcrichton self-assigned this Aug 22, 2023
@github-actions github-actions bot added the wasmtime:api Related to the API of the `wasmtime` crate itself label Aug 22, 2023
@github-actions
Copy link

Subscribe to Label Action

cc @peterhuene

This issue or pull request has been labeled: "wasmtime:api"

Thus the following users have been cc'd because of the following labels:

  • peterhuene: wasmtime:api

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

silesmo and others added 13 commits August 23, 2023 10:02
Avoid collecting resources-as-we-go in favor of doing that more
declaratively elsewhere. Additionally handle imported-vs-exported
resource via the order that interfaces are visited.
* Use `&mut self` instead of `StoreContextMut`
* Make resource traits as supertraits of the `Host` trait generated for
  each resource.
* More uniformly handle types/returns with resource methods.
* Fix derivations of `Clone` and `Copy` for where handles are contained.
@alexcrichton
Copy link
Member

Ok I've worked on this a bit this morning and I've pushed up Nor2-io#1 which contains another round of comments for me (and some fixes for the deny/vet-related things I think). As I mentioned over there I'm happy to go through anything if you'd like clarifications or explanations!

silesmo and others added 6 commits August 23, 2023 22:05
Updates to host resources and `bindgen!`

resources in exported functions arguments break with this commit and will be fixed in following commits.
This is now the same as the preexisting `generate_function_trait_sig`
This determination happens by looking up the origin definition of a
resource, not the leaf possibly-aliased type.
Resources don't have their representation asserted since the bare type
itself doesn't implement `ComponentType`, but otherwise generate type
aliases the same way as other type aliases.
No need to update `wit-bindgen` to 0.10.0 just yet, that'll happen in a
future update if necessary.
@alexcrichton
Copy link
Member

One more round of changes at Nor2-io#2 (again happy to go through anything if you'd like), and then I think this is good to go 👍

Copy link
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.

Thanks again @silesmo for all your work on this!

Since this now has bits and pieces from me too I'm going to ask @pchickey to do a once-over on this to confirm it's all reasonable, but otherwise I think this is good to go 👍

@alexcrichton
Copy link
Member

Also @silesmo would you be up for filing an issue for follow-up work?

@silesmo
Copy link
Member Author

silesmo commented Aug 24, 2023

Also @silesmo would you be up for filing an issue for follow-up work?

Yes I can do that! 🙂

@alexcrichton alexcrichton removed the request for review from pchickey August 28, 2023 18:39
@alexcrichton
Copy link
Member

Pat's busy with streams and such so he'll take a look at this after-the-fact, so I'm going to go ahead and flag this for merge. Thanks again @silesmo!

@alexcrichton
Copy link
Member

oh I think this may need a rebase for merging now?

auto-merge was automatically disabled August 30, 2023 02:50

Head branch was pushed to by a user without write access

@silesmo
Copy link
Member Author

silesmo commented Aug 30, 2023

oh I think this may need a rebase for merging now?

I merged the latest changes now

@alexcrichton alexcrichton added this pull request to the merge queue Aug 30, 2023
Merged via the queue into bytecodealliance:main with commit 878a243 Aug 30, 2023
18 checks passed
eduardomourar pushed a commit to eduardomourar/wasmtime that referenced this pull request Sep 6, 2023
* added trap to resource dtor

* basic resources

* fixed signature and trait bounds

* fixed static function

* fixed trait signature

* basic resources

* added result to resource drop

* reverted formatting

* added doc comment to resource `dtor`

* fixed merge issue

* fixed another merge issue

* added resource import codegen test

* Revert changes to `Cargo.lock`

* Update `Cargo.lock` with the wit-bindgen update

* Add `cargo vet` entries for new crates

* Restore old-style of printing types in bindgen

Avoid collecting resources-as-we-go in favor of doing that more
declaratively elsewhere. Additionally handle imported-vs-exported
resource via the order that interfaces are visited.

* Update the shape of resource traits:

* Use `&mut self` instead of `StoreContextMut`
* Make resource traits as supertraits of the `Host` trait generated for
  each resource.
* More uniformly handle types/returns with resource methods.
* Fix derivations of `Clone` and `Copy` for where handles are contained.

* Fix generation of handle typedefs

* Support resources-in-worlds

* Remove now-duplicate function

This is now the same as the preexisting `generate_function_trait_sig`

* Fix classifying handles as imported or exported

This determination happens by looking up the origin definition of a
resource, not the leaf possibly-aliased type.

* Fix chains-of-use of resources

Resources don't have their representation asserted since the bare type
itself doesn't implement `ComponentType`, but otherwise generate type
aliases the same way as other type aliases.

* Revert `Cargo.lock` changes

No need to update `wit-bindgen` to 0.10.0 just yet, that'll happen in a
future update if necessary.

* Add basic runtime tests for resources

* fixed merge issue

---------

Co-authored-by: Alex Crichton <alex@alexcrichton.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wasmtime:api Related to the API of the `wasmtime` crate itself
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants