Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Release workflow #201

Merged
merged 30 commits into from
Feb 17, 2025
Merged

Release workflow #201

merged 30 commits into from
Feb 17, 2025

Conversation

AndWeHaveAPlan
Copy link
Contributor

@AndWeHaveAPlan AndWeHaveAPlan commented Feb 10, 2025

Automatically create tag and relese based on 'version' in cargo.toml, triggered on main branch only. Do nothing if tag and relese already exist
Example

Release artifacts:

  • llvm-linux-emscripten.tar.gz
  • llvm-linux-gnu.tar.gz
  • resolc
  • resolc.js
  • resolc.wasm

Current issues:

  • If build and upload fail, tag and release must be manualy deleted to trigger new build for the same tag
  • No tests performed for release
  • No Win and MacOS build

Next steps:

  • Workflow refactoring
  • Build other platforms

@AndWeHaveAPlan AndWeHaveAPlan changed the title llvm+resolc Release workflow Feb 10, 2025
@xermicus
Copy link
Member

Can we please test the release workflows in a fork of this repository?

@xermicus
Copy link
Member

Note: The test failure is caused by a change in go-ethereum, I'm on it.

@AndWeHaveAPlan
Copy link
Contributor Author

AndWeHaveAPlan commented Feb 10, 2025

Can we please test the release workflows in a fork of this repository?

Ok, I've forked the repo to
https://github.com/paritytech/revive-worflow-test/actions/runs/13241577827

@AndWeHaveAPlan
Copy link
Contributor Author

Works in fork

v0.1.0-dev.10 "release"

@athei
Copy link
Member

athei commented Feb 10, 2025

Didn't we agree to have the LLVM release separate? Meaning it is triggered manually so that it doesn't have to be rebuild for every release.

@xermicus
Copy link
Member

That was the idea at first, however not doing this simplifies the release process. It also eases changing LLVM compilation settings between releases.

@AndWeHaveAPlan
Copy link
Contributor Author

Didn't we agree to have the LLVM release separate? Meaning it is triggered manually so that it doesn't have to be rebuild for every release.

This is more of a temporary solution and will change (if needed), especially with addition of the win/mac build. For now we use actions/cache with hashfiles(...) to avoid full rebuild

@athei
Copy link
Member

athei commented Feb 10, 2025

I see. Thanks for clarifying.

@xermicus
Copy link
Member

xermicus commented Feb 10, 2025

v0.1.0-dev.10 "release"

I just checked the linux binary, it isn't the statically linked MUSL build? Also the workflow does not indicate at all that it will be building against MUSL. For Linux we want the musl build (only). Easiest way is to use musl-cross, see the Dockerfile in this repository (you can essentially just build this image and then copy the bianry from a dummy container afterwards).

Note: The gnu LLVM is fine to release though. LLVM builds we want to release both: MUSL for reproducibility and GNU so that contributors with slow laptops don't have to wait for an hour.

@xermicus xermicus mentioned this pull request Feb 11, 2025
@athei
Copy link
Member

athei commented Feb 12, 2025

Will the rest I asked for also be removed?

@AndWeHaveAPlan
Copy link
Contributor Author

Ok, lets summarize everything we have

Note: The gnu LLVM is fine to release though. LLVM builds we want to release both: MUSL for reproducibility and GNU so that contributors with slow laptops don't have to wait for an hour.

The LLVM artifacts should not be released. It is statically linked into the the other artifacts and there is no reason for people to download them. If they are released at all it will be in a separate workflow just to be downloaded by this very workflow.

So we need to release both LLVM, as mentioned here

  • linux-gnu (?)
  • macos-gnu
  • windows-gnu
  • emscripten
  • musl

and revive

From The final goal is to have two workflows:

  • build and publish LLVM as a separate tag and release (like llvm-*). Triggered manually
  • build and publish revive as a "true" release (like v1.2.3). Triggered for main branch by version change in cargo.toml and use llvm from the latest llvm-* release

This is a 'step 1' - 'minimal' workflow, so we can have at least some form of release for now. The LLVM release workflow is in a separate pr, but there are problems with the win build

Does everyone agree with this, or do we have a different point of view?


So I propose next steps:

  • Step 2: two separate releases, but only linux-musl for now
  • Step 3: add macos
  • Step 4: add windows

@athei
Copy link
Member

athei commented Feb 12, 2025

So we need to release both LLVM, #93 (comment)

Can you scroll down a bit? Just the next post in this thread I wrote why this list of targets does not make sense. But to recap. We want those AND ONLY THOSE releases of the resolc binary:

