Skip to content
This repository has been archived by the owner on Aug 21, 2024. It is now read-only.

Add run_in_docker.sh to allow repetitive builds #2008

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

alon-dotan-starkware
Copy link
Contributor

@alon-dotan-starkware alon-dotan-starkware commented Jun 26, 2024

This change is Reviewable

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.35%. Comparing base (a3bda1e) to head (ec040a2).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2008   +/-   ##
=======================================
  Coverage   78.35%   78.35%           
=======================================
  Files          63       63           
  Lines        8990     8990           
  Branches     8990     8990           
=======================================
  Hits         7044     7044           
  Misses       1498     1498           
  Partials      448      448           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@dorimedini-starkware dorimedini-starkware left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @alon-dotan-starkware)

a discussion (no related file):
please add a build_native_blockifier_artifacts.sh script that does everything (including the cargo build).

Even better: since we don't want any of the below scripts to be run "standalone" in the context of this repo, I would prefer all logic reside in the same bash file (dockerfile must stay separate of course)



scripts/install_build_tools.sh line 9 at r1 (raw file):

        curl \
        python3-dev
}

we have a single use for all of these scripts, if this isn't used in that use case please delete

Code quote:

function install_base_packages () {
    apt update && apt -y install \
        build-essential \
        clang \
        curl \
        python3-dev
}

scripts/install_build_tools.sh line 41 at r1 (raw file):

# Uncomment only if base packages is not installed via Dockerfile
# install_base_packages

in which case(s) is this useful? these scripts have a single purpose

Copy link
Collaborator

@dorimedini-starkware dorimedini-starkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @alon-dotan-starkware)


Dockerfile line 15 at r1 (raw file):

COPY scripts/install_build_tools.sh .
RUN bash install_build_tools.sh

are all of these lines cached? or will consecutive runs of run_in_docker reinstall everything?

Code quote:

FROM ubuntu:20.04

RUN apt update && apt -y install \
    build-essential \
    clang \
    curl \
    python3-dev

ENV RUSTUP_HOME=/opt/rust
ENV CARGO_HOME=/opt/rust
ENV PATH=$PATH:/opt/rust/bin


COPY scripts/install_build_tools.sh .
RUN bash install_build_tools.sh

Copy link
Collaborator

@dorimedini-starkware dorimedini-starkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @alon-dotan-starkware)


run_in_docker.sh line 11 at r1 (raw file):

    --net host \
    -e CARGO_HOME=${HOME}/.cargo \
    -u $UID \

or is there a better way to inject this env var? (needed for cargo build)

Suggestion:

    -e CARGO_HOME=${HOME}/.cargo \
    -e PYO3_PYTHON=/usr/local/bin/pypy3.9 \
    -u $UID \

Copy link
Collaborator

@dorimedini-starkware dorimedini-starkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @alon-dotan-starkware)


scripts/install_build_tools.sh line 9 at r1 (raw file):

Previously, dorimedini-starkware wrote…

we have a single use for all of these scripts, if this isn't used in that use case please delete

nevermind... need a main script, a dockerfile, and a script called from the dockerfile. my bad

Copy link
Collaborator

@dorimedini-starkware dorimedini-starkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @alon-dotan-starkware)


run_in_docker.sh line 10 at r1 (raw file):

    --rm \
    --net host \
    -e CARGO_HOME=${HOME}/.cargo \

the ci rust-caching mechanism is supposed to populate .cargo with prebuilt dependencies; is it possible to copy them over when building the docker / running the docker?
or is it not required?
I would like to run this script directly in the CI without a double point of truth (the cargo build command should exist in one place), and want to use the caching

Code quote:

-e CARGO_HOME=${HOME}/.cargo \

gswirski pushed a commit to reilabs/blockifier that referenced this pull request Jun 26, 2024
* feat: new types in unity bindgen

* feat: add types support to typescript codegen too

* feat: handle token directly to get type name

* chore: correct type names

* chore: update to correct tuple  types

* fmt

* Update mod.rs

* Update mod.rs

* Update mod.rs

* feat: add support for complex enums to unity bindgen

* feat: support generic args in typescript

* fmt

* feat: newer tuple type for c#

* feat: value type name in record field

* feat: generic rust like enum for unity bindgen

* refacotr: optional generic args for record

* feat: add suppport for generic enums for typescript v2

* feat: update test & disable typescript

* fix: update unity systsem bindgen to use new felt getter starkware-libs#2008

* chore: clippy

* clean uop
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants