-
Notifications
You must be signed in to change notification settings - Fork 25
Reduce total CI duration #536
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
Open
elle-j
wants to merge
25
commits into
main
Choose a base branch
from
lj/speedup-ci
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+707
−521
Open
Changes from all commits
Commits
Show all changes
25 commits
Select commit
Hold shift + click to select a range
d9add2a
Refactor reusable-build into a per-build structure.
elle-j 7b8151a
Refactor differential-tests to use individual reusable builds.
elle-j 9819663
Refactor hardhat workflow to use individual reusable build.
elle-j c52f97f
Refactor test-wasm to use individual reusable build.
elle-j 2130579
Small refactor to alleviate testing of LLVM draft releases.
elle-j 0444b3a
Gate `RELEASE_RESOLC_WASM_URI` behind `is_release` input.
elle-j 5bf5860
Use treeless LLVM clone and require LLVM version detection.
elle-j 59b28a8
Update build job names for easier GH UI navigation.
elle-j 3328a8c
Skip unrelated workflows when tooling-projects change.
elle-j 79b28e7
Run hardhat tests only after required test job succeeds.
elle-j f6d3de1
Skip some workflow runs for doc-only updates.
elle-j a91135f
Remove paths-ignore from required test job.
elle-j 641b427
Skip some workflow runs for package.json updates.
elle-j 3d12278
Skip some workflow runs for package.json updates.
elle-j 7d5d996
Update revive-differential-tests ref.
elle-j 8964c31
Merge branch 'main' into lj/speedup-ci
elle-j 719afc5
Update e2e timeout-minutes.
elle-j 026bfbd
Trigger workflow rerun.
elle-j 18301db
Update revive-differential-tests ref.
elle-j a68b3c6
Upgrade actions using Node 20.
elle-j a0ede88
Revert version upgrade on action in test-llvm-builder.
elle-j 558855e
Revert automatic double quote replacement in test-llvm-builder.
elle-j 0c82252
Update node-version in test-wasm.
elle-j 39885ec
Capture Playwright traces on test-wasm failures.
elle-j f48782f
Trigger workflow rerun.
elle-j File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,86 @@ | ||
| name: Build resolc | ||
| description: Cache-aware resolc build for a single target. | ||
|
|
||
| inputs: | ||
| target: | ||
| description: "Target triple (e.g., x86_64-unknown-linux-musl)" | ||
| required: true | ||
| build-type: | ||
| description: "'native' or 'musl'" | ||
| required: true | ||
| retention-days: | ||
| description: "Artifact retention in days" | ||
| required: false | ||
| default: "1" | ||
|
|
||
| outputs: | ||
| url: | ||
| description: "Uploaded artifact URL" | ||
| value: ${{ steps.upload.outputs.artifact-url }} | ||
| sha: | ||
| description: "Uploaded artifact digest" | ||
| value: ${{ steps.upload.outputs.artifact-digest }} | ||
|
|
||
| runs: | ||
| using: composite | ||
| steps: | ||
| - name: Check Build Cache | ||
| id: cache | ||
| uses: actions/cache@v5 | ||
| with: | ||
| # Use glob to match binaries with and without extensions. | ||
| path: resolc-${{ inputs.target }}* | ||
| key: build-${{ inputs.target }}-${{ github.sha }} | ||
|
|
||
| - name: Set Up Rust Toolchain | ||
| if: ${{ steps.cache.outputs.cache-hit != 'true' }} | ||
| uses: actions-rust-lang/setup-rust-toolchain@v1 | ||
| with: | ||
| rustflags: "" | ||
| cache-key: ${{ inputs.target }} | ||
|
|
||
| - name: Download LLVM | ||
| if: ${{ steps.cache.outputs.cache-hit != 'true' }} | ||
| uses: ./.github/actions/get-llvm | ||
| with: | ||
| target: ${{ inputs.target }} | ||
|
|
||
| - name: Build (Native) | ||
| if: ${{ steps.cache.outputs.cache-hit != 'true' && inputs.build-type == 'native' }} | ||
| shell: bash | ||
| run: | | ||
| export LLVM_SYS_221_PREFIX=$PWD/llvm-${{ inputs.target }} | ||
| make install-bin | ||
| mv target/release/resolc resolc-${{ inputs.target }} || mv target/release/resolc.exe resolc-${{ inputs.target }}.exe | ||
|
|
||
| - name: Build (MUSL) | ||
| if: ${{ steps.cache.outputs.cache-hit != 'true' && inputs.build-type == 'musl' }} | ||
| shell: bash | ||
| run: | | ||
| docker run -v $PWD:/opt/revive $RUST_MUSL_CROSS_IMAGE /bin/bash -c " | ||
| cd /opt/revive | ||
| chown -R root:root . | ||
| apt update && apt upgrade -y && apt install -y pkg-config | ||
| export LLVM_SYS_221_PREFIX=/opt/revive/llvm-${{ inputs.target }} | ||
| make install-bin | ||
| mv target/${{ inputs.target }}/release/resolc resolc-${{ inputs.target }} | ||
| " | ||
| sudo chown -R $(id -u):$(id -g) . | ||
|
|
||
| - name: Install Solc | ||
| uses: ./.github/actions/get-solc | ||
|
|
||
| - name: Basic Sanity Check | ||
| shell: bash | ||
| run: | | ||
| result=$(./resolc-${{ inputs.target }} --bin crates/integration/contracts/flipper.sol) | ||
| echo $result | ||
| if [[ $result == *'50564d'* ]]; then exit 0; else exit 1; fi | ||
|
|
||
| - name: Upload Artifact | ||
| id: upload | ||
| uses: actions/upload-artifact@v6 | ||
| with: | ||
| name: resolc-${{ inputs.target }} | ||
| path: resolc-${{ inputs.target }}* | ||
| retention-days: ${{ inputs.retention-days }} | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer to use our Dockerfile. Currently a two stage build yielding an alpine image including solc, so maybe we want to split the Dockerfile up.
Or what would be even better, split this action up into build-resolc-native and build-resolc-musl. build-musl just tries to build the Dockerfile and sanity check by using the built image to build a contract. This would build the native and musl linux builds in parallel and also ensure the Dockerfile works on CI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea on testing the Dockerfile! Regarding native vs musl linux, I think we should still build the musl here though, because that's the binary we distribute so we'd be more confident if that's the build we're using for differential tests etc.
I looked at ways we could refactor the Dockerfile such that it can still be used as it is today (no semantic change), or optionally skip e.g. the LLVM stage if a prebuilt is passed. This way the Dockerfile will always be used for building the linux musl build and can also use the published LLVM.
The full Dockerfile can be tested via a separate workflow (running in parallel). Since building LLVM will take a while, we could e.g. run that on pushes to
mainonly.(If we go with the Dockerfile refactor, it may be better to add it as a follow-up PR to keep that change more isolated, and keep the preexisting build logic for this PR.)
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dumb question: Why do we build the linux release target here at all? We should just be using the musl build on linux anyways from here on.
(I'm not saying we don't build the MUSL here, but instead of an inlined script that builds the MUSL binary inside the container, just build the Dockerfile, which is the exact same thing)
I don't think the Dockerfile needs a change. I think either we should extract the binary from the image or if thats too cumbersome, copy out the binary via running the image as the inline script does currently. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're already only building the musl build. The "Build (Native)" step has only been used for Windows and macOS.
Yeah I definitely agree that we should just build the Dockerfile and use the binary that it creates, but if it's built as-is that'd mean this is also unnecessarily run:
What I meant is we could pass the LLVM we get from the get-llvm action into the Dockerfile to skip those two
RUNlines. Otherwise, running the Dockerfile as-is would add the LLVM building overhead.And we can still have another workflow exercising the entire Dockerfile, including building of LLVM, that could be skipped on PRs.
Let me know if this makes sense or if I may be missing something obvious here.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, I fired way too quickly. It just looked to me like we build native too. Good call, this would add a lot of unnecessary overhead, I didn't consider it!
Yeah I like your idea. IIRC you can
COPY --from=imagename, so should work across separate docker build invocations too. This would allow to factor it out into 2 separate nice and small Dockerfiles (e.g. Dockerfile.llvm, Dockerfile.resolc) and then for this purpose we just mount in the musl LLVM build obtained from the get-llvm action. Maybe add adockerphony target to the Makefile which runs the two docker build commands, this kinda serves as documentation / shortcut for people that just want to build a portable docker image.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good, created an issue for it here to have a focused PR for it 👍