-
Notifications
You must be signed in to change notification settings - Fork 931
feat: Add reproducible builds release workflows and push images to DockerHub #7614
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
base: unstable
Are you sure you want to change the base?
Conversation
|
Ubuntu seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
82dcfef to
238fbaa
Compare
|
Doing some testing on this, will post the comment when ready |
|
Some required checks have failed. Could you please take a look @MoeMahhouk? 🙏 |
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.
Thanks for the PR. I left some comments after doing some testing
Thanks for reviewing it. |
Is there a reason why is this PR still a draft? |
Not really, I am waiting for your final feedback to open it for review/merge. |
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 have tested this and it is working. Would be great to get some comments from the devs
I have added a label test-reproducible, that will trigger the reproducible builds CI to run. When we have release PR, we can add this label
to run the check and ensure that the binaries are reproducible.
| TZ_VAL = UTC | ||
|
|
||
| # Default features for lighthouse | ||
| FEATURES ?= gnosis,slasher-lmdb,slasher-mdbx,slasher-redb,jemalloc |
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 think this breaks the cli-check due to this: chong-he@824a5dc
I think we still support mdbx but it's going to be removed some time? We will wait for the devs to comment on this
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.
what do you suggest to change it into? I am open for any suggestions that suits the purpose
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 will wait for a review from @michaelsproul when he has the time
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.
jemalloc is now the default
| FEATURES ?= gnosis,slasher-lmdb,slasher-mdbx,slasher-redb,jemalloc | |
| FEATURES ?= gnosis,slasher-lmdb,slasher-mdbx,slasher-redb |
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.
For simplicity, we could probably use the CROSS_FEATURES that's already defined in the Makefile? or is there something in CROSS_FEATURES that make the reproducible invalid?
| jobs: | ||
| extract-version: | ||
| name: extract version | ||
| runs-on: ubuntu-latest |
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.
Should we stick to ubuntu-22.04 here? The docker.yml is also running on this now:
lighthouse/.github/workflows/docker.yml
Line 29 in 999b045
| runs-on: ubuntu-22.04 |
I somehow recall that we usually go with the previous version of OS for maximal compatibility
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 have no strong opinion about this. I will defer this decision to you
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.
For consistency, let's use Ubuntu 22.04 here to be the same with docker.yml file
| build-aarch64: | ||
| name: test reproducible builds (aarch64) | ||
| runs-on: ubuntu-24.04-arm |
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.
Similar comment as above, wondering should we use ubuntu-22.04-arm instead?
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.
same as my answer above
|
@chong-he , I just wanted to double check if this is waiting on me for anything else so far? |
No The PR is now waiting for other team members to review |
|
Hi @MoeMahhouk, this pull request has been closed automatically due to 30 days of inactivity. If you’d like to continue working on it, feel free to reopen at any time. |
|
Re-opening this PR as it was awaiting review. |
|
I like this idea but would like it to be less maintenance-intensive. |
good idea, could you provide an example on that suggestion? do you have a reference link or would you like to take that up and refine it to be a parameter instead? |
|
Hi @MoeMahhouk, this pull request has been closed automatically due to 30 days of inactivity. If you’d like to continue working on it, feel free to reopen at any time. |
|
Hey folks, @chong-he @jimmygchen ,this PR got auto-closed due to inactivity, but the changes are current and needs review. Could we reopen this for another pass? |
Reopening, could you solve some conflicts in the branch? |
solved the conflicts and merged. Let me know if you want me to do any other change |
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.
Look at this a second time and try to suggest some simplifications so that it is easier to maintain and make the code cleaner.
Also, I would prefer to not have the symbol (green tick, cross etc) to keep the code clean.
| TZ_VAL = UTC | ||
|
|
||
| # Default features for lighthouse | ||
| FEATURES ?= gnosis,slasher-lmdb,slasher-mdbx,slasher-redb,jemalloc |
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.
jemalloc is now the default
| FEATURES ?= gnosis,slasher-lmdb,slasher-mdbx,slasher-redb,jemalloc | |
| FEATURES ?= gnosis,slasher-lmdb,slasher-mdbx,slasher-redb |
| echo "=== Building first Docker image (x86_64) ===" | ||
| docker build -f Dockerfile.reproducible \ | ||
| --build-arg RUST_TARGET="x86_64-unknown-linux-gnu" \ | ||
| --build-arg RUST_IMAGE="rust:1.88-bullseye@sha256:8e3c421122bf4cd3b2a866af41a4dd52d87ad9e315fd2cb5100e87a7187a9816" \ |
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 think we can remove this line and instead just let it read from Dockerfile.reproducible for the RUST_IMAGE. I have tested it on my local repo and it works, see: https://github.com/chong-he/lighthouse/pull/9/files#diff-587298ff141278ce3be7c54a559f9f31472cc5b384e285e2105b3dee319ba31d
This also applies to all other instances that uses this RUST_IMAGE. Taking the idea from Dockerfile:
Line 1 in e3ee7fe
| FROM rust:1.88.0-bullseye AS builder |
We can also remove the sha256 in the RUST_IMAGE in Dockerfile.reproducible. That will make it much easier to maintain.
We also do not need a separate image on x86_64 or arm64 from my understanding, as docker is able to select the image based on the host: https://docs.docker.com/build/building/multi-platform/#difference-between-single-platform-and-multi-platform-images, so that will make things simpler
| --build-arg RUST_IMAGE="rust:1.88-bullseye@sha256:8e3c421122bf4cd3b2a866af41a4dd52d87ad9e315fd2cb5100e87a7187a9816" \ |
| exit 1 | ||
| fi | ||
| summary: |
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 would like to suggest to remove this part to make things simpler.
I think it is sufficient to have 2 additional CIs for reproducible, one for x86_64, another one for arm64. If both passes then good, if any is failing, then we can look at the CI itself, so this summary job is not necessary imho
| @@ -0,0 +1,253 @@ | |||
| name: release-reproducible | |||
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 would suggest to name this as docker-reproducible and also the same for the file name, so make it distinct right from the name
| echo "🔄 Verifying reproducible builds for ${{ matrix.arch }}..." | ||
| # Build first image | ||
| echo "=== Building first verification 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.
I know I raised this before to have a reproducible check before publishing the image. I am sorry, thinking about it now, we already have the reproducible-CI checks, and this seems repetitive now. In the interest of making things simpler, should we remove the check? I am trying to make the changes as minimal as possible, so that it is easier to read and maintain. After all, the checks for reproducibility is running every 2 days so if anything, we can catch it earlier.
I am just considering about the consequence - if without the checks and the docker images are pushed, then it could be that the docker images are not reproducible in the worse outcome?
| FROM gcr.io/distroless/cc-debian12:nonroot-6755e21ccd99ddead6edc8106ba03888cbeed41a | ||
| COPY --from=builder /lighthouse /lighthouse | ||
|
|
||
| EXPOSE 30303 30303/udp 9001 8545 8546 |
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 am not sure if this is necessary? The ports 8545, 8546, 30303 are for the EL
| .PHONY: build-reproducible | ||
| build-reproducible: ## Build the lighthouse binary into `target` directory with reproducible builds | ||
| SOURCE_DATE_EPOCH=$(SOURCE_DATE) \ | ||
| RUSTFLAGS="${RUST_BUILD_FLAGS} --remap-path-prefix $$(pwd)=." \ | ||
| CARGO_INCREMENTAL=${CARGO_INCREMENTAL_VAL} \ | ||
| LC_ALL=${LOCALE_VAL} \ | ||
| TZ=${TZ_VAL} \ | ||
| cargo build --bin lighthouse --features "$(FEATURES)" --profile "$(PROFILE)" --locked --target $(RUST_TARGET) |
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.
Not sure about this. Do we need this, given that we have 1 for x86_64 and 1 for arm64 below?
I see that the build-reproducible-all below also don't call this build-reproducible
Issue Addressed
This pull request introduces workflows and updates to ensure reproducible builds for the Lighthouse project. It adds two GitHub Actions workflows for building and testing reproducible Docker images and binaries, updates the
Makefileto streamline reproducible build configurations, and modifies theDockerfile.reproducibleto align with the new build process. Additionally, it removes thereproducibleprofile fromCargo.toml.Proposed Changes
New GitHub Actions Workflows:
.github/workflows/release-reproducible.yml: Adds a workflow to build and push reproducible multi-architecture Docker images for releases, including support for dry runs..github/workflows/reproducible-build.yml: Adds a workflow to test reproducibility by comparing binaries built twice forx86_64andaarch64architectures, with failure reporting and artifact uploads.Build Configuration Updates:
Makefile: Refactors reproducible build targets, centralizes environment variables for reproducibility, and updates Docker build arguments forx86_64andaarch64architectures.Dockerfile.reproducible: Updates the base Rust image to version 1.86, removes hardcoded reproducibility settings, and delegates build logic to theMakefile.Profile Removal:
Cargo.toml: Removes thereproducibleprofile, simplifying build configurations and relying on external tooling for reproducibility.Additional Info
This is mainly a follow up to this work #6799 where I refine the reproducible build configuration to simplify the CI workflow to generate the reproducible images and pushes them to DockerHub. I also added a cron job workflow (inspired from the Reth repo) that checks every two days or pull requests that touches files that might affect reproducibility to catch potential regressions.
In case, this is too much, let me know and I can create a separate PR for this to be merged later when necessary
close #7486
close #7485