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

Support adapting core wasm to components #374

Merged
merged 15 commits into from
Jun 5, 2024
Merged

Conversation

elliottt
Copy link
Contributor

@elliottt elliottt commented May 23, 2024

This PR adds a component adapter based on the wasi_snapshot_preview1 adapter that also supports the fastly host call abi. The adapter can be used in two forms:

  1. using the new viceroy adapt command to adapt a core-wasm module to a component ahead-of-time,
  2. using the --adapt flag to viceroy serve or viceroy run, adapting the core-wasm in-memory.

This functionality opens the door to running viceroy's existing test suite with components, and also to a longer-term migration towards components in-general.

This feature comes with a downside to the development experience though: any changes made to the adapter require that we also run the make adapter target, and commit the changes to the wasi_snapshot_preview1.wasm binary. It would be great to check if this was necessary in CI, but it's not clear to me what the right mechanism is to enforce that. However, changes to the adapter should be infrequent, so hopefully this isn't too much of a burden.

@elliottt elliottt force-pushed the trevor/component-adapter branch 2 times, most recently from ee9e367 to c6ef717 Compare May 24, 2024 00:06
@elliottt elliottt force-pushed the trevor/component-adapter branch 3 times, most recently from 698ec38 to 79e86af Compare May 30, 2024 18:44
@elliottt elliottt marked this pull request as ready for review May 30, 2024 23:22
@elliottt elliottt requested review from acfoltzer and acw May 30, 2024 23:22
@elliottt elliottt changed the title Add the adapt command to produce components from core wasm Support adapting core wasm to components May 30, 2024
Copy link
Contributor

@acfoltzer acfoltzer left a comment

Choose a reason for hiding this comment

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

So exciting to see this coming together!

Mostly minor comments from me on this one. I will cop to only skimming much of the adapter code, since it is so mechanical. I tried to at least make sure that we were adapting the right thing in each case.

crates/adapter/Cargo.toml Outdated Show resolved Hide resolved
cli/src/main.rs Outdated Show resolved Hide resolved
cli/src/opts.rs Outdated Show resolved Hide resolved
crates/adapter/src/macros.rs Show resolved Hide resolved
crates/adapter/src/macros.rs Show resolved Hide resolved
crates/adapter/src/lib.rs Show resolved Hide resolved
crates/adapter/src/lib.rs Show resolved Hide resolved
crates/adapter/src/lib.rs Show resolved Hide resolved
crates/adapter/src/lib.rs Show resolved Hide resolved
crates/adapter/src/fastly/config_store.rs Outdated Show resolved Hide resolved
@elliottt elliottt requested a review from acfoltzer June 5, 2024 18:20
Copy link
Contributor

@acfoltzer acfoltzer left a comment

Choose a reason for hiding this comment

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

🚀 Looks great! As discussed in Slack, a number of my comments were really on the upstream preview1 adapter, and we're going to hold off on making those changes here in order to avoid drift and confusion from upstream.

@elliottt elliottt merged commit bc77f49 into main Jun 5, 2024
7 checks passed
@elliottt elliottt deleted the trevor/component-adapter branch June 5, 2024 20:44
GeeWee pushed a commit to GeeWee/Viceroy that referenced this pull request Jul 25, 2024
This PR adds a component adapter based on the wasi_snapshot_preview1 adapter
that also supports the fastly host call abi. The adapter can be used in two
forms:

1. using the new viceroy adapt command to adapt a core-wasm module to a
   component ahead-of-time,
2. using the --adapt flag to viceroy serve or viceroy run, adapting the
   core-wasm in-memory. This functionality opens the door to running viceroy's
   existing test suite with components, and also to a longer-term migration
   towards components in-general.

This feature comes with a downside to the development experience though: any
changes made to the adapter require that we also run the make adapter target,
and commit the changes to the wasi_snapshot_preview1.wasm binary. It would be
great to check if this was necessary in CI, but it's not clear to me what the
right mechanism is to enforce that. However, changes to the adapter should be
infrequent, so hopefully this isn't too much of a burden.
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.

2 participants