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

add linker to wasi preview2 that accepts custom closure #7778

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

sehz
Copy link

@sehz sehz commented Jan 15, 2024

Allow passing of closure to wasi command linking. Current add_to_linker is re-implemented more general API. Without this, it's hard to create reusable wasi context store

@sehz sehz requested a review from a team as a code owner January 15, 2024 23:32
@sehz sehz requested review from alexcrichton and removed request for a team January 15, 2024 23:32
@github-actions github-actions bot added the wasi Issues pertaining to WASI label Jan 16, 2024
@alexcrichton
Copy link
Member

Thanks for the PR, but this is something we were ideally hoping to avoid with the WasiView trait design. Would it be possible to implement WasiView for your type T within the Store<T>?

@sehz
Copy link
Author

sehz commented Jan 18, 2024

Issue is that T is becoming very complex. Ideally Store should be composable. Key problem is that store is tied to linker which makes difficult separate host implementation

@alexcrichton
Copy link
Member

I understand that T can be complex, and I also understand that writing a closure is shorter than writing a trait impl, but I'm not sure why the complexity would make trait impls more difficult (as opposed to a simple T). The fact that the store and linker are tied together is indeed a bit limiting, but with trait bounds it should be possible to design roughly the sme APIs.

Is there anything preventing the trait impls on T in your case?

@alexcrichton
Copy link
Member

We chattec a bit more tonight about this and I think it's a reasonable enough feature to add given it's pretty small and solves a concrete use case. If you're ok fixing the failing test on CI (I think there's a new trait bound needed) I can r+

@sehz
Copy link
Author

sehz commented Feb 1, 2024

Thanks. Will fix CI

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wasi Issues pertaining to WASI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants