wasi-adapter: Implement provider crate that embeds the adapter binaries #8792

juntyr · 2024-06-13T05:37:04Z

This PR adds a new wasi-preview1-component-adapter-provider crate that embeds the binary contents of the three wasi adapters. As per @alexcrichton's suggestion, this crate contains the adapter files as artefacts, which are checked in CI to match the freshly compiled versions of the wasi-preview1-component-adapter crate. The new crate is intended to be published on crates.io and allow Rust code which programmatically builds WASM components to use the adapter without needing more than a normal crate dependency (instead of having to manually download the file).

juntyr · 2024-06-13T05:38:01Z

crates/wasi-preview1-component-adapter/provider/src/lib.rs

+pub const WASI_SNAPSHOT_PREVIEW1_COMMAND_ADAPTER: &[u8] =
+    include_bytes!("../artefacts/wasi_snapshot_preview1.command.wasm");
+
+/// The "proxy" adapter provides implements a HTTP proxy.


@alexcrichton I didn't find as much information on the proxy online and in code. Does it implement a pre-defined HTTP proxy world?

It corresponds to this world, notably wasi:http/proxy which doesn't have filesystem/env vars/cli arguments/etc

Thanks! I think what confused me is that the adapter defines the world inline here:

wasmtime/crates/wasi-preview1-component-adapter/src/lib.rs

Lines 83 to 102 in 3bcbd5e

#[cfg(feature = "proxy")]

wit_bindgen_rust_macro::generate!({

path: "../wasi-http/wit",

inline: r#"

package wasmtime:adapter;

world adapter {

import wasi:clocks/wall-clock@0.2.0;

import wasi:clocks/monotonic-clock@0.2.0;

import wasi:random/random@0.2.0;

import wasi:cli/stdout@0.2.0;

import wasi:cli/stderr@0.2.0;

import wasi:cli/stdin@0.2.0;

}

"#,

std_feature,

raw_strings,

runtime_path: "crate::bindings::wit_bindgen_rt_shim",

skip: ["poll"],

});

which includes most imports but not the incoming HTTP request handler import and not the outgoing HTTP request export.

juntyr · 2024-06-13T05:54:38Z

@alexcrichton Well at least the CI check works :)

I tried rebuilding the adapters locally with 1.76.0 stable and the pinned wasm-tools version and got the local CI script to pass, but the GitHub CI check still failed. Is there something else I need to do to make the build reproducible and push a matching artefact?

alexcrichton

Thanks for this! For the CI failure you say you're using Rust 1.76 but CI is using Rust 1.78, so that may be the culprit?

In addition to that there's a few more things here I think:

By default the version of Rust used to compile the adapter will change whenever we bump the MSRV. This seems a little unfortunate and I think it'd be better to do this as a discrete step for the adapter to reduce the annoyance of updating our MSRV. Could the rust version be explicitly specified for the CI job that builds the adapter as exactly 1.78.0? This might require splitting out the CI job so it's only build the adapter and doing nothing else.
Currently there's not an easy way to rebuild the adapter locally and then check in the results. Could the cmp commands in the build script be conditional somehow? For example they should only run on CI but locally you can skip them if you want to regenerate all the adapters.

alexcrichton · 2024-06-13T15:00:24Z

crates/wasi-preview1-component-adapter/provider/src/lib.rs

+pub const WASI_SNAPSHOT_PREVIEW1_COMMAND_ADAPTER: &[u8] =
+    include_bytes!("../artefacts/wasi_snapshot_preview1.command.wasm");
+
+/// The "proxy" adapter provides implements a HTTP proxy.


It corresponds to this world, notably wasi:http/proxy which doesn't have filesystem/env vars/cli arguments/etc

juntyr · 2024-06-14T07:44:36Z

Thanks for this! For the CI failure you say you're using Rust 1.76 but CI is using Rust 1.78, so that may be the culprit?

In addition to that there's a few more things here I think:

By default the version of Rust used to compile the adapter will change whenever we bump the MSRV. This seems a little unfortunate and I think it'd be better to do this as a discrete step for the adapter to reduce the annoyance of updating our MSRV. Could the rust version be explicitly specified for the CI job that builds the adapter as exactly 1.78.0? This might require splitting out the CI job so it's only build the adapter and doing nothing else.

Currently there's not an easy way to rebuild the adapter locally and then check in the results. Could the cmp commands in the build script be conditional somehow? For example they should only run on CI but locally you can skip them if you want to regenerate all the adapters.

I'm a bit stumped. I check in CI and it now seems to build with 1.79 but even when I build locally with 1.79 it still produces different artefacts.

