bytecodealliance / wasmtime

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

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

Open juntyr opened 6 days ago

juntyr commented 6 days ago

Follow-up to #8792, which was reverted in #8856. The new design follows @alexcrichton's suggestions in https://github.com/bytecodealliance/wasmtime/pull/8856#issuecomment-2182798604.

The provider repo is materialised into the workspace to ensure that it is built with the same version, lints, etc as the other crates.

juntyr commented 6 days ago

I absolutely understand your concerns.

I like the idea of downloading the adapter binaries as artifacts rather than rebuilding them as it guarantees they're the same as the published ones. Downloading from the tag though feels like it might be brittle and run a risk of racing with the job that's uploading artifacts per-tag. Could the artifact instead be downloaded with the actions/download-artifact action? Also, could the logic to download this and package it be shared with the per-PR CI too?

That's a good idea! I don't have experience with workflow artefacts yet but I'll see how they work.

Could publish.rs perhaps be leveraged for this? That way delta in logic-on-each PR and logic-on-publish is as small as possible.

That would be one option. Alternatively, we could have a common CI script, which is invoked in the normal CI and during publish, which downloads the artefact and does rustfmt+check+clippy+package checks (skipped during publish, so this would be a codepath that's not exercised during publish)? Then the publish action would call the same script as CI, and only the cargo publish to crates.io would only happen in the publish action.

If cargo publish --dry-run can execute without credentials, we could also run that in CI as well.

Also ideally this could be done with a "dry run" of sorts to ensure that everything actually works once we hit the publication of all this. Would you be up for getting this running on your fork of Wasmtime for example? You'll probably have to comment/tweak some other things to not publish to crates.io for example, though.

I hadn't thought of this, but yes of course! In theory, adding --dry-run into the publish.rs script (and to the provider crate publish step) should be sufficient? Once we have the scripts in a good state, I'll do a run-through.

I hope I can implement your ideas tomorrow and then do a test run on Friday, so there is enough time before the next publish cycle kicks off (if things take longer, we'll just take the next train)

juntyr commented 5 days ago

I tried to run the full publish cycle in my own repo but have come across some issues (I'm yet at the stage where I can actually test the adapter):

The tests were run on top of https://github.com/juntyr/wasmtime/pull/10, which includes the extra commit https://github.com/juntyr/wasmtime/pull/10/commits/cbb0a0a074a54781bcccdb2d4cf4ba9e6397fe37 to run the full tests / publish cycle in my repo and without actually executing any cargo publish.

Do you have any ideas? Also, what do you think of the current implementation?

juntyr commented 4 days ago

@alexcrichton I've slowly been getting further and further into the try runs and I now have one running that I hope will work.

The extra commit to enable the try is https://github.com/juntyr/wasmtime/pull/19/commits/f1ad3a341bca34957dc543119a8046048269ebe9 - the issue with the artifact action was the wasmtime repo seems to have the artifacts under the hash of the merge commit (maybe because of the merge queue), which I cannot reproduce in my repo, so I just pick a run since it doesn't matter for the try right now.

I'll report back once https://github.com/juntyr/wasmtime/actions/runs/9715657045 (and the follow-up publish action has been completed) - does the implementation look good already or is there anything else that should be changed?

juntyr commented 4 days ago

[Just using any run id for testing perhaps wasn't such a good idea (I think the workflow should work but right now it chooses a run id without artifacts ... I'll try to see if I can get lucky, otherwise I need a better way to filter the runs so that we can demonstrate that the publish action works)]

None of this of course affects the code in this PR, just the extra commit in my own repo to test the full publish pipeline

juntyr commented 4 days ago

I feel like GitHub has started to rate limit my repo as the CI action just no longer even starts (e.g. https://github.com/juntyr/wasmtime/pull/23), without any notice. I'll keep an eye on this tonight, but I'm not sure my test run can do much more tonight, I'm sorry

juntyr commented 4 days ago

@alexcrichton I got a full release process run to succeed:

I hope the implementation and full try-run of the release process are now good to go :)