linux-musl
macOS (universal)
windows-msvc
emscripten

So I propose next steps:

I propose you start by fixing the trivial suggestions I made here: #201 (review)

  • Remove all gnu builds. Not only the artifact.
  • Don't release any LLVM artifacts from this workflow.

I don't know how to express this in any simpler terms.

@athei
Copy link
Member

athei commented Feb 13, 2025

Please let me know when I should give it another review. Let's merge it without mac und windows for now so we have at least that.

@KarimJedda
Copy link

@AndWeHaveAPlan can you please add a few more comments and address Alex's questions? Pushing changes is great, discussing & explaining them is even better! Thank you :)

@AndWeHaveAPlan
Copy link
Contributor Author

fork example for current workflow

  • without gnu
  • without llvm artifacts
  • with new resolc_web.js file

@AndWeHaveAPlan AndWeHaveAPlan requested a review from athei February 14, 2025 08:41
@mutantcornholio
Copy link
Contributor

Just to sync discussions between parallel streams: LLVM artifacts will be released in separate tags (the part I'm working on), to later be reused in this release workflow, as well as in test workflow.
#207 (comment)
#207 (comment)

Copy link
Member

@athei athei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks this looks like what we want :)

My main grumble here was that it is unclear when the author is still working on it and when it is considered "done" from their point of view. So I can start reviewing.

Copy link
Member

@xermicus xermicus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like we are getting close, thanks!

Can you please update the RELEASE.md file to reflect how the release works with the new workflow?

Comment on lines 163 to 168
- name: llvm-musl-cache save
if: steps.llvm-musl-cache.outputs.cache-hit != 'true'
uses: actions/cache/save@v4
with:
path: target-llvm/musl/target-final
key: llvm-linux-musl-${{ hashFiles('crates/solidity/**') }}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we cache the musl artifacts directly after building it? Otherwise it's either both musl and emscripten work and get cached or nothing gets cached.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, reordered

- name: build musl
run: |
mkdir resolc-out
docker run -v $PWD:/opt/revive messense/rust-musl-cross:x86_64-musl /bin/bash -c "
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately it doesn't look like they version it properly with tags. Can you please change this to use the image digest? This gives us manual control and will avoid breaking the release workflow randomly.

Copy link
Contributor Author

@AndWeHaveAPlan AndWeHaveAPlan Feb 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, set @sha256:68b86bc7cb2867259e6b233415a665ff4469c28b57763e78c3bfea1c68091561, image used in fork workflow

Comment on lines 181 to 185
- name: download solc
run: |
curl -o solc https://github.com/ethereum/solidity/releases/download/v0.8.28/solc-static-linux
chmod +x solc
echo "$PWD" >> $GITHUB_PATH
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is technically not required for builds.

However, executing a basic tests using the release build, like compiling a flipper contract, would be good. Can you please add such a test? It can be really simple, i.e. running resolc --bin crates/integration/contracts/flipper.sol, expecting the PVM magic bytes 50564d in the output and expecting a return value of 0. Same for the Wasm build. It's unlikely but should the release workflow for some reason produce defective artifacts, without the test, we will only find it out the annoying way.

@AndWeHaveAPlan
Copy link
Contributor Author

update:

  • simple resolc checks as proposed by @xermicus
  • fixed messense/rust-musl-cross image version
  • rearranged cache steps
  • tag and release are now created after build
  • updated README.md
  • release description extracted from README.md

fork examples (10, 11, 12)

Copy link
Member

@xermicus xermicus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good thanks! Just some nits then we can merge.

Copy link
Member

@xermicus xermicus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Can you please trigger it again in the fork to be sure that it works with ubuntu 24.04 runners? Once this runs through I'll merge.

@AndWeHaveAPlan
Copy link
Contributor Author

Thanks! Can you please trigger it again in the fork to be sure that it works with ubuntu 24.04 runners? Once this runs through I'll merge.

just finished

@xermicus xermicus merged commit c221044 into main Feb 17, 2025
6 checks passed
@xermicus xermicus deleted the es/release-revive branch February 17, 2025 10:09
@xermicus
Copy link
Member

xermicus commented Feb 18, 2025

@AndWeHaveAPlan I tried the release but it failed, can you please take a look and fix it ASAP?

@AndWeHaveAPlan
Copy link
Contributor Author

problem with actions permissions, fixed by repo settings for now
https://github.com/paritytech/revive/releases/tag/untagged-eddd2fa6b2a2959fe9a2

@xermicus
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants