feat: dependencies.sh script for dev onboarding #1116
Conversation
There was a problem hiding this comment.
Reviewable status: 0 of 1 files reviewed, all discussions resolved (waiting on @alon-dotan-starkware and @noaov1)
scripts/dependencies.sh line 3 at r1 (raw file):
#!/bin/bash sudo -s
when running this script, sudo context is entered but the apt commands aren't run until I exit.
Why?
should every command in this script be prepended with sudo explicitly?
Code quote:
sudo -sscripts/dependencies.sh line 4 at r1 (raw file):
sudo -s pushd /tmp
instead of cd, + popd added at the end
Code quote:
pushd /tmp
dorimedini-starkware
force-pushed
the
dori/dependencies-script
branch
from
1d10040
to
e5d55da
Compare
There was a problem hiding this comment.
Reviewable status: 0 of 1 files reviewed, all discussions resolved (waiting on @alon-dotan-starkware and @noaov1)
scripts/dependencies.sh line 3 at r1 (raw file):
Previously, dorimedini-starkware wrote…
when running this script, sudo context is entered but the apt commands aren't run until I
exit.
Why?
should every command in this script be prepended withsudoexplicitly?
explicit sudos seem to work
There was a problem hiding this comment.
Reviewable status: 0 of 1 files reviewed, all discussions resolved (waiting on @noaov1)
scripts/dependencies.sh line 3 at r1 (raw file):
Previously, dorimedini-starkware wrote…
explicit
sudos seem to work
Its duplicate code... Just import the function
There was a problem hiding this comment.
Reviewable status: 0 of 1 files reviewed, all discussions resolved (waiting on @noaov1)
scripts/dependencies.sh line 3 at r1 (raw file):
Previously, alon-dotan-starkware wrote…
Its duplicate code... Just import the function
hmmm... what would be the difference between this script and setup_native_deps.sh?
should we just rename setup_native_deps to dependencies, or should we decouple these scripts?
Signed-off-by: Dori Medini <dori@starkware.co>
dorimedini-starkware
force-pushed
the
dori/dependencies-script
branch
from
e5d55da
to
376d61e
Compare
There was a problem hiding this comment.
Reviewable status: 0 of 1 files reviewed, all discussions resolved (waiting on @noaov1)
scripts/dependencies.sh line 3 at r1 (raw file):
Previously, dorimedini-starkware wrote…
hmmm... what would be the difference between this script and setup_native_deps.sh?
should we just rename setup_native_deps to dependencies, or should we decouple these scripts?
if we want to keep the scripts separate... which invocations of setup_native_deps should actually invoke dependencies?
There was a problem hiding this comment.
Reviewable status: 0 of 1 files reviewed, all discussions resolved (waiting on @noaov1)
scripts/dependencies.sh line 3 at r1 (raw file):
Previously, dorimedini-starkware wrote…
if we want to keep the scripts separate... which invocations of
setup_native_depsshould actually invokedependencies?
We can add the protobuf deps in here
There was a problem hiding this comment.
Reviewable status: 0 of 1 files reviewed, all discussions resolved (waiting on @noaov1)
scripts/dependencies.sh line 3 at r1 (raw file):
Previously, Yoni-Starkware (Yoni) wrote…
We can add the protobuf deps in here
@Yoni-Starkware will do that in a future PR after this is merged
There was a problem hiding this comment.
Reviewable status: 0 of 1 files reviewed, all discussions resolved (waiting on @noaov1)
scripts/dependencies.sh line 3 at r1 (raw file):
Previously, ShahakShama wrote…
@Yoni-Starkware will do that in a future PR after this is merged
*I will
There was a problem hiding this comment.
Reviewable status: 0 of 1 files reviewed, all discussions resolved (waiting on @noaov1)
scripts/dependencies.sh line 3 at r1 (raw file):
Previously, ShahakShama wrote…
*I will
when someone LGTMs I'll merge :)
There was a problem hiding this comment.
Reviewable status: 0 of 1 files reviewed, all discussions resolved (waiting on @noaov1)
scripts/dependencies.sh line 3 at r1 (raw file):
Previously, dorimedini-starkware wrote…
when someone LGTMs I'll merge :)
can we NOT!! pust protobuf here and add it to the build.rs?
There was a problem hiding this comment.
Reviewable status: 0 of 13 files reviewed, 2 unresolved discussions (waiting on @noaov1)
Dockerfile line 14 at r4 (raw file):
COPY scripts/install_build_tools.sh . COPY scripts/dependencies.sh .
you will still need setup_native_deps, this script is just a wrapper
Code quote:
COPY scripts/dependencies.sh ..github/workflows/blockifier_ci.yml line 32 at r4 (raw file):
- 'crates/native_blockifier/**' - 'scripts/build_native_blockifier.sh' - 'scripts/dependencies.sh'
if this job needs triggering on change to dev setup script, then it needs triggering when setup_native_deps is edited, right?
same comment for all CI workflows in this PR where you added dependencies.sh to the triggers
Code quote:
'scripts/dependencies.sh'
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1116 +/- ##
===========================================
- Coverage 74.18% 47.00% -27.19%
===========================================
Files 359 246 -113
Lines 36240 26859 -9381
Branches 36240 26859 -9381
===========================================
- Hits 26886 12626 -14260
- Misses 7220 13332 +6112
+ Partials 2134 901 -1233
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Reviewable status: 0 of 13 files reviewed, all discussions resolved (waiting on @noaov1)
There was a problem hiding this comment.
Reviewed 1 of 1 files at r3.
Reviewable status: 0 of 13 files reviewed, all discussions resolved (waiting on @noaov1)
There was a problem hiding this comment.
Reviewable status: 0 of 13 files reviewed, all discussions resolved (waiting on @noaov1)
There was a problem hiding this comment.
Reviewed 13 of 13 files at r4, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @noaov1)
This change is