wasi-adapter: Implement provider crate that embeds the adapter binaries [v2] by juntyr · Pull Request #8874 · bytecodealliance/wasmtime

juntyr · 2024-06-26T11:47:50Z

Follow-up to #8792, which was reverted in #8856. The new design follows @alexcrichton's suggestions in #8856 (comment).

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

repo contains a template for the provider crate
template crate is materialised in CI to be tested
template crate is materialised just before publishing

alexcrichton

Thanks for doing this! My biggest worry with this though is that there's nontrivial logic which will only get exercised once-a-month at a time and place where no one's really watching CI for failures and it's difficult to fix. Notably this is the publish-to-cratesio.yml workflow with logic there.

The viability of this approach of synthesizing a crate on-the-fly to publish in my mind hinges on that being as simple as possible. For example the sed command there is duplicated with the one in build-wasi-preview1-component-adapter.sh. I understand why it's duplicated, but it's increasing the risk of a publish-only failure that isn't otherwise caught during development.

One example is that publish.rs runs in "verify" mode on all PRs and only does publication errors later on. This has caught a significant number of errors where they would have only otherwise appeared later on during publication.

I'll admit though that I'm at a bit of a loss of how best to do this. Without native support in Cargo for something like this I've never come across a great way to orchestrate this. Here's some loose thoughts though:

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

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.

Sorry I understand I'm asking for a fair bit here. Much of this to me stems from Cargo not having native support for a feature like this and otherwise I don't think there's a non-brittle way to orchestrate this.

juntyr · 2024-06-26T19:43:52Z

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 · 2024-06-27T12:23:18Z

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 publish artefact actions always fail: e.g. https://github.com/juntyr/wasmtime/actions/runs/9695441124/job/26755206220
my latest release attempt failed without any logs at all: https://github.com/juntyr/wasmtime/actions/runs/9695801920/job/26756378300

The tests were run on top of juntyr#10, which includes the extra commit juntyr@cbb0a0a 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?

alexcrichton

my latest release attempt failed without any logs at all

For this I check the summary page which shows:

GitHub Actions has encountered an internal error when running your job.

So I think that's just a transient error.

Also, what do you think of the current implementation?

I'm liking how it's shaping up! I left a few comments related to composite actions how I think this can refactor out a bit more, but otherwise looks good 👍

alexcrichton · 2024-06-27T13:55:55Z

+    - run: |
+        sha=${{ github.sha }}
+        # Remember to update this logic in publish-artifacts.yml as well
+        echo ARTIFACT_RUN_ID=$(
+          gh api -H 'Accept: application/vnd.github+json' \
+              /repos/${{ github.repository }}/actions/workflows/main.yml/runs\?exclude_pull_requests=true \
+              | jq '.workflow_runs' \
+              | jq "map(select(.head_commit.id == \"$sha\"))[0].id" \
+        ) >> "$GITHUB_ENV"


I think this might be missing github.token being pulled from secrets and set in the environment? I think that would mean that this might not have the right permissions and might be why it's failing for you

Also since this is duplicated with publish-artifacts.yml it might be a good candidate for a composite action we could store in .github/actions/*

alexcrichton · 2024-06-27T14:00:10Z

+    - uses: actions/download-artifact@v4
+      with:
+        name: bins-wasi-preview1-component-adapter
+        path: crates/wasi-preview1-component-adapter/provider/artefacts
+
+    - run: ./ci/build-wasi-preview1-component-adapter-provider.sh
+      env:
+        CHECK: 1


Could these steps be pulled into a composite action as well? That way this could be shared with the publish step, notably the configuration of the download-artifact step. Also I think it's ok to remove the CHECK variable and run the same checks on publication since it's a quick-to-build crate and we can throw some more tools like clippy on the publish step.

In theory that means that the publish step is:

Run the composite action to figure out the run-id

Run the composite action to setup the artifacts and perform checks

Run a publish

That feels like a good state to me -- aka I like this refactoring 👍

So this would be two new composite actions?

Yeah, one for acquiring the run-id matching github.sha used during publish-artifacts and publish-to-cratesio. Another for verifying the adapter shared between main.yml and publish-to-cratesio

juntyr · 2024-06-29T07:19:58Z

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

Wasi adapter preview v2 try juntyr/wasmtime#22 is the PR which includes this PR and the extra commit juntyr@0f9c451 to allow CI to run in my repo (and to pick any run id instead of the sha of the merged commit one since that one doesn't work in my repo, my guess is because it doesn't have a merge queue, but we already know it works in the main repo and at the latest this will be caught immediately after merging the PR when the publish artifacts workflow runs)
Bump Wasmtime to 24.0.0 juntyr/wasmtime#24 is the release cut PR (full CI success in the PR; publish artifact action after merge fails in https://github.com/juntyr/wasmtime/actions/runs/9721774907/job/26834892676 after successfully completing my code, since I don't have GitHub pages set up in my fork)
Release Wasmtime 23.0.0 juntyr/wasmtime#25 is the release latest PR (full CI success in the PR)
https://github.com/juntyr/wasmtime/actions/runs/9722005042 is the full release CI run on the to-be-released branch (all succeed, needed a 2nd run since I forgot to remove the release tag from a previous run and so it succeeded but didn't trigger the following steps in my first attempt)
https://github.com/juntyr/wasmtime/actions/runs/9722304795 is the publish artifacts run on the released branch (all succeed)
https://github.com/juntyr/wasmtime/actions/runs/9722304796 is the publish to crates.io run on the released branch (all succeed, of course this one is modified to only do dry runs)

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

juntyr · 2024-07-03T08:28:06Z

Is there any chance this PR could still be reviewed before the v23 cutoff?

tschneidereit · 2024-07-03T13:17:01Z

@juntyr Alex is on vacation this week, so the review will take a bit longer still. I can't really comment on what that'll do to the timing relative to the release, however

juntyr · 2024-07-03T13:19:38Z

@juntyr Alex is on vacation this week, so the review will take a bit longer still. I can't really comment on what that'll do to the timing relative to the release, however

Ok thanks - I hope Alex has a refreshing holiday :) I'll be on holiday myself for the two weeks after that, so perhaps we can finalize and merge the PR in the v24 cycle

alexcrichton

Ok thanks again for this! Additionally thanks for proving this out with some runs in your own repo, it's much appreciated! I'll backport this to 23.0.0 so we can see how the release goes while it's fresh in our heads

juntyr · 2024-07-08T18:51:02Z

@alexcrichton Thank you so much for your guidance and I'm excited to (hopefully) be released with v23 soon :D

…es [v2] (bytecodealliance#8874) * Add wasi adapter provider template which is materialised in CI * Add rustfmt component to adapter CI * Draft an extra publish step for the adapter provider * Check adapter provider in a separate step with adapter artifacts * Use artifact downloads in the publish action as well * Record results from adapter provider step as well * Refactor to use composite actions * Add missing shell property * Fix spelling mistake * Try using the env context

…er binaries (#8916) * wasi-adapter: Implement provider crate that embeds the adapter binaries [v2] (#8874) * Add wasi adapter provider template which is materialised in CI * Add rustfmt component to adapter CI * Draft an extra publish step for the adapter provider * Check adapter provider in a separate step with adapter artifacts * Use artifact downloads in the publish action as well * Record results from adapter provider step as well * Refactor to use composite actions * Add missing shell property * Fix spelling mistake * Try using the env context * Add release note --------- Co-authored-by: Juniper Tyree <50025784+juntyr@users.noreply.github.com>

Add wasi adapter provider template which is materialised in CI

7455066

juntyr requested review from a team as code owners June 26, 2024 11:47

juntyr requested review from alexcrichton and removed request for a team June 26, 2024 11:47

juntyr added 2 commits June 26, 2024 11:52

Add rustfmt component to adapter CI

7481af5

Draft an extra publish step for the adapter provider

a7ccb4e

alexcrichton reviewed Jun 26, 2024

View reviewed changes

juntyr added 3 commits June 27, 2024 06:55

Check adapter provider in a separate step with adapter artifacts

0d2d898

Use artifact downloads in the publish action as well

8a68e58

Record results from adapter provider step as well

fa0ad8f

alexcrichton reviewed Jun 27, 2024

View reviewed changes

juntyr added 3 commits June 28, 2024 13:03

Refactor to use composite actions

9820246

Add missing shell property

8e78664

Fix spelling mistake

9b7d9b5

This comment was marked as outdated.

Sign in to view

Try using the env context

825bd32

This comment was marked as outdated.

Sign in to view

juntyr requested a review from alexcrichton July 1, 2024 06:38

alexcrichton approved these changes Jul 8, 2024

View reviewed changes

alexcrichton added this pull request to the merge queue Jul 8, 2024

github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jul 8, 2024

alexcrichton added this pull request to the merge queue Jul 8, 2024

Merged via the queue into bytecodealliance:main with commit 83c8048 Jul 8, 2024

juntyr deleted the wasi-adapter-provider-v2 branch July 8, 2024 18:50

alexcrichton mentioned this pull request Jul 8, 2024

[23.0.0] wasi-adapter: Implement provider crate that embeds the adapter binaries #8916

Merged

Conversation

juntyr commented Jun 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexcrichton left a comment

Choose a reason for hiding this comment

Uh oh!

juntyr commented Jun 26, 2024

Uh oh!

juntyr commented Jun 27, 2024

Uh oh!

alexcrichton left a comment

Choose a reason for hiding this comment

Uh oh!

alexcrichton Jun 27, 2024

Choose a reason for hiding this comment

Uh oh!

alexcrichton Jun 27, 2024

Choose a reason for hiding this comment

Uh oh!

juntyr Jun 27, 2024

Choose a reason for hiding this comment

Uh oh!

alexcrichton Jun 27, 2024

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

juntyr commented Jun 29, 2024

Uh oh!

juntyr commented Jul 3, 2024

Uh oh!

tschneidereit commented Jul 3, 2024

Uh oh!

juntyr commented Jul 3, 2024

Uh oh!

alexcrichton left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

juntyr commented Jul 8, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

juntyr commented Jun 26, 2024 •

edited

Loading