Stream: git-wasmtime

Topic: wasmtime / PR #8874 wasi-adapter: Implement provider crat...

Wasmtime GitHub notifications bot (Jun 26 2024 at 11:47):

juntyr requested wasmtime-core-reviewers for a review on PR #8874.

Wasmtime GitHub notifications bot (Jun 26 2024 at 11:47):

juntyr requested wasmtime-default-reviewers for a review on PR #8874.

Wasmtime GitHub notifications bot (Jun 26 2024 at 11:47):

juntyr requested alexcrichton for a review on PR #8874.

Wasmtime GitHub notifications bot (Jun 26 2024 at 11:47):

juntyr opened PR #8874 from juntyr:wasi-adapter-provider-v2 to bytecodealliance:main:

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.

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

Wasmtime GitHub notifications bot (Jun 26 2024 at 11:47):

juntyr edited PR #8874:

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.

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

Wasmtime GitHub notifications bot (Jun 26 2024 at 11:47):

juntyr edited PR #8874:

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.

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

Wasmtime GitHub notifications bot (Jun 26 2024 at 11:52):

juntyr updated PR #8874.

Wasmtime GitHub notifications bot (Jun 26 2024 at 12:31):

juntyr updated PR #8874.

Wasmtime GitHub notifications bot (Jun 26 2024 at 13:07):

juntyr edited PR #8874:

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.

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

Wasmtime GitHub notifications bot (Jun 26 2024 at 18:54):

alexcrichton submitted PR review:

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.

Wasmtime GitHub notifications bot (Jun 26 2024 at 19:43):

juntyr commented on PR #8874:

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)

Wasmtime GitHub notifications bot (Jun 27 2024 at 06:55):

juntyr updated PR #8874.

Wasmtime GitHub notifications bot (Jun 27 2024 at 07:07):

juntyr updated PR #8874.

Wasmtime GitHub notifications bot (Jun 27 2024 at 08:20):

juntyr updated PR #8874.

Wasmtime GitHub notifications bot (Jun 27 2024 at 12:23):

juntyr commented on PR #8874:

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

Wasmtime GitHub notifications bot (Jun 27 2024 at 14:00):

alexcrichton submitted PR review:

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 :+1:

Wasmtime GitHub notifications bot (Jun 27 2024 at 14:00):

alexcrichton submitted PR review:

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 :+1:

Wasmtime GitHub notifications bot (Jun 27 2024 at 14:00):

alexcrichton created PR review comment:

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/*

Wasmtime GitHub notifications bot (Jun 27 2024 at 14:00):

alexcrichton created PR review comment:

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:

1. Run the composite action to figure out the run-id
2. Run the composite action to setup the artifacts and perform checks
3. Run a publish

That feels like a good state to me -- aka I like this refactoring :+1:

Wasmtime GitHub notifications bot (Jun 27 2024 at 14:04):

juntyr submitted PR review.

Wasmtime GitHub notifications bot (Jun 27 2024 at 14:04):

juntyr created PR review comment:

So this would be two new composite actions?

Wasmtime GitHub notifications bot (Jun 27 2024 at 14:08):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jun 27 2024 at 14:08):

alexcrichton created PR review comment:

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

Wasmtime GitHub notifications bot (Jun 28 2024 at 13:03):

juntyr updated PR #8874.

Wasmtime GitHub notifications bot (Jun 28 2024 at 13:09):

juntyr updated PR #8874.

Wasmtime GitHub notifications bot (Jun 28 2024 at 15:53):

juntyr updated PR #8874.

Wasmtime GitHub notifications bot (Jun 28 2024 at 16:10):

juntyr commented on PR #8874:

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

Wasmtime GitHub notifications bot (Jun 28 2024 at 16:39):

juntyr commented on PR #8874:

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)

Wasmtime GitHub notifications bot (Jun 28 2024 at 16:40):

juntyr edited a comment on PR #8874:

[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

Wasmtime GitHub notifications bot (Jun 28 2024 at 17:38):

juntyr updated PR #8874.

Wasmtime GitHub notifications bot (Jun 28 2024 at 18:12):

juntyr commented on PR #8874:

I feel like GitHub has started to rate limit my repo as the CI action just no longer even starts, 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

Wasmtime GitHub notifications bot (Jun 28 2024 at 18:13):

juntyr edited a comment on PR #8874:

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

Wasmtime GitHub notifications bot (Jun 29 2024 at 07:19):

juntyr commented on PR #8874:

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

• https://github.com/juntyr/wasmtime/pull/22 is the PR which includes this PR and the extra commit https://github.com/juntyr/wasmtime/pull/22/commits/0f9c4516adabc9d65cef0da7d45699a86044f260 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)
• https://github.com/juntyr/wasmtime/pull/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)
• https://github.com/juntyr/wasmtime/pull/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 :)

Wasmtime GitHub notifications bot (Jul 01 2024 at 06:38):

juntyr requested alexcrichton for a review on PR #8874.

Wasmtime GitHub notifications bot (Jul 03 2024 at 08:28):

juntyr commented on PR #8874:

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

Wasmtime GitHub notifications bot (Jul 03 2024 at 13:17):

tschneidereit commented on PR #8874:

@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

Wasmtime GitHub notifications bot (Jul 03 2024 at 13:19):

juntyr commented on PR #8874:

@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

Wasmtime GitHub notifications bot (Jul 08 2024 at 18:09):

alexcrichton submitted PR review:

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

Wasmtime GitHub notifications bot (Jul 08 2024 at 18:43):

alexcrichton merged PR #8874.

Wasmtime GitHub notifications bot (Jul 08 2024 at 18:51):

juntyr commented on PR #8874:

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

Last updated: Oct 23 2024 at 20:03 UTC