bytecodealliance / wasmtime

A fast and secure runtime for WebAssembly
https://wasmtime.dev/
Apache License 2.0
15.21k stars 1.28k forks source link

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

Closed juntyr closed 3 months ago

juntyr commented 3 months ago

Fixes #8759

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 commented 3 months ago

@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?

juntyr commented 3 months ago

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:

I'll add that once CI works.

juntyr commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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

juntyr commented 3 months ago

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 commented 3 months ago

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)

juntyr commented 3 months ago

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!

juntyr commented 3 months ago

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