feat: add action to upload pre-compiled artifacts#3029
feat: add action to upload pre-compiled artifacts#3029lima-limon-inc wants to merge 6 commits into0xMiden:nextfrom
Conversation
There was a problem hiding this comment.
There are actually two artifacts that we want to produce from this workflow, as part of publishing a release:
- The
miden-vmexecutable, which this PR does currently - The
core.masppackage file, produced by the build script of themiden-core-libcrate. We may need a dedicated Makefile task for this though, since there isn't an easy way to locate the build script output directory without capturing the verbose output ofcargo buildin JSON form. We could either modify the build script to output the.maspto bothOUT_DIRandCARGO_TARGET_DIR/core.masp(so that we have a predictable location to obtain it from), or write acargoscript that links againstmiden-core-liband writes the embedded package bytes to disk somewhere. Either way, we'll need that artifact as well.
There was a problem hiding this comment.
Noted! I've both rebased and added miden-core.masp with the latest commits. I've also updated the description with the rationale.
0591c87 to
579ca8b
Compare
579ca8b to
545b461
Compare
1abc793 to
9a154ab
Compare
|
Automated check (CONTRIBUTING.md) Findings:
Recommendations:
Next steps:
|
9a154ab to
f4af25a
Compare
huitseeker
left a comment
There was a problem hiding this comment.
The release job needs to prove the tag is a real stable release from main, because otherwise it can publish binaries that midenup treats as trusted release assets. The guard has to run before publish or upload because release.published accepts any published release.
This requires an explicit guard before upload/publish, for example checking github.event.release.prerelease == false, tag name matches ^v?[0-9]+\.[0-9]+\.[0-9]+$, and/or git rev-parse "$RELEASE_TAG^{commit}" == git rev-parse origin/main.
| uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 | ||
| with: | ||
| fetch-depth: 0 | ||
| ref: ${{ github.event.release.tag_name }} |
There was a problem hiding this comment.
Could this job be gated on publish and verify that the release tag is the same revision being published?
GitHub starts jobs in parallel by default, and this job builds from ${{ github.event.release.tag_name }} while publish checks out main. If publishing fails, or if the tag points at a different commit, the release page can still get assets that midenup will treat as matching the crate release.
There was a problem hiding this comment.
In retrospect, shouldn't publish checkout the release tag name as well? When a release is created with a specific tag, we want that tag to be bundled as a release, right? Why would we checkout main?
Checking mainout introduces two potential problems if I'm not mistaken:
- Like you mentioned, the
mainbranch and the commit corresponding to the released tag might differ - Doesn't this introduce a sort of race condition? What if there's a commit on the main branch right before the publish action run? That commit would be
cargo published; yet it was not part of the release.
If I'm not mistaken, the miden-client check the tag out instead: https://github.com/0xMiden/miden-client/blob/e663d0d88a9adf04d36b1f72060e0599fa265369/.github/workflows/publish-crates-release.yml#L20-L25 (this got changed on this PR 0xMiden/miden-client#1862)
I committed this approach here: ff461ca; but do let me know if you want it reverted.
There was a problem hiding this comment.
Yes, I think you’re right.
The release tag should be the source of truth for both jobs. Checking out main can publish a commit that is not the tagged release, especially if main moves after the release is created.
So ff461ca looks right to me. I’d keep upload-artifacts behind needs: [publish], and keep the existing verify-main-head: "true" check so the tag still has to match origin/main before anything is published.
|
#2881 is about to be merged - when you rebase this PR on |
| - name: Build miden-core.masp | ||
| if: matrix.os == 'ubuntu-latest' && matrix.target == 'x86_64-unknown-linux-gnu' | ||
| run: | | ||
| make packages |
There was a problem hiding this comment.
This job uses make packages, which runs cargo +nightly -Zscript, but this workflow only updates the default toolchain.
On a clean runner without nightly, this step looks like it would fail before miden-core.masp is uploaded.
Should we add an install of nightly here, like the other workflows that run cargo +nightly?
| uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 | ||
| with: | ||
| fetch-depth: 0 | ||
| ref: ${{ github.event.release.tag_name }} |
There was a problem hiding this comment.
Yes, I think you’re right.
The release tag should be the source of truth for both jobs. Checking out main can publish a commit that is not the tagged release, especially if main moves after the release is created.
So ff461ca looks right to me. I’d keep upload-artifacts behind needs: [publish], and keep the existing verify-main-head: "true" check so the tag still has to match origin/main before anything is published.
Closes 0xMiden/midenup#179
This PR adds a step to the VM's CI which, after a release is made, adds pre-compiled artifacts to the corresponding Github release page. Currently entailing:
miden-vmCLI binarymiden-core.maspPackageThese are then used by
midenupin order to speed installs up.Attribution note: This added step was taken from the compiler's CI and adapted for the miden-vm.
Follows up on 0xMiden/miden-client#1936
Implementation notes:
As mentioned by @bitwalker in this comment, the
.masppackage is being built on themiden-core-libcrate's build script. However,CARGO_TARGET_DIRis not exposed to build scripts and writing files outside ofOUT_DIRis explicitly discouraged by the docs.There is an ongoing (and even active) conversation on the rust-lang repository regarding saving files outside the
OUT_DIRwhich were generated on a build_script (like rust-lang/cargo#2729, rust-lang/cargo#13663, rust-lang/cargo#9661, amongst others). However, since it is currently actively discouraged, I went with the cargo script instead.As a bonus,
cargo scriptstabilization, despite beingnightlyonly currently seems to be approaching stabilization soon-ish (see rust-lang/cargo#16569).