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

build: set up reproducible builds via Docker #206

Merged
merged 33 commits into from
May 22, 2024
Merged

build: set up reproducible builds via Docker #206

merged 33 commits into from
May 22, 2024

Conversation

rvanasa
Copy link
Collaborator

@rvanasa rvanasa commented May 4, 2024

This PR enables reproducible builds via a Docker container. Based on the approach used for the Internet Identity canister.

Supersedes #193.

@@ -1,6 +1,15 @@
use cketh_common::eth_rpc_client::providers::RpcApi;

use crate::*;
use crate::{
Copy link
Collaborator Author

@rvanasa rvanasa May 4, 2024

Choose a reason for hiding this comment

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

This refactoring was necessary for the two-stage Rust build (--only-dependencies and --evm-rpc) to work as expected in the Docker environment.

Comment on lines +13 to +15
apt -yq update && \
apt -yqq install --no-install-recommends curl ca-certificates \
build-essential pkg-config libssl-dev llvm-dev liblmdb-dev clang cmake
Copy link
Collaborator Author

@rvanasa rvanasa May 4, 2024

Choose a reason for hiding this comment

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

This comes from the Internet Identity Dockerfile. Does this remain reproducible if one of these packages is updated on the apt registry?

Copy link
Member

Choose a reason for hiding this comment

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

That's a good question, maybe @frederikrothenberger can enlighten us here 😁

Choose a reason for hiding this comment

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

We never ran into issues so far, but good point. It seems broken?
@nmattia: did we just get lucky (or didn't notice)?

Copy link

Choose a reason for hiding this comment

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

We got lucky! And in general Ubuntu doesn't do big updates without an actual Ubuntu update; stable enough for the needs of II

Copy link

Choose a reason for hiding this comment

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

just to clarify the context for II (I have no context here): II pins cargo & npm which are the two important tools; and npm statically links most libraries that may cause differences in the build output (zlib, etc)

Copy link
Member

Choose a reason for hiding this comment

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

Thanks a lot guys for the quick answer! If it's good enough for II, it should be good enough for the EVM RPC canister.

Copy link

Choose a reason for hiding this comment

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

ok, looks like all the bash files were taken from II. If reproducibility is important you might also want to copy the repro tests that run on CI:

and if the apt issue actually becomes an issue you're probably better off switching to bazel or nix, but you should be fine

Choose a reason for hiding this comment

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

This is a problem shared by II, cycles-ledger and now the EVM RPC canister. Eventually we need to address it.

@rvanasa rvanasa marked this pull request as ready for review May 4, 2024 03:41
Copy link
Member

@gregorydemay gregorydemay left a comment

Choose a reason for hiding this comment

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

Thanks a lot @rvanasa for this effort. I think the pieces are mostly there; however, I failed to reproduce the build between Mac and devenv, which is unexpected. I think the CI checks suggested by Nicolas would make a lot of sense.

Dockerfile Outdated Show resolved Hide resolved
Comment on lines +13 to +15
apt -yq update && \
apt -yqq install --no-install-recommends curl ca-certificates \
build-essential pkg-config libssl-dev llvm-dev liblmdb-dev clang cmake
Copy link
Member

Choose a reason for hiding this comment

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

Thanks a lot guys for the quick answer! If it's good enough for II, it should be good enough for the EVM RPC canister.

Dockerfile Outdated
&& rm -rf src \
&& rm -rf Cargo.toml \
&& rm -rf Cargo.lock \
&& rm -rf target \
Copy link
Member

Choose a reason for hiding this comment

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

Are you sure about this? AFAIK, II put the built dependencies in

ENV CARGO_TARGET_DIR=/cargo_target

and never deletes it. Otherwise I currently don't see the point of pre-building the cargo dependencies.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed rm -rf target; the other lines are necessary for the COPY line below to work as expected.

CARGO_HOME=/cargo \
PATH=/cargo/bin:$PATH

WORKDIR /evm_rpc
Copy link
Member

Choose a reason for hiding this comment

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

unless I'm missing something, I don't think you need an explicit working directory, the default (I think /) should be fine

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a workaround for a nebulous error message (along the lines of ERROR: cannot copy to non-directory: /var/lib/docker/overlay2/fso8ezwpr886l9bjnx3jni37e/merged/lib). I'll address this in a follow-up PR (trying to get this merged by the end of the day if possible).

README.md Outdated Show resolved Hide resolved
RUN mkdir -p src \
&& echo "fn main() {}" > src/main.rs \
&& echo "" > src/lib.rs \
&& mkdir -p e2e/rust/src \
Copy link
Member

Choose a reason for hiding this comment

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

Since we only care about building the canister evm_rpc.wasm.gz, why do we need to care about stuff in e2e?

Copy link
Collaborator Author

@rvanasa rvanasa May 21, 2024

Choose a reason for hiding this comment

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

This was required to allow the Cargo workspace to pre-build as expected (since it also refers to the E2E Cargo.toml file).

scripts/bootstrap Show resolved Hide resolved
scripts/build Outdated Show resolved Hide resolved
scripts/docker-build Outdated Show resolved Hide resolved
scripts/version Outdated Show resolved Hide resolved
@rvanasa rvanasa marked this pull request as draft May 22, 2024 11:34
@rvanasa rvanasa marked this pull request as ready for review May 22, 2024 14:50
scripts/docker-build Outdated Show resolved Hide resolved
@gregorydemay
Copy link
Member

LGTM, thanks for pushing this effort @rvanasa!

@rvanasa rvanasa merged commit a6d2f9b into main May 22, 2024
3 checks passed
@rvanasa rvanasa deleted the docker-build branch May 22, 2024 16:12
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.

5 participants