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

chore: Use cargo-chef to build the hivetests docker image #14884

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

janjakubnanista
Copy link

@janjakubnanista janjakubnanista commented Mar 6, 2025

Ref #14777

The docker build for the hive tests is now a bit lacking - it builds the binary outside the container, then just copies it in. Besides fragility, this also means longer buildtimes since the build is not utilizing cargo-chef.

This PR modifies the prepare-reth workflow and the hive tests Dockerfile to improve the speed and stability of the CI builds:

  • Update .github/assets/hive/Dockerfile to use cargo-chef
  • Exclude .git and dist from being copied along with the source code where not needed - this effectively breaks the docker layer caching. The --exclude flag is still in stable Dockerfile syntax and needs to be enabled. See the docs
  • Drop the local rust cache since we're now building in docker

The caching has been tested in the forked repository (on ubuntu-latest so the times are not directly translatable):

The runs were performed on debug branch that only differs from main in the fact that it uses ubuntu-latest and removes the condition if: github.repository == 'paradigmxyz/reth'

@fgimenez
Copy link
Member

fgimenez commented Mar 6, 2025

@janjakubnanista thanks for your contribution! improving speed and stability sounds very good :)

i've forked this branch to a branch in this repo to execute the hive workflow with these changes. i get this error: https://github.com/paradigmxyz/reth/actions/runs/13705636252/job/38330111508. can you please take a look?

@janjakubnanista
Copy link
Author

Hey @fgimenez! Thanks for that - I debugged the issue in my fork and all should be good now, already for a review. The cached runs seem super fast (see PR description)

@emhane emhane requested a review from fgimenez March 7, 2025 05:45
Copy link
Member

@emhane emhane left a comment

Choose a reason for hiding this comment

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

amazing

@janjakubnanista
Copy link
Author

@emhane or @fgimenez - i see some lints failed, some vulnerabilities in the lockfile - suspicious since i have not touched the lockfile. Any ideas?

@emhane
Copy link
Member

emhane commented Mar 7, 2025

@emhane or @fgimenez - i see some lints failed, some vulnerabilities in the lockfile - suspicious since i have not touched the lockfile. Any ideas?

merge in latest main might fix it

Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

ty!

pending @fgimenez

failing ci unrelated #14889

@fgimenez
Copy link
Member

fgimenez commented Mar 7, 2025

@janjakubnanista thx! the build succeeds now, but it takes much longer than before, this build is with the changes in this branch https://github.com/paradigmxyz/reth/actions/runs/13717164093, building the hivetests image took 19m23s, and this is the last regular build executed last night https://github.com/paradigmxyz/reth/actions/runs/13710819568 building the image took 8m46s, is this expected?

@emhane
Copy link
Member

emhane commented Mar 7, 2025

@janjakubnanista thx! the build succeeds now, but it takes much longer than before, this build is with the changes in this branch https://github.com/paradigmxyz/reth/actions/runs/13717164093, building the hivetests image took 19m23s, and this is the last regular build executed last night https://github.com/paradigmxyz/reth/actions/runs/13710819568 building the image took 8m46s, is this expected?

could this be because we are building loads of other branches in-between the hive tests, that don't make use of cargo chef? probs right way to test is running several hive jobs right after each other on that branch @fgimenez

@emhane
Copy link
Member

emhane commented Mar 7, 2025

yepp, just re-ran hive and reth built in 37s !! https://github.com/paradigmxyz/reth/actions/runs/13718282056

@fgimenez
Copy link
Member

fgimenez commented Mar 7, 2025

yepp, just re-ran hive and reth built in 37s !! https://github.com/paradigmxyz/reth/actions/runs/13718282056

awesome! maybe the first time it needs to build the cache. makes me wonder, if we run again from main, after some commits have been merged, will it need to rebuild the cache?

we can test @janjakubnanista if you rebase on top of current main or merge main CI would pass, and we can retrigger hive from a different commit

@fgimenez
Copy link
Member

fgimenez commented Mar 7, 2025

we can test @janjakubnanista if you rebase on top of current main or merge main CI would pass, and we can retrigger hive from a different commit

I've rebased the branch forked from this one on top of current main and run hive twice, the first one building the image took 23m https://github.com/paradigmxyz/reth/actions/runs/13726539897 and the second one 39s https://github.com/paradigmxyz/reth/actions/runs/13727275178

@mattsse
Copy link
Collaborator

mattsse commented Mar 8, 2025

failing lint tests are unrelated, good to merge?

@emhane emhane enabled auto-merge March 10, 2025 10:21
@janjakubnanista
Copy link
Author

@mattsse @emhane should i rebase on the newest main you think? to get rid of those red lints

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

Successfully merging this pull request may close these issues.

4 participants