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 reproducible build profile and Dockerfile #42

Merged
merged 9 commits into from
Jan 17, 2025

Conversation

MoeMahhouk
Copy link
Contributor

This PR adds a reproducible build profile and a Dockerfile that will build the binary reproducibly and puts it in a distroless minimal base image.
This will later be used in the BuilderNet v1.3 where most services will run in containers.

Steps to test:

mkdir output1 output2
docker build --no-cache --output=output1 .
docker build --no-cache --output=output2 . 
sha256sum output1/rbuilder output2/rbuilder
# hashes should be identical
rm -rf output1 output2 

@ZanCorDX
Copy link
Collaborator

ZanCorDX commented Jan 14, 2025

I see you don't use release defaults (https://doc.rust-lang.org/cargo/reference/profiles.html):

[profile.release]
opt-level = 3
debug = false
split-debuginfo = '...'  # Platform-specific.
strip = "none"
debug-assertions = false
overflow-checks = false
lto = false
panic = 'unwind'
incremental = false
codegen-units = 16
rpath = false

I'm worried about behavior change. Are you 100% sure about each item? eg: overflow-checks, panic.

@metachris
Copy link

metachris commented Jan 14, 2025

agree with Dan about there being a bunch of items. imo the fewer changes the better! which ones are absolutely necessary?

@MoeMahhouk
Copy link
Contributor Author

I see you don't use release defaults (https://doc.rust-lang.org/cargo/reference/profiles.html):

[profile.release]
opt-level = 3
debug = false
split-debuginfo = '...'  # Platform-specific.
strip = "none"
debug-assertions = false
overflow-checks = false
lto = false
panic = 'unwind'
incremental = false
codegen-units = 16
rpath = false

I'm worried about behavior change. Are you 100% sure about each item? eg: overflow-checks, panic.

good point, I will test again with the release profile without changes and see if the reproducibility still holds.
The only change that I think might be important is this codegen-units = 1 because it might introduce non-determinism factor to the build process.

Dockerfile Outdated
ENV BUILD_PROFILE=$BUILD_PROFILE

# Extra Cargo flags
ARG 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.

would it be too much to ask to break out each flag into it's own line, with a comment what it does? like in mev-boost here: https://github.com/flashbots/mev-boost/blob/develop/Makefile#L4-L15

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I can do that there too

@MoeMahhouk MoeMahhouk requested a review from metachris January 17, 2025 12:34
Dockerfile Outdated
Comment on lines 6 to 17
# RUSTFLAGS breakdown:
# -C target-feature=+crt-static -> Statically link the C runtime library for standalone binaries
# -C link-arg=-Wl,--build-id=none -> Remove build ID from binary for reproducibility
# -Clink-arg=-static-libgcc -> Statically link against libgcc
# -C metadata='' -> Remove metadata hash from symbol names for reproducible builds
# --remap-path-prefix $(pwd)=. -> Replace absolute paths with '.' in debug info
ARG RUSTFLAGS="-C target-feature=+crt-static \
-C link-arg=-Wl,--build-id=none \
-Clink-arg=-static-libgcc \
-C metadata='' \
--remap-path-prefix $(pwd)=."
ENV RUSTFLAGS="$RUSTFLAGS"

Choose a reason for hiding this comment

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

Suggested change
# RUSTFLAGS breakdown:
# -C target-feature=+crt-static -> Statically link the C runtime library for standalone binaries
# -C link-arg=-Wl,--build-id=none -> Remove build ID from binary for reproducibility
# -Clink-arg=-static-libgcc -> Statically link against libgcc
# -C metadata='' -> Remove metadata hash from symbol names for reproducible builds
# --remap-path-prefix $(pwd)=. -> Replace absolute paths with '.' in debug info
ARG RUSTFLAGS="-C target-feature=+crt-static \
-C link-arg=-Wl,--build-id=none \
-Clink-arg=-static-libgcc \
-C metadata='' \
--remap-path-prefix $(pwd)=."
ENV RUSTFLAGS="$RUSTFLAGS"
ARG RUSTFLAGS="-C target-feature=+crt-static \ # Statically link the C runtime library for standalone binaries
-C link-arg=-Wl,--build-id=none \ # Remove build ID from binary for reproducibility
-Clink-arg=-static-libgcc \ # Statically link against libgcc
-C metadata='' \ # Remove metadata hash from symbol names for reproducible builds
--remap-path-prefix $(pwd)=." # Replace absolute paths with '.' in debug info
ENV RUSTFLAGS="$RUSTFLAGS"

Copy link
Contributor Author

@MoeMahhouk MoeMahhouk Jan 17, 2025

Choose a reason for hiding this comment

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

this doesnt work in Docker
You cant have inline comments there. Thats why I pulled them outside of it.
However, this is possible

ARG RUSTFLAGS="\
# Statically link the C runtime library for standalone binaries
-C target-feature=+crt-static \
# Remove build ID from binary for reproducibility
-C link-arg=-Wl,--build-id=none \
# Statically link against libgcc
-Clink-arg=-static-libgcc \
# Remove metadata hash from symbol names for reproducible builds
-C metadata='' \
# Replace absolute paths with '.' in debug info
--remap-path-prefix $(pwd)=."
ENV RUSTFLAGS="$RUSTFLAGS"

* 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
@ZanCorDX ZanCorDX requested a review from metachris January 17, 2025 14:53
Copy link

@bakhtin bakhtin left a comment

Choose a reason for hiding this comment

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

lgtm.

One implementation detail that I don't particularly like is duplication of flags in both Dockerfile and Makefile. One would need to keep them in sync to match the behavior of Docker build vs make build-reproducible.
Maybe in the future we can move them to an externally managed profile.

Dockerfile Outdated Show resolved Hide resolved
@MoeMahhouk MoeMahhouk merged commit 260b67e into main Jan 17, 2025
1 check passed
@MoeMahhouk MoeMahhouk deleted the reproducible-builds branch January 17, 2025 22:23
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.

4 participants