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

Initial wasm component support #367

Merged
merged 15 commits into from
May 15, 2024
Merged

Initial wasm component support #367

merged 15 commits into from
May 15, 2024

Conversation

elliottt
Copy link
Contributor

@elliottt elliottt commented May 8, 2024

This is an initial step towards support for Wasm Components on viceroy 🎉

There are some restrictions to start with:

  1. There aren't currently any toolchains that target components directly, and we don't have SDKs that are able to target the fastly:api/compute world that this branch expects
  2. Components are not supported for the run command

Additionally, if you run a component with viceroy, you will be greeted with a large warning that indicates that this path is currently unsupported.

Reviewing Advice

For the most part, I tried to keep the modifications to additions, though the files that handle linking and execution of wasm/components did require some modification. The additions in lib/src/component should mostly reflect the current state of the core wasm abi, but componentized via the lib/wit/deps/fastly/compute.wit world.

I would appreciate heavy scrutiny on the following files:

  • lib/src/lib.rs
  • lib/src/execute.rs
  • lib/src/linking.rs

Everything in the lib/src/component tree would definitely benefit from additional eyes, but the plan is to consider that execution path unstable for now, and bugs/changes are expected.

elliottt and others added 8 commits May 8, 2024 10:23
@elliottt elliottt requested review from acfoltzer, acw and athomason May 8, 2024 19:17
@@ -0,0 +1,18 @@
interface environment {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🥳

Copy link
Contributor

@acw acw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, most of the comments are either just happy noises, or small suggestions. There were a couple things that I think should be fixed before merge, though. Specifically, there's a wrong implementation in the secret store, and there's an if let ... that should be a match. I also think some guidance on profiling and components might be nice, but I don't think those rise to the level of the first two.

This is awesome stuff!

lib/src/component/async_io.rs Show resolved Hide resolved
lib/src/component/backend.rs Show resolved Hide resolved
lib/src/component/backend.rs Show resolved Hide resolved
lib/src/component/backend.rs Show resolved Hide resolved
lib/src/component/backend.rs Show resolved Hide resolved
lib/wit/deps/fastly/compute.wit Show resolved Hide resolved
lib/wit/deps/filesystem/types.wit Show resolved Hide resolved
lib/wit/deps/http/types.wit Show resolved Hide resolved
lib/wit/deps/random/random.wit Show resolved Hide resolved
lib/wit/viceroy.wit Show resolved Hide resolved
@elliottt elliottt requested a review from acw May 14, 2024 21:55
Copy link
Contributor

@acw acw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks for the changes!

@elliottt elliottt merged commit 7040716 into main May 15, 2024
7 checks passed
@elliottt elliottt deleted the lafp/component branch May 15, 2024 00:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants