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

refactor!: docker builds #357

Merged
merged 12 commits into from
Jul 8, 2024
Merged

refactor!: docker builds #357

merged 12 commits into from
Jul 8, 2024

Conversation

0xaatif
Copy link
Contributor

@0xaatif 0xaatif commented Jul 2, 2024

This blocks #309

Breaking changes

  • we now only publish one image, which contains rpc, worker, verifier, and leader, rather than separate worker and leader containers. Users may have to select an ENTRYPOINT or pass additional parameters to their run command
  • we don't include the .env file from the repo in our docker build

Change summary:

  • gets rid of this awful hack:
    pub(crate) fn get_package_version(package_name: &str) -> Result<Option<String>> {
    let manifest_dir = env!("CARGO_MANIFEST_DIR");
    let zero_bin_path = Path::new(manifest_dir)
    .join("../") // Adjust the path according to your workspace structure
    .canonicalize()?;
    let cargo_lock_path = zero_bin_path.join("Cargo.lock");
    let cargo_lock_file = File::open(cargo_lock_path);
    if cargo_lock_file.is_err() {
    return Ok(None);
    }
    let mut cargo_lock_contents = String::new();
    BufReader::new(cargo_lock_file?).read_to_string(&mut cargo_lock_contents)?;
    let lockfile: toml::Value = toml::from_str(&cargo_lock_contents)?;
    if let Some(package) = lockfile["package"]
    .as_array()
    .unwrap()
    .iter()
    .find(|&p| p["name"].as_str() == Some(package_name))
    {
    let version = package["version"].as_str().unwrap();
    return Ok(Some(version.to_string()));
    }
    Ok(None)
    }
    • reviewers, I'm not sure why we don't just have this in evm_arithmetization/src/lib.rs: can anyone educate me?
      pub const VERSION: &str = env!("CARGO_PKG_VERSION");
  • one Dockerfile, with:
    • configuration for e.g cargo build --dev
    • best practices for caching, user config, .dockerignore, apt install...
    • no more sediting our Cargo.toml with a 200-character long command
    • (more) loosely coupled to our code, so e.g whitespace changes(!), new workspace packages etc don't break docker builds
  • one place for RUSTFLAGS

@0xaatif 0xaatif self-assigned this Jul 2, 2024
@github-actions github-actions bot added the crate: zero_bin Anything related to the zero-bin subcrates. label Jul 2, 2024
Dockerfile Show resolved Hide resolved
Dockerfile Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
Dockerfile Show resolved Hide resolved
Dockerfile Show resolved Hide resolved
Dockerfile Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
Dockerfile Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
@atanmarko
Copy link
Member

👌 for the github release image. We will use it as discussed and agreed.

@atanmarko
Copy link
Member

@Nashtare @BGluth Should check the evm_arithmetization versioning, I believe they have discussed it before.

@0xaatif 0xaatif changed the title refactor: docker builds refactor!: docker builds Jul 4, 2024
# Disable the lld linker for now, as it's causing issues with the linkme package.
# https://github.com/rust-lang/rust/pull/124129
# https://github.com/dtolnay/linkme/pull/88
export RUSTFLAGS='-C target-cpu=native -Zlinker-features=-lld'
Copy link
Collaborator

Choose a reason for hiding this comment

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

I see you moved the disabling of lld linker in config.toml but why removing -C target-cpu=native. This should be disabled when building cross-platform images, but it doesn't hurt keeping it in the scripts.

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'll take your lead on this

zero_bin/tools/prove_stdio.sh Outdated Show resolved Hide resolved
Comment on lines +46 to +50
env::set_var(
EVM_ARITH_VER_KEY,
// see build.rs
env!("EVM_ARITHMETIZATION_PACKAGE_VERSION"),
);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe this bit sets the entire version number, i.e. X.Y.Z while we're not interested in the patch section. Any increment of Z should not yield circuits regeneration.

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 don't know the code that reads this, but I thought it would be safer (and rare enough) to include. I'll take your lead on this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

@Nashtare Nashtare left a comment

Choose a reason for hiding this comment

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

LGTM on the versioning part.

@0xaatif 0xaatif merged commit d02e150 into develop Jul 8, 2024
14 checks passed
@0xaatif 0xaatif deleted the 0xaatif/docker branch July 8, 2024 15:44
@Nashtare Nashtare added this to the Type 1 - Q3 2024 milestone Jul 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crate: zero_bin Anything related to the zero-bin subcrates.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants