-
Notifications
You must be signed in to change notification settings - Fork 107
Add run_in_docker.sh to allow repetitive builds #2008
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
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. |
There was a problem hiding this 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
There was a problem hiding this 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
There was a problem hiding this 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 \
There was a problem hiding this 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
There was a problem hiding this 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 \
* 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
This change is