I absolutely agree we should explicitly lock down the adapter build settings to ensure that local build and CI builds remain the same unless someone explicitly changes that setting (including the Rust version). Once I have a successful build, I'll implement that.

What we could do perhaps is to offer two (mutually exclusive) options for the CI script:

no options just build the adapters to the target folder, as before
--replace also copies the newly built adapters into the provider crate
--check checks the newly built adapters against the provider crate (used in CI)

I'll add that once CI works.

juntyr · 2024-06-14T07:59:13Z

I restarted my dev container, cleared my target folder, followed all toolchain setup steps from the job output log, and reran the build script locally, which produced no change in the adapter binaries. So locally the build is reproducible, but something must still be different in CI.

alexcrichton · 2024-06-14T14:47:44Z

Ah yesterday it was Rust 1.78 but we upgraded to 1.79 yesterday too, explaining that discrepancy. What os are you on? I've seen that macOS produces different binaries than Linux, for example, and that might be happening here. I can try to dig in today as well and see what's going on.

juntyr · 2024-06-14T17:52:38Z

I was running on a Linux GitHub codespace container and tried both 1.78 and 1.79 today. I also used the same version of wasm tools as the one in CI and checked the Rust version hash from the CI job log. If you do have a bit of time to spare to investigate, that would be brilliant :)

alexcrichton · 2024-06-14T18:44:30Z

Hm interestingly I checked out this branch, ran the script, and it didn't fail. Locally I've got rustc 1.78 and wasm-tools 1.201, as opposed to CI which for the last build used rustc 1.79 and wasm-tools 1.0.27.

I added some debugging in the hopes of seeing what's happening in CI

alexcrichton · 2024-06-14T18:57:23Z

Ok now I think it's more clear what's happening:

Reference copy of adapter is not the same as the generated copy of
the adapter
  reference copy: crates/wasi-preview1-component-adapter/provider/artefacts/wasi_snapshot_preview1.command.wasm
      built copy: target/wasm32-unknown-unknown/release/wasi_snapshot_preview1.command.wasm
To automatically update the reference copy set `BLESS=1` in the
environment
--- /dev/fd/63	2024-06-14 18:50:51.726335825 +0000
+++ /dev/fd/62	2024-06-14 18:50:51.726335825 +0000
@@ -1,4 +1,4 @@
-(module $wasi_preview1_component_adapter.command.adapter:
+(module $wasi_preview1_component_adapter.command.adapter:9a152d0ad902adeb995c2ab83556f62119151301
   (type (;0;) (func))
   (type (;1;) (func (param i32)))
   (type (;2;) (func (result i64)))

CI only sets the VERSION environment variable to the current git commit. That I think will no longer be viable with this approach so we'll need to remove that from CI and the script here.

juntyr · 2024-06-14T19:04:53Z

Ok now I think it's more clear what's happening:

Reference copy of adapter is not the same as the generated copy of
the adapter
  reference copy: crates/wasi-preview1-component-adapter/provider/artefacts/wasi_snapshot_preview1.command.wasm
      built copy: target/wasm32-unknown-unknown/release/wasi_snapshot_preview1.command.wasm
To automatically update the reference copy set `BLESS=1` in the
environment
--- /dev/fd/63	2024-06-14 18:50:51.726335825 +0000
+++ /dev/fd/62	2024-06-14 18:50:51.726335825 +0000
@@ -1,4 +1,4 @@
-(module $wasi_preview1_component_adapter.command.adapter:
+(module $wasi_preview1_component_adapter.command.adapter:9a152d0ad902adeb995c2ab83556f62119151301
   (type (;0;) (func))
   (type (;1;) (func (param i32)))
   (type (;2;) (func (result i64)))

CI only sets the VERSION environment variable to the current git commit. That I think will no longer be viable with this approach so we'll need to remove that from CI and the script here.

Oooh, that makes sense! I never set the version variable locally and of course we can only know it once a commit is made.

I do like the idea that the version points to the last commit when the adapter was changed. Could git be used to check the file tree, excluding the artefacts folder, for the latest commit hash, and then use that?

alexcrichton · 2024-06-14T19:06:01Z

Yeah I think that'd also be reasonable to do!

juntyr · 2024-06-15T09:54:33Z

Yeah I think that'd also be reasonable to do!

I tried, I tried. It unfortunately doesn't work out as any commit that would change the adapter would need to refer to itself to provide the updated artefacts (especially since in CI we run on merge-into-main commits which don't exist yet). So I've reverted to the most basic solution of just using the crate version :/

juntyr · 2024-06-15T14:41:52Z

If we do want more info in the version, it could also include a hash of the source files (but this would only help to distinguish git versions, though without relation to their commit hash, not published versions)

alexcrichton

I think it's reasonable to remove the hash yeah. One difficulty of using the crate version though is that these binaries would need to be updated whenever we bump the version of the repo which is currently an automated process. It's possible to update that automation, but I think it's probably just easiest to remove the version info entirely for now.

Otherwise can you also move the CI checks for the adapter to their own build job? That way it can pin rustc to a fixed version which isn't updated when we update the rest of CI in the future (done by updating our MSRV right now).

As some minor follow-up work:

Can this update scripts/publish.rs as well? I think that'll end up getting caught when full CI runs on merge.
Mind updating crates/test-programs/artifacts/build.rs to depend on this crate with a [build-dependencies]`? That otherwise builds the adapters right now and it shouldn't need to any longer after this.

juntyr · 2024-06-17T10:21:02Z

I think it's reasonable to remove the hash yeah. One difficulty of using the crate version though is that these binaries would need to be updated whenever we bump the version of the repo which is currently an automated process. It's possible to update that automation, but I think it's probably just easiest to remove the version info entirely for now.

What if the adapter crate got its own explicit version, that could be bumped independently of the other crates? I've prototyped this for now, but I can also revert the change and use the workspace version and not add the version to the metadata.

Otherwise can you also move the CI checks for the adapter to their own build job? That way it can pin rustc to a fixed version which isn't updated when we update the rest of CI in the future (done by updating our MSRV right now).

Done - the adapter crate now lists an explicit MSRV which the build script uses.

As some minor follow-up work:

* Can this update `scripts/publish.rs` as well? I think that'll end up getting caught when full CI runs on merge.

Done!

* Mind updating `crates/test-programs/artifacts/build.rs to depend on this crate with a `[build-dependencies]`? That otherwise builds the adapters right now and it shouldn't need to any longer after this.

Done!

alexcrichton · 2024-06-17T16:34:02Z

ci/build-wasi-preview1-component-adapter.sh

+rustup toolchain install $RUST_VERSION --profile minimal
+rustup target add wasm32-wasi wasm32-unknown-unknown --toolchain $RUST_VERSION
+
+export RUSTUP_TOOLCHAIN=$RUST_VERSION
+
+cargo --version


Could this be deferred to CI configuration rather than built-in to this script? It might make sense to double-check that the rustc version activated matches the rust-version on the crate itself but otherwise installation of the toolchain can get unwieldy to work with when running this script locally.

alexcrichton · 2024-06-17T16:37:51Z

crates/wasi-preview1-component-adapter/Cargo.toml

@@ -1,6 +1,7 @@
 [package]
 name = "wasi-preview1-component-adapter"
-version.workspace = true
+version = "23.0.0+wasi-0.2.0"


While having an explicit version here improves things my worry is that this will basically never get updated, even when it needs to. Currently all other version management in this repository is automated so I would expect that this would never change as a result.

I can think of two other primary alternatives for continuing to embed version information in the adapter:

Update the bump-version script/CI configuration to additionally rebuild the adapters. I find this not super appealing though because it adds binary file churn to this repository which feels unfortunate.

Add a build.rs to the provider crate which injects version information. To be maximally useful this would probably want to have zero build-dependencies which makes it a bit difficult to do, though.

Overall I'd probably still be most in favor of just dropping the version information in the binary. AFAIK we haven't really ended up using it all that much in practice.

Ok, let's just remove the version info (for now) then

juntyr · 2024-06-18T05:34:31Z

@alexcrichton I hope this is now good to go - thank you for all of your rounds of feedback!

alexcrichton

Thank you as well for pushing on this!

…r binaries (bytecodealliance#8792)" This reverts commit 2dbf8f1.

…r binaries (#8792)" (#8856) * Revert "wasi-adapter: Implement provider crate that embeds the adapter binaries (#8792)" This reverts commit 2dbf8f1. * Enable requisite feature for cranelift-frontend testing

juntyr requested review from a team as code owners June 13, 2024 05:37

juntyr requested review from alexcrichton and removed request for a team June 13, 2024 05:37

juntyr commented Jun 13, 2024

View reviewed changes

alexcrichton reviewed Jun 13, 2024

View reviewed changes

juntyr force-pushed the wasi-adapter-provider branch from e617878 to 631f48a Compare June 15, 2024 08:35

alexcrichton reviewed Jun 16, 2024

View reviewed changes

alexcrichton reviewed Jun 17, 2024

View reviewed changes

juntyr and others added 9 commits June 18, 2024 05:28

wasi-adapter: Implement provider crate that embeds the adapter binaries

297085e

Upgrade wasi adapters to the latest version

675856a

Update adapter docs

3e2cadb

Recompile asi adapters with 1.78

f95a9be

Recompile wasi adapters with 1.79

b30a5f7

Add some debugging to adapter build script

f47b9d0

Fix script debugging

624605a

Compute wasi adapter version based on latest adapter commit hash

3a9b0c6

Try to bless wasi adapters again

0885e44

juntyr added 9 commits June 18, 2024 05:28

Try to work around CI auto-merges

2781757

Revert to just using workspace version

aa86354

Add the wasi adapter provider to the crate publication list

c752987

Use wasi adapter provider in artifacts test + explicit MSRV in CI

8c6b8f8

Explicit adapter crate version

62dd99c

Small fix

4704783

Remove version info from adapter metadata

801e849

Check but don't install rust toolchain in build script

0a62a13

Bless after rebase

bde3ed2

juntyr force-pushed the wasi-adapter-provider branch from 002f7f0 to bde3ed2 Compare June 18, 2024 05:29

alexcrichton approved these changes Jun 18, 2024

View reviewed changes

alexcrichton added this pull request to the merge queue Jun 18, 2024

Merged via the queue into bytecodealliance:main with commit 2dbf8f1 Jun 18, 2024
36 checks passed

alexcrichton added a commit to alexcrichton/wasmtime that referenced this pull request Jun 20, 2024

Revert "wasi-adapter: Implement provider crate that embeds the adapte…

6a35d26

…r binaries (bytecodealliance#8792)" This reverts commit 2dbf8f1.

alexcrichton mentioned this pull request Jun 20, 2024

Revert "wasi-adapter: Implement provider crate that embeds the adapter binaries (#8792)" #8856

Merged

rvolosatovs mentioned this pull request Jun 21, 2024

Deprecate in favor of wasi-preview1-component-adapter-provider wasmCloud/wasmcloud-component-adapters#24

Closed

juntyr mentioned this pull request Jun 26, 2024

wasi-adapter: Implement provider crate that embeds the adapter binaries [v2] #8874

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

wasi-adapter: Implement provider crate that embeds the adapter binaries #8792

wasi-adapter: Implement provider crate that embeds the adapter binaries #8792

juntyr commented Jun 13, 2024

juntyr Jun 13, 2024

alexcrichton Jun 13, 2024

juntyr Jun 14, 2024

juntyr commented Jun 13, 2024

alexcrichton left a comment

alexcrichton Jun 13, 2024

juntyr commented Jun 14, 2024

juntyr commented Jun 14, 2024

alexcrichton commented Jun 14, 2024

juntyr commented Jun 14, 2024

alexcrichton commented Jun 14, 2024

alexcrichton commented Jun 14, 2024

juntyr commented Jun 14, 2024

alexcrichton commented Jun 14, 2024

juntyr commented Jun 15, 2024

juntyr commented Jun 15, 2024

alexcrichton left a comment

juntyr commented Jun 17, 2024

alexcrichton Jun 17, 2024

juntyr Jun 18, 2024

alexcrichton Jun 17, 2024

juntyr Jun 18, 2024

juntyr commented Jun 18, 2024

alexcrichton left a comment

	#[cfg(feature = "proxy")]
	wit_bindgen_rust_macro::generate!({
	path: "../wasi-http/wit",
	inline: r#"
	package wasmtime:adapter;

	world adapter {
	import wasi:clocks/wall-clock@0.2.0;
	import wasi:clocks/monotonic-clock@0.2.0;
	import wasi:random/random@0.2.0;
	import wasi:cli/stdout@0.2.0;
	import wasi:cli/stderr@0.2.0;
	import wasi:cli/stdin@0.2.0;
	}
	"#,
	std_feature,
	raw_strings,
	runtime_path: "crate::bindings::wit_bindgen_rt_shim",
	skip: ["poll"],
	});

wasi-adapter: Implement provider crate that embeds the adapter binaries #8792

wasi-adapter: Implement provider crate that embeds the adapter binaries #8792

Conversation

juntyr commented Jun 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juntyr commented Jun 13, 2024

alexcrichton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juntyr commented Jun 14, 2024

juntyr commented Jun 14, 2024

alexcrichton commented Jun 14, 2024

juntyr commented Jun 14, 2024

alexcrichton commented Jun 14, 2024

alexcrichton commented Jun 14, 2024

juntyr commented Jun 14, 2024

alexcrichton commented Jun 14, 2024

juntyr commented Jun 15, 2024

juntyr commented Jun 15, 2024

alexcrichton left a comment

Choose a reason for hiding this comment

juntyr commented Jun 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juntyr commented Jun 18, 2024

alexcrichton left a comment

Choose a reason for hiding this comment