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

feat: add extra job for the reproducible docker build in the CI #43

Merged
merged 4 commits into from
Jan 17, 2025

Conversation

MoeMahhouk
Copy link
Contributor

This PR adds the reproducible build CI job to create the distroless base image with the reproducibly built binary and uploading the result container image

@@ -87,7 +94,12 @@ jobs:
run: |
git config --global --add safe.directory "$(pwd)"
. $HOME/.cargo/env
cargo build --release --target ${{ matrix.configs.target }}
SOURCE_DATE_EPOCH=$(git log -1 --pretty=%ct)
RUSTFLAGS="-C target-feature=+crt-static -C link-arg=-Wl,--build-id=none -Clink-arg=-static-libgcc -C metadata='' --remap-path-prefix $(pwd)=." \

Choose a reason for hiding this comment

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

I feel it's important to explain exactly what each of these flags does. I like the way it's done in mev-boost, which is adding flags one by one, with a comment for each: https://github.com/flashbots/mev-boost/blob/develop/Makefile#L4-L15

Cargo.toml Show resolved Hide resolved
@@ -87,7 +94,12 @@ jobs:
run: |
git config --global --add safe.directory "$(pwd)"
. $HOME/.cargo/env
cargo build --release --target ${{ matrix.configs.target }}
SOURCE_DATE_EPOCH=$(git log -1 --pretty=%ct)
RUSTFLAGS="-C target-feature=+crt-static -C link-arg=-Wl,--build-id=none -Clink-arg=-static-libgcc -C metadata='' --remap-path-prefix $(pwd)=." \
Copy link

Choose a reason for hiding this comment

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

Can we move RUSTFLAGS to a Cargo.toml config? The flags would either make a new profile (reproducible? which is now removed). Or, replace the default release profile.
I feel it is important to have it in the config file rather than just in the pipeline definition when the release is built outside of a CI.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea.
I wrote the reason behind removing the introduced cargo profile below.

Copy link
Contributor Author

@MoeMahhouk MoeMahhouk Jan 16, 2025

Choose a reason for hiding this comment

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

I tried to create profile.reproducible.env or even profile.reproducible and add env variables to it.
It seems Rust cargo.toml does not support env build variables for a specific build profile like profile.reproducible or profile.release.
Do you have a referrence for this that I can look into?

Copy link

@bakhtin bakhtin Jan 16, 2025

Choose a reason for hiding this comment

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

Check out https://doc.rust-lang.org/cargo/reference/manifest.html and https://doc.rust-lang.org/cargo/reference/config.html#configuration-format. You can override the profile definition in the config file (you'll need to create one for the repo). Overridden profile ([profile.<name>.build-override]) allows setting rustflags for the specific profile.

It seems that you need to remove profile definitions from Cargo.toml and create them in .cargo/config.toml with overrides.

To set env vars for a profile (e.g., SOURCE_DATE_EPOCH) it seems you can override rustc-wrapper with something like rustc-wrapper=SOURCE_DATE_EPOCH=<...> rustc

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried this and it seems that cargo does not support env vars for specific build profiles and seems a bit too much overkill for the purpose that we need it for.
So I will go with the suggestion of @metachris above because it is straight forward and clean in the Makefile and will call the build target in the github workflow job there instead.
This way, it will be also consistent between the CI and local builds

@MoeMahhouk MoeMahhouk force-pushed the reproducible-build-ci branch from d9f65b8 to 3e8e59d Compare January 17, 2025 12:26
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is from the PR#42 and not introduced by this one.

Dockerfile Outdated
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is from the PR#42 and not introduced by this one.

@MoeMahhouk MoeMahhouk merged commit 16b2904 into reproducible-builds Jan 17, 2025
1 check passed
@MoeMahhouk MoeMahhouk deleted the reproducible-build-ci branch January 17, 2025 14:22
MoeMahhouk added a commit that referenced this pull request Jan 17, 2025
* feat: Add reproducible build profile and Dockerfile

* remove the extra Cargo.toml profiles

* Add comments to the rust flags

* feat: add extra job for the reproducible docker build in the CI (#43)

* feat: add extra job for the reproducible docker build in the CI

* remove the extra Cargo.toml profiles

* add comments to the rust flags and move them into Makefile

* Add comments to the rust flags

* refactor the comments position

* chore: re-use the build logic and setup from the makefile

* remove unnecessary flags

* Revert "remove unnecessary flags"

This reverts commit 1977afb.

* remove static linking of libgcc
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.

3 participants