juntyr requested wasmtime-core-reviewers for a review on PR #8874.
juntyr requested wasmtime-default-reviewers for a review on PR #8874.
juntyr requested alexcrichton for a review on PR #8874.
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
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
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
juntyr updated PR #8874.
juntyr updated PR #8874.
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
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 inbuild-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.
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 updated PR #8874.
juntyr updated PR #8874.
juntyr updated 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?
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:
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:
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 youAlso 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 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 theCHECK
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 :+1:
juntyr submitted PR review.
juntyr created PR review comment:
So this would be two new composite actions?
alexcrichton submitted PR review.
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
juntyr updated PR #8874.
juntyr updated PR #8874.
juntyr updated 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?
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)
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
juntyr updated 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
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
@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 :)
juntyr requested alexcrichton for a review on PR #8874.
Is there any chance this PR could still be reviewed before the v23 cutoff?
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
@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 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
alexcrichton merged PR #8874.
@alexcrichton Thank you so much for your guidance and I'm excited to (hopefully) be released with v23 soon :D
Last updated: Dec 23 2024 at 12:05 UTC