perf: remove duplicate program ci builds#1972
Conversation
WalkthroughCentralizes CI setup into a versioned, per-component setup action; adds scripts/devenv/* installers and shared utilities; removes pervasive devenv.sh sourcing across workflows; introduces CI-mode NX targets and CI-limited prover builds; delegates root install to devenv orchestrator; removes snarkjs devDependency. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Runner as GitHub Runner
participant WF as Workflow Job
participant SA as setup-and-build Action
participant DV as scripts/devenv/*
participant NX as npx nx build-ci
participant T as Test Matrix
Runner->>WF: start job
WF->>SA: invoke setup-and-build (skip-components, cache-suffix)
SA->>DV: load versions & shared helpers
SA->>DV: for each component: check cache → (install if cache miss)
SA->>DV: download proving keys (if needed)
SA-->>WF: export PATH/env, caches populated
WF->>NX: npx nx build-ci @lightprotocol/zk-compression-cli
WF->>T: run matrix sub-tests (with per-subtest retries for flaky tests)
T-->>WF: results
sequenceDiagram
autonumber
actor CI as CI Step
participant BP as buildProver.sh
CI->>BP: run buildProver.sh --ci
BP->>BP: parse args → CI_MODE=true
BP->>CI: build only Linux x64 prover binary
CI-->>BP: binary produced
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60–90 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
.github/workflows/light-system-programs-tests.yml (1)
85-90: Same concern as CLI v1 workflowPlease apply the same cleanup/verification as noted in
.github/workflows/cli-v1.yml..github/workflows/forester-tests.yml (1)
82-87: Same concern as CLI v1 workflowPlease apply the same cleanup/verification as noted in
.github/workflows/cli-v1.yml..github/workflows/rust.yml (1)
97-102: Same concern as CLI v1 workflowPlease apply the same cleanup/verification as noted in
.github/workflows/cli-v1.yml..github/workflows/sdk-tests.yml (1)
96-98: Same concern as the preceding commentPlease apply the same cleanup/verification as called out just above.
.github/workflows/cli-v2.yml (1)
73-78: Same concern as CLI v1 workflowPlease apply the same cleanup/verification as noted in
.github/workflows/cli-v1.yml.
🧹 Nitpick comments (2)
.github/workflows/cli-v1.yml (1)
73-78: Clean up commented CLI step and verify build coverageIf the composite
setup-and-buildaction already compiles the CLI, we should delete this commented block to avoid future confusion. Please also double-check that the action indeed produces the CLI artifacts; otherwise the subsequentnx testmay end up rebuilding or fail outright.Apply this diff to drop the dead block once confirmed:
- # NOTE: CLI build removed - now handled by setup-and-build action - # Commenting out to test if duplicate build is necessary - # - name: Build CLI with V1 - # run: | - # source ./scripts/devenv.sh - # npx nx build @lightprotocol/zk-compression-cli.github/workflows/sdk-tests.yml (1)
82-84: Remove commented CLI build and confirm composite action coverageAs with the CLI workflows, let’s delete this residual block once you’ve confirmed the composite action delivers the CLI build artifacts.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.github/workflows/cli-v1.yml(1 hunks).github/workflows/cli-v2.yml(1 hunks).github/workflows/forester-tests.yml(1 hunks).github/workflows/light-system-programs-tests.yml(1 hunks).github/workflows/rust.yml(1 hunks).github/workflows/sdk-tests.yml(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: cli-v1
- GitHub Check: stateless-js-v2
- GitHub Check: stateless-js-v1
- GitHub Check: Forester e2e test
- GitHub Check: Test concurrent-merkle-tree
- GitHub Check: lint
- GitHub Check: Test program-libs-fast
- GitHub Check: Test program-libs-slow
- GitHub Check: Test batched-merkle-tree-simulate
- GitHub Check: system-programs (system-cpi-test-v2-event, ["cargo-test-sbf -p system-cpi-v2-test -- event::parse"])
- GitHub Check: system-programs (system-cpi-test-v2-functional, ["cargo-test-sbf -p system-cpi-v2-test -- functio...
- GitHub Check: system-programs (random-e2e-test, ["cargo-test-sbf -p e2e-test"])
- GitHub Check: system-programs (system-cpi-test-v2-other, ["cargo-test-sbf -p system-cpi-v2-test -- --skip funct...
- GitHub Check: system-programs (light-compressed-token, ["cargo-test-sbf -p compressed-token-test"])
- GitHub Check: system-programs (system-cpi-test, ["cargo-test-sbf -p system-cpi-test", "cargo test -p light-syst...
- GitHub Check: system-programs (light-system-program, ["cargo-test-sbf -p system-test"])
- GitHub Check: system-programs (account-compression, ["cargo-test-sbf -p account-compression-test"])
- GitHub Check: system-programs (light-registry, ["cargo-test-sbf -p registry-test"])
- GitHub Check: cli-v2
ananas-block
force-pushed
the
jorrit/perf-optimize-ci-build-time
branch
from
f924aea to
e4235b3
Compare
ananas-block
changed the title
ananas-block
force-pushed
the
jorrit/perf-optimize-ci-build-time
branch
from
e4235b3 to
cf40131
Compare
ananas-block
force-pushed
the
jorrit/perf-optimize-ci-build-time
branch
from
cf40131 to
c4e1e35
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
.github/workflows/rust.yml (1)
97-101: Prefer pnpm exec for Nx to avoid npx download overheadUse pnpm exec nx build-ci @lightprotocol/zk-compression-cli for faster, deterministic CI (uses workspace-installed Nx).
.github/workflows/light-system-programs-tests.yml (1)
85-89: Use pnpm exec for Nx invocationReplace npx nx with pnpm exec nx to leverage the workspace install and speed up CI.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.github/actions/setup-and-build-nocheck/action.yml(0 hunks).github/workflows/forester-tests.yml(1 hunks).github/workflows/light-system-programs-tests.yml(1 hunks).github/workflows/rust.yml(1 hunks).github/workflows/sdk-tests.yml(1 hunks)cli/package.json(2 hunks)
💤 Files with no reviewable changes (1)
- .github/actions/setup-and-build-nocheck/action.yml
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/forester-tests.yml
- .github/workflows/sdk-tests.yml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: cli-v1
- GitHub Check: system-programs (system-cpi-test-v2-functional, ["cargo-test-sbf -p system-cpi-v2-test -- functio...
- GitHub Check: system-programs (system-cpi-test-v2-event, ["cargo-test-sbf -p system-cpi-v2-test -- event::parse"])
- GitHub Check: system-programs (random-e2e-test, ["cargo-test-sbf -p e2e-test"])
- GitHub Check: system-programs (system-cpi-test, ["cargo-test-sbf -p system-cpi-test", "cargo test -p light-syst...
- GitHub Check: system-programs (light-compressed-token, ["cargo-test-sbf -p compressed-token-test"])
- GitHub Check: system-programs (system-cpi-test-v2-other, ["cargo-test-sbf -p system-cpi-v2-test -- --skip funct...
- GitHub Check: system-programs (light-system-program, ["cargo-test-sbf -p system-test"])
- GitHub Check: system-programs (light-registry, ["cargo-test-sbf -p registry-test"])
- GitHub Check: system-programs (account-compression, ["cargo-test-sbf -p account-compression-test"])
- GitHub Check: Forester e2e test
- GitHub Check: Test program-libs-slow
- GitHub Check: Test concurrent-merkle-tree
- GitHub Check: Test program-libs-fast
- GitHub Check: stateless-js-v2
- GitHub Check: Test batched-merkle-tree-simulate
- GitHub Check: stateless-js-v1
- GitHub Check: cli-v2
- GitHub Check: lint
🔇 Additional comments (1)
cli/package.json (1)
113-118: CI scripts LGTMadd-bins-ci and build-ci are consistent with the new CI mode and keep release paths unchanged.
ananas-block
force-pushed
the
jorrit/perf-optimize-ci-build-time
branch
from
d728de4 to
18c9244
Compare
ananas-block
force-pushed
the
jorrit/perf-optimize-ci-build-time
branch
from
5140b25 to
68671e0
Compare
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (1)
scripts/devenv/install-node.sh (1)
9-10: Prefer separate assignments for command substitutions.ShellCheck flagged these combined declare/assign patterns. Split the
localdeclarations from the command substitutions to keep the exit status of the helper calls intact.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (31)
.github/actions/setup-and-build-nocheck/action.yml(0 hunks).github/actions/setup-and-build/action.yml(2 hunks).github/workflows/cli-v1.yml(1 hunks).github/workflows/cli-v2.yml(1 hunks).github/workflows/forester-tests.yml(2 hunks).github/workflows/js-v2.yml(1 hunks).github/workflows/js.yml(1 hunks).github/workflows/light-system-programs-tests.yml(1 hunks).github/workflows/lint.yml(0 hunks).github/workflows/release-pr-validation.yml(1 hunks).github/workflows/release-rust.yml(3 hunks).github/workflows/rust.yml(1 hunks).github/workflows/sdk-tests.yml(1 hunks)cli/package.json(2 hunks)cli/scripts/buildProver.sh(2 hunks)package.json(0 hunks)scripts/devenv/download-gnark-keys.sh(1 hunks)scripts/devenv/install-anchor.sh(1 hunks)scripts/devenv/install-dependencies.sh(1 hunks)scripts/devenv/install-go.sh(1 hunks)scripts/devenv/install-jq.sh(1 hunks)scripts/devenv/install-node.sh(1 hunks)scripts/devenv/install-photon.sh(1 hunks)scripts/devenv/install-pnpm.sh(1 hunks)scripts/devenv/install-redis.sh(1 hunks)scripts/devenv/install-rust.sh(1 hunks)scripts/devenv/install-solana.sh(1 hunks)scripts/devenv/install.sh(1 hunks)scripts/devenv/shared.sh(1 hunks)scripts/devenv/versions.sh(1 hunks)scripts/install.sh(1 hunks)
💤 Files with no reviewable changes (3)
- .github/workflows/lint.yml
- package.json
- .github/actions/setup-and-build-nocheck/action.yml
🚧 Files skipped from review as they are similar to previous changes (4)
- .github/workflows/rust.yml
- cli/package.json
- .github/workflows/light-system-programs-tests.yml
- .github/workflows/js.yml
🧰 Additional context used
🧬 Code graph analysis (11)
scripts/devenv/install-solana.sh (1)
scripts/devenv/shared.sh (4)
get_version(54-64)get_suffix(66-76)download(78-81)log(51-51)
scripts/devenv/download-gnark-keys.sh (1)
scripts/devenv/shared.sh (1)
log(51-51)
scripts/devenv/install-pnpm.sh (1)
scripts/devenv/shared.sh (4)
get_version(54-64)get_suffix(66-76)download(78-81)log(51-51)
scripts/devenv/install-jq.sh (1)
scripts/devenv/shared.sh (4)
get_version(54-64)get_suffix(66-76)download(78-81)log(51-51)
scripts/devenv/install-go.sh (1)
scripts/devenv/shared.sh (4)
get_version(54-64)get_suffix(66-76)download(78-81)log(51-51)
scripts/devenv/install-redis.sh (1)
scripts/devenv/shared.sh (2)
get_version(54-64)log(51-51)
scripts/devenv/install-node.sh (1)
scripts/devenv/shared.sh (4)
get_version(54-64)get_suffix(66-76)download(78-81)log(51-51)
scripts/devenv/install-anchor.sh (1)
scripts/devenv/shared.sh (2)
get_version(54-64)log(51-51)
scripts/devenv/install-dependencies.sh (1)
scripts/devenv/shared.sh (1)
log(51-51)
scripts/devenv/install-rust.sh (1)
scripts/devenv/shared.sh (1)
log(51-51)
scripts/devenv/install-photon.sh (1)
scripts/devenv/shared.sh (2)
get_version(54-64)log(51-51)
🪛 Shellcheck (0.11.0)
scripts/devenv/install-solana.sh
[warning] 9-9: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 10-10: Declare and assign separately to avoid masking return values.
(SC2155)
cli/scripts/buildProver.sh
[warning] 12-12: In POSIX sh, [[ ]] is undefined.
(SC3010)
scripts/devenv/shared.sh
[warning] 48-48: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 49-49: Declare and assign separately to avoid masking return values.
(SC2155)
scripts/devenv/install-pnpm.sh
[warning] 9-9: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 10-10: Declare and assign separately to avoid masking return values.
(SC2155)
scripts/devenv/install-jq.sh
[warning] 9-9: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 10-10: Declare and assign separately to avoid masking return values.
(SC2155)
scripts/devenv/install-go.sh
[warning] 9-9: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 10-10: Declare and assign separately to avoid masking return values.
(SC2155)
scripts/devenv/install-redis.sh
[warning] 9-9: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 24-24: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
[warning] 28-28: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
scripts/devenv/install-node.sh
[warning] 9-9: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 10-10: Declare and assign separately to avoid masking return values.
(SC2155)
scripts/devenv/install-anchor.sh
[warning] 7-7: Declare and assign separately to avoid masking return values.
(SC2155)
scripts/devenv/versions.sh
[warning] 6-6: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 7-7: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 8-8: Declare and assign separately to avoid masking return values.
(SC2155)
scripts/devenv/install-photon.sh
[warning] 7-7: Declare and assign separately to avoid masking return values.
(SC2155)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: cli-v1
- GitHub Check: stateless-js-v1
- GitHub Check: stateless-js-v2
- GitHub Check: cli-v2
- GitHub Check: lint
- GitHub Check: system-programs (system-cpi-test-v2-functional, ["cargo-test-sbf -p system-cpi-v2-test -- functio...
- GitHub Check: system-programs (light-compressed-token, ["cargo-test-sbf -p compressed-token-test"])
- GitHub Check: system-programs (random-e2e-test, ["cargo-test-sbf -p e2e-test"])
- GitHub Check: system-programs (system-cpi-test-v2-event, ["cargo-test-sbf -p system-cpi-v2-test -- event::parse"])
- GitHub Check: system-programs (account-compression, ["cargo-test-sbf -p account-compression-test"])
- GitHub Check: system-programs (system-cpi-test, ["cargo-test-sbf -p system-cpi-test", "cargo test -p light-syst...
- GitHub Check: system-programs (system-cpi-test-v2-other, ["cargo-test-sbf -p system-cpi-v2-test -- --skip funct...
- GitHub Check: system-programs (light-registry, ["cargo-test-sbf -p registry-test"])
- GitHub Check: system-programs (light-system-program, ["cargo-test-sbf -p system-test"])
- GitHub Check: Forester e2e test
- GitHub Check: Test concurrent-merkle-tree
- GitHub Check: Test program-libs-fast
- GitHub Check: Test batched-merkle-tree-simulate
- GitHub Check: Test program-libs-slow
| echo "Building Rust programs..." | ||
| npx nx build @lightprotocol/programs || { | ||
| echo "Failed to build programs, retrying with verbose output..." | ||
| npx nx build @lightprotocol/programs --verbose | ||
| exit 1 | ||
| } |
There was a problem hiding this comment.
Fix fallback build blocks: they always fail even after a successful retry.
Both “Build Rust programs” and “Build Rust program-tests” wrap the initial npx nx build … in an || { … exit 1; } block. Because the block ends with exit 1, the step fails even when the verbose retry succeeds. Please restructure so the fallback exits only if the second attempt also fails.
- npx nx build @lightprotocol/programs || {
- echo "Failed to build programs, retrying with verbose output..."
- npx nx build @lightprotocol/programs --verbose
- exit 1
- }
+ if ! npx nx build @lightprotocol/programs; then
+ echo "Failed to build programs, retrying with verbose output..."
+ if ! npx nx build @lightprotocol/programs --verbose; then
+ exit 1
+ fi
+ fi
@@
- npx nx build @lightprotocol/program-tests || {
- echo "Failed to build program-tests, retrying with verbose output..."
- npx nx build @lightprotocol/program-tests --verbose
- exit 1
- }
+ if ! npx nx build @lightprotocol/program-tests; then
+ echo "Failed to build program-tests, retrying with verbose output..."
+ if ! npx nx build @lightprotocol/program-tests --verbose; then
+ exit 1
+ fi
+ fiAlso applies to: 243-246
🤖 Prompt for AI Agents
In .github/actions/setup-and-build/action.yml around lines 231-236 (and
similarly 243-246), the fallback block always exits with 1 even when the verbose
retry succeeds because the block ends with exit 1; change the control flow so
the initial build attempts run and, on failure, invoke the verbose retry and
only exit non-zero if that second attempt also fails (for example, run the
verbose build and chain || exit 1 to it, or test its exit status and exit only
on failure).
| if [ ! -d "${PREFIX}/go" ] || [ ! -f "${PREFIX}/go/bin/go" ]; then | ||
| echo "Installing Go..." | ||
| local version=$(get_version "go") | ||
| local suffix=$(get_suffix "go") | ||
| local url="https://go.dev/dl/go${version}.${suffix}.tar.gz" | ||
| download "$url" "${PREFIX}/go.tar.gz" | ||
| tar -xzf "${PREFIX}/go.tar.gz" -C "${PREFIX}" | ||
| rm "${PREFIX}/go.tar.gz" | ||
| log "go" | ||
| else | ||
| echo "Go already installed, skipping..." | ||
| fi |
There was a problem hiding this comment.
Ensure Go updates when the pinned version changes.
The script skips as soon as ${PREFIX}/go/bin/go exists, so when we bump GO_VERSION, anyone who already ran the script keeps the old toolchain. Please compare the current binary’s go version output against the expected version and force a reinstall (removing the existing tree first) when they differ.
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 9-9: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 10-10: Declare and assign separately to avoid masking return values.
(SC2155)
🤖 Prompt for AI Agents
In scripts/devenv/install-go.sh around lines 7 to 18, the script currently skips
installation if ${PREFIX}/go/bin/go exists which prevents upgrades when
GO_VERSION changes; modify it to run the existing binary’s `go version`, parse
the installed version, compare it to the expected pinned version (derived via
get_version "go"), and if they differ remove the existing ${PREFIX}/go tree (or
${PREFIX}/go/bin/go) and proceed with download/extract/install as currently
implemented; keep the existing success logging and ensure the removal happens
only when versions mismatch to avoid unnecessary reinstalls.
| if [ ! -f "${PREFIX}/bin/jq" ]; then | ||
| echo "Installing jq..." | ||
| local version=$(get_version "jq") | ||
| local suffix=$(get_suffix "jq") | ||
| local url="https://github.com/jqlang/jq/releases/download/${version}/${suffix}" | ||
| download "$url" "${PREFIX}/bin/jq" | ||
| log "jq" | ||
| else | ||
| echo "jq already installed, skipping..." | ||
| fi |
There was a problem hiding this comment.
Make jq re-install when the pinned version changes.
Once ${PREFIX}/bin/jq exists we never download again, so bumping JQ_VERSION won’t update local setups. Please read jq --version (normalizing to match JQ_VERSION) and trigger a reinstall whenever the versions differ.
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 9-9: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 10-10: Declare and assign separately to avoid masking return values.
(SC2155)
🤖 Prompt for AI Agents
In scripts/devenv/install-jq.sh around lines 7 to 16, the script currently skips
downloading jq when ${PREFIX}/bin/jq exists; change it to read the installed jq
version (run "${PREFIX}/bin/jq --version"), normalize that output to the same
format returned by get_version "jq" and compare; if versions differ, remove or
overwrite the existing binary and download the pinned release using
get_version/get_suffix and download "$url" "${PREFIX}/bin/jq" (preserve
executable mode), otherwise skip; keep the existing log messages and use the
same download flow so bumping JQ_VERSION triggers a reinstall.
| if [ ! -f "${PREFIX}/bin/pnpm" ]; then | ||
| echo "Installing pnpm..." | ||
| local version=$(get_version "pnpm") | ||
| local suffix=$(get_suffix "pnpm") | ||
| local url="https://github.com/pnpm/pnpm/releases/download/v${version}/pnpm-${suffix}" | ||
| download "$url" "${PREFIX}/bin/pnpm" | ||
| chmod +x "${PREFIX}/bin/pnpm" | ||
| log "pnpm" | ||
| else | ||
| echo "pnpm already installed, skipping..." | ||
| fi |
There was a problem hiding this comment.
Reinstall pnpm when the pinned version is bumped.
We only look for ${PREFIX}/bin/pnpm, so after the first install we’ll keep that binary forever, even if PNPM_VERSION changes. Please inspect pnpm --version (or similar) and trigger a fresh download when it no longer matches the pinned version.
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 9-9: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 10-10: Declare and assign separately to avoid masking return values.
(SC2155)
🤖 Prompt for AI Agents
In scripts/devenv/install-pnpm.sh around lines 7 to 17, the script only checks
for existence of ${PREFIX}/bin/pnpm so it never updates when PNPM_VERSION
changes; modify the logic to run the installed pnpm binary to obtain its version
(e.g. invoke ${PREFIX}/bin/pnpm --version), compare that value against the
pinned version returned by get_version "pnpm", and if they differ remove or
overwrite the existing binary and download the new release before setting
executable permissions and logging; keep the existing fallback that skips
install only when the binary exists and the versions match.
| if [ ! -f "${PREFIX}/bin/solana" ] || [ ! -f "${PREFIX}/bin/solana-keygen" ]; then | ||
| echo "Installing Solana..." | ||
| local version=$(get_version "solana") | ||
| local suffix=$(get_suffix "solana") | ||
| local url="https://github.com/anza-xyz/agave/releases/download/v${version}/solana-release-${suffix}.tar.bz2" | ||
| download "$url" "${PREFIX}/solana-release.tar.bz2" | ||
| tar -xjf "${PREFIX}/solana-release.tar.bz2" -C "${PREFIX}/bin" --strip-components 2 | ||
| rm "${PREFIX}/solana-release.tar.bz2" | ||
| log "solana" | ||
| else | ||
| echo "Solana already installed, skipping..." | ||
| fi |
There was a problem hiding this comment.
Allow Solana to upgrade on version changes.
Presence of solana/solana-keygen alone isn’t enough—after we bump SOLANA_VERSION, the script will still skip and leave the old release in place. Please read the existing binary’s solana --version, compare it to the expected version, and reinstall (after cleaning the old artifacts) whenever they differ.
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 9-9: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 10-10: Declare and assign separately to avoid masking return values.
(SC2155)
🤖 Prompt for AI Agents
scripts/devenv/install-solana.sh lines 7-18: current check only tests for
presence of ${PREFIX}/bin/solana and solana-keygen which prevents upgrades when
SOLANA_VERSION changes; modify the logic to run installation when either binary
is missing OR when the installed binary's version (run ${PREFIX}/bin/solana
--version and parse the version string) differs from get_version "solana"; if
versions differ remove the old artifacts under ${PREFIX}/bin (solana,
solana-keygen, any related files extracted by previous releases), then proceed
to download and extract the new tarball as before and log the installation.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
.github/actions/setup-and-build/action.yml (1)
233-236: Ensure retry blocks exit only when both attempts fail.Both build steps still wrap the initial
npx nx build …in an|| { … exit 1; }block that unconditionally terminates the step even when the verbose retry succeeds. Please bail out only after the second attempt fails (same issue mentioned earlier).- npx nx build @lightprotocol/programs || { - echo "Failed to build programs, retrying with verbose output..." - npx nx build @lightprotocol/programs --verbose - exit 1 - } + if ! npx nx build @lightprotocol/programs; then + echo "Failed to build programs, retrying with verbose output..." + if ! npx nx build @lightprotocol/programs --verbose; then + exit 1 + fi + fi @@ - npx nx build @lightprotocol/program-tests || { - echo "Failed to build program-tests, retrying with verbose output..." - npx nx build @lightprotocol/program-tests --verbose - exit 1 - } + if ! npx nx build @lightprotocol/program-tests; then + echo "Failed to build program-tests, retrying with verbose output..." + if ! npx nx build @lightprotocol/program-tests --verbose; then + exit 1 + fi + fiAlso applies to: 244-247
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/actions/setup-and-build/action.yml(2 hunks)scripts/devenv/install-redis.sh(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
scripts/devenv/install-redis.sh (1)
scripts/devenv/shared.sh (2)
get_version(54-64)log(51-51)
🪛 Shellcheck (0.11.0)
scripts/devenv/install-redis.sh
[warning] 9-9: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 24-24: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
[warning] 28-28: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: system-programs (system-cpi-test-v2-other, ["cargo-test-sbf -p system-cpi-v2-test -- --skip funct...
- GitHub Check: system-programs (system-cpi-test-v2-functional, ["cargo-test-sbf -p system-cpi-v2-test -- functio...
- GitHub Check: system-programs (account-compression, ["cargo-test-sbf -p account-compression-test"])
- GitHub Check: system-programs (random-e2e-test, ["cargo-test-sbf -p e2e-test"])
- GitHub Check: system-programs (system-cpi-test-v2-event, ["cargo-test-sbf -p system-cpi-v2-test -- event::parse"])
- GitHub Check: system-programs (light-system-program, ["cargo-test-sbf -p system-test"])
- GitHub Check: system-programs (light-compressed-token, ["cargo-test-sbf -p compressed-token-test"])
- GitHub Check: system-programs (light-registry, ["cargo-test-sbf -p registry-test"])
- GitHub Check: system-programs (system-cpi-test, ["cargo-test-sbf -p system-cpi-test", "cargo test -p light-syst...
- GitHub Check: cli-v2
- GitHub Check: Forester e2e test
- GitHub Check: cli-v1
- GitHub Check: lint
- GitHub Check: Test program-libs-fast
- GitHub Check: Test concurrent-merkle-tree
- GitHub Check: Test program-libs-slow
- GitHub Check: stateless-js-v2
- GitHub Check: Test batched-merkle-tree-simulate
- GitHub Check: stateless-js-v1
| local url="http://download.redis.io/releases/redis-${version}.tar.gz" | ||
|
|
||
| if ! command -v make >/dev/null 2>&1; then | ||
| echo "Warning: 'make' not found. Redis installation requires build tools." | ||
| if [ "$OS" = "Darwin" ]; then | ||
| echo "Please install Xcode command line tools: xcode-select --install" | ||
| elif [ "$OS" = "Linux" ]; then | ||
| echo "Please install build essentials (Ubuntu: apt-get install build-essential)" | ||
| fi | ||
| echo "Skipping Redis installation..." | ||
| return | ||
| fi | ||
|
|
||
| curl -sSL --retry 5 --retry-delay 10 "$url" | tar -xz -C "${PREFIX}" |
There was a problem hiding this comment.
Switch Redis download to HTTPS.
We hard-code an http:// tarball URL. Anyone on-path can tamper with the archive, yielding a malicious Redis binary that runs with our devenv privileges. Redis publishes the same artifacts over HTTPS—please swap to https:// (and keep --retry etc.) so the installer is not vulnerable to MITM/file-injection attacks.
🤖 Prompt for AI Agents
In scripts/devenv/install-redis.sh around lines 10 to 23, the Redis tarball URL
uses http which is vulnerable to MITM; update the url variable to use
https://download.redis.io/releases/redis-${version}.tar.gz so the download
happens over TLS, and keep the existing curl flags (--retry 5 --retry-delay 10
-sSL) and tar extraction unchanged to preserve retry behavior and extraction
into ${PREFIX}.
| #!/bin/bash | ||
| REDIS_DIR="$(cd "$(dirname "$(dirname "$0")")" && pwd)" | ||
| REDIS_CONF="${REDIS_DIR}/etc/redis.conf" | ||
| REDIS_PID="${REDIS_DIR}/var/redis.pid" | ||
| REDIS_LOG="${REDIS_DIR}/var/redis.log" | ||
|
|
||
| mkdir -p "${REDIS_DIR}/var" | ||
|
|
||
| if [ -f "$REDIS_PID" ] && kill -0 "$(cat "$REDIS_PID")" 2>/dev/null; then | ||
| echo "Redis is already running (PID: $(cat "$REDIS_PID"))" | ||
| exit 0 | ||
| fi | ||
|
|
||
| echo "Starting Redis server..." | ||
| "${REDIS_DIR}/bin/redis-server" "$REDIS_CONF" \ | ||
| --daemonize yes \ | ||
| --pidfile "$REDIS_PID" \ | ||
| --logfile "$REDIS_LOG" | ||
|
|
||
| if [ $? -eq 0 ]; then | ||
| echo "Redis started successfully" | ||
| echo " - PID: $(cat "$REDIS_PID")" | ||
| echo " - Config: $REDIS_CONF" | ||
| echo " - Log: $REDIS_LOG" | ||
| echo " - Connection: redis://localhost:6379" | ||
| else | ||
| echo "Failed to start Redis" | ||
| exit 1 | ||
| fi | ||
| EOF | ||
|
|
||
| cat > "${PREFIX}/bin/redis-stop" << 'EOF' | ||
| #!/bin/bash | ||
| REDIS_DIR="$(cd "$(dirname "$(dirname "$0")")" && pwd)" | ||
| REDIS_PID="${REDIS_DIR}/var/redis.pid" | ||
|
|
||
| if [ ! -f "$REDIS_PID" ]; then | ||
| echo "Redis PID file not found. Redis may not be running." | ||
| exit 1 | ||
| fi | ||
|
|
||
| PID=$(cat "$REDIS_PID") | ||
| if kill -0 "$PID" 2>/dev/null; then | ||
| echo "Stopping Redis (PID: $PID)..." | ||
| kill "$PID" | ||
|
|
||
| for i in {1..10}; do | ||
| if ! kill -0 "$PID" 2>/dev/null; then | ||
| rm -f "$REDIS_PID" | ||
| echo "Redis stopped successfully" | ||
| exit 0 | ||
| fi | ||
| sleep 1 | ||
| done | ||
|
|
||
| echo "Forcing Redis shutdown..." | ||
| kill -9 "$PID" 2>/dev/null | ||
| rm -f "$REDIS_PID" | ||
| echo "Redis stopped" | ||
| else | ||
| echo "Redis process not found" | ||
| rm -f "$REDIS_PID" | ||
| fi | ||
| EOF | ||
|
|
||
| cat > "${PREFIX}/bin/redis-status" << 'EOF' | ||
| #!/bin/bash | ||
| REDIS_DIR="$(cd "$(dirname "$(dirname "$0")")" && pwd)" | ||
| REDIS_PID="${REDIS_DIR}/var/redis.pid" | ||
|
|
||
| if [ -f "$REDIS_PID" ] && kill -0 "$(cat "$REDIS_PID")" 2>/dev/null; then | ||
| PID=$(cat "$REDIS_PID") | ||
| echo "Redis is running (PID: $PID)" | ||
| echo "Connection: redis://localhost:6379" | ||
|
|
||
| if command -v "${REDIS_DIR}/bin/redis-cli" >/dev/null 2>&1; then | ||
| if "${REDIS_DIR}/bin/redis-cli" ping >/dev/null 2>&1; then | ||
| echo "Status: HEALTHY" | ||
| else | ||
| echo "Status: UNHEALTHY (process running but not responding)" | ||
| fi | ||
| fi | ||
| else | ||
| echo "Redis is not running" | ||
| fi |
There was a problem hiding this comment.
Fix REDIS_DIR detection in helper scripts.
Each helper calls dirname "$(dirname "$0")". When the script is launched via $PATH (the common case), $0 has no slash and dirname collapses to ., so REDIS_DIR becomes the caller’s CWD. All paths (config, logs, pid) point to the wrong place and Redis will not start/stop/status correctly. Please resolve the actual script directory (e.g. normalize $0 with command -v or use a ${BASH_SOURCE[0]} + command -v fallback) before walking up to ...
🤖 Prompt for AI Agents
scripts/devenv/install-redis.sh lines 52-136: the helper scripts compute
REDIS_DIR using dirname "$(dirname "$0")", which breaks when the script is
invoked via $PATH (no slash in $0); replace that logic with resolving the actual
script file path first (prefer ${BASH_SOURCE[0]} when running under bash, else
fallback to command -v "$0" and resolve symlinks/absolute path with readlink -f
or equivalent), then derive REDIS_DIR by taking the parent of the parent
directory of that resolved script path and using cd ... && pwd to normalize;
apply this change to all three helper script headers so REDIS_CONF, REDIS_PID
and REDIS_LOG point to the correct locations regardless of invocation method.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
cli/package.json (1)
168-175: Addressed: Nx cache now covers prover and scriptsgnark-prover and scripts included in build-ci inputs fix the prior stale-cache risk.
cli/scripts/buildProver.sh (1)
11-22: Addressed: POSIX shell parsing and new --ci optionSwitched to POSIX-compatible [ ] and added CI_MODE flag; argument parsing is now portable with sh.
.github/actions/setup-and-build/action.yml (1)
229-247: Retry logic no longer hard-fails after a successful retrySwitch to if/then with a verbose second attempt resolves the previous control-flow bug.
🧹 Nitpick comments (6)
scripts/devenv/install-rust.sh (3)
1-4: Add strict bash flags for safer installs.Fail fast and catch unset vars.
#!/usr/bin/env bash +set -euo pipefail + SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" source "${SCRIPT_DIR}/shared.sh"
15-15: Include the toolchain in the install log entry.Improves traceability of which version was provisioned.
- log "rust" + log "rust:${RUST_VERSION}"
11-14: Add error handling and install Rust toolchains with minimal profile
- Add
set -euo pipefailat the top ofscripts/devenv/install-rust.sh.- Use
--default-toolchain nonein the installer to skip the implicit stable install.- Replace
rustup installwithrustup toolchain installand add--profile minimalfor both${RUST_VERSION}andnightly(required bylint.sh/format.sh).- Keep only the clippy component for
${RUST_VERSION}.+ set -euo pipefail … - curl --retry 5 --retry-delay 10 --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh -s -- -y --no-modify-path + curl --retry 5 --retry-delay 10 --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh -s -- -y --no-modify-path --default-toolchain none export PATH="${PREFIX}/cargo/bin:${PATH}" - rustup install ${RUST_VERSION} nightly - rustup component add --toolchain ${RUST_VERSION} clippy + rustup toolchain install "${RUST_VERSION}" --profile minimal + rustup toolchain install nightly --profile minimal + rustup component add --toolchain "${RUST_VERSION}" clippy.github/workflows/sdk-tests.yml (2)
55-56: Matrix 'packages' appears unused in this jobNo step references matrix.packages. Consider removing to avoid confusion.
73-76: Gate or remove CLI build to save timeThis job’s tests don’t appear to use the CLI. Either drop the step or run it only when needed (e.g., for sub-tests that require it).
Example gating by program:
- - name: Build CLI (CI mode - Linux x64 only) - run: | - npx nx build-ci @lightprotocol/zk-compression-cli + - name: Build CLI (CI mode - Linux x64 only) + if: matrix.program == 'sdk-anchor-test-program' # adjust as needed + run: npx nx build-ci @lightprotocol/zk-compression-cli.github/actions/setup-and-build/action.yml (1)
54-59: Honor documented skip-components for GoInput doc mentions skipping “go”, but Setup Go always runs. Add a guard to align behavior.
- - name: Setup Go + - name: Setup Go + if: "!contains(inputs.skip-components, 'go')" uses: actions/setup-go@v5 with: go-version: ${{ steps.versions.outputs.go }} cache-dependency-path: prover/server/go.sumOptionally extend similar guards for solana/anchor/photon if you plan to skip them too.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.github/actions/setup-and-build/action.yml(2 hunks).github/workflows/light-system-programs-tests.yml(2 hunks).github/workflows/lint.yml(1 hunks).github/workflows/rust.yml(3 hunks).github/workflows/sdk-tests.yml(2 hunks)cli/package.json(2 hunks)cli/scripts/buildProver.sh(2 hunks)scripts/devenv/install-rust.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/rust.yml
🧰 Additional context used
🧬 Code graph analysis (1)
scripts/devenv/install-rust.sh (1)
scripts/devenv/shared.sh (1)
log(51-51)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
- GitHub Check: system-programs (random-e2e-test, ["cargo-test-sbf -p e2e-test"])
- GitHub Check: system-programs (light-compressed-token, ["cargo-test-sbf -p compressed-token-test"])
- GitHub Check: system-programs (system-cpi-test, ["cargo-test-sbf -p system-cpi-test", "cargo test -p light-syst...
- GitHub Check: system-programs (system-cpi-test-v2-functional, ["cargo-test-sbf -p system-cpi-v2-test -- functio...
- GitHub Check: system-programs (light-system-program, ["cargo-test-sbf -p system-test"])
- GitHub Check: system-programs (account-compression-and-registry, ["cargo-test-sbf -p account-compression-test",...
- GitHub Check: stateless-js-v1
- GitHub Check: stateless-js-v2
- GitHub Check: cli-v2
- GitHub Check: Forester e2e test
- GitHub Check: cli-v1
- GitHub Check: Test program-libs-fast
- GitHub Check: Test program-libs-slow
- GitHub Check: Test batched-merkle-tree-simulate
- GitHub Check: lint
🔇 Additional comments (4)
.github/workflows/lint.yml (1)
31-33: LGTM: skipping proving-keys and redis for lint keeps the job leanChange is safe and reduces setup time.
cli/package.json (1)
113-118: CI scripts wiring looks correctadd-bins-ci and build-ci align with the new --ci path in buildProver.sh.
cli/scripts/buildProver.sh (1)
84-101: CI-only Linux x64 build path is clear and safeConditional build logic matches the new build-ci target usage.
.github/actions/setup-and-build/action.yml (1)
30-44: Version-driven setup is solidCentralizing versions via scripts/devenv/versions.sh improves reproducibility.
| IFS=',' read -r -a sub_tests <<< "${{ join(fromJSON(matrix['sub-tests']), ', ') }}" | ||
| for subtest in "${sub_tests[@]}" | ||
| do | ||
| echo "$subtest" | ||
|
|
||
| if [ "$subtest" == "cargo-test-sbf -p compressed-token-test" ]; then | ||
| npx nx build-ci @lightprotocol/zk-compression-cli | ||
| fi | ||
| RUSTFLAGS="-D warnings" eval "$subtest" | ||
| if [ "$subtest" == "cargo-test-sbf -p e2e-test" ]; then |
There was a problem hiding this comment.
Fix sub-test splitting: leading spaces break the equality check (CLI never builds)
join with ", " then IFS=',' introduces a leading space in each element, so [ "$subtest" == "cargo-test-sbf -p compressed-token-test" ] won’t match and CLI won’t build.
Apply either or both fixes:
- Remove the space in the join delimiter:
- IFS=',' read -r -a sub_tests <<< "${{ join(fromJSON(matrix['sub-tests']), ', ') }}"
+ IFS=',' read -r -a sub_tests <<< "${{ join(fromJSON(matrix['sub-tests']), ',') }}"- Trim each subtest before using it:
for subtest in "${sub_tests[@]}"
do
- echo "$subtest"
- if [ "$subtest" == "cargo-test-sbf -p compressed-token-test" ]; then
+ subtest="$(echo "$subtest" | xargs)" # trim
+ echo "$subtest"
+ if [ "$subtest" = "cargo-test-sbf -p compressed-token-test" ]; then
npx nx build-ci @lightprotocol/zk-compression-cli
fi
RUSTFLAGS="-D warnings" eval "$subtest"Note: also switched to single '=' for POSIX test.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| IFS=',' read -r -a sub_tests <<< "${{ join(fromJSON(matrix['sub-tests']), ', ') }}" | |
| for subtest in "${sub_tests[@]}" | |
| do | |
| echo "$subtest" | |
| if [ "$subtest" == "cargo-test-sbf -p compressed-token-test" ]; then | |
| npx nx build-ci @lightprotocol/zk-compression-cli | |
| fi | |
| RUSTFLAGS="-D warnings" eval "$subtest" | |
| if [ "$subtest" == "cargo-test-sbf -p e2e-test" ]; then | |
| IFS=',' read -r -a sub_tests <<< "${{ join(fromJSON(matrix['sub-tests']), ',') }}" | |
| for subtest in "${sub_tests[@]}" | |
| do | |
| subtest="$(echo "$subtest" | xargs)" # trim leading/trailing whitespace | |
| echo "$subtest" | |
| if [ "$subtest" = "cargo-test-sbf -p compressed-token-test" ]; then | |
| npx nx build-ci @lightprotocol/zk-compression-cli | |
| fi | |
| RUSTFLAGS="-D warnings" eval "$subtest" | |
| if [ "$subtest" == "cargo-test-sbf -p e2e-test" ]; then |
🤖 Prompt for AI Agents
.github/workflows/light-system-programs-tests.yml lines 93-101: the split
produces leading spaces because the matrix join uses ", " so IFS=',' leaves a
leading space in each element causing the string equality checks to fail; fix by
changing the join delimiter to "," (no space) or trim whitespace from each
subtest before comparing/using it, and use POSIX-compatible single '=' in the
test expressions; update the script to either produce comma-separated items
without spaces or run a trim (e.g., strip leading/trailing whitespace) on
"$subtest" prior to the echo, comparisons and eval (and replace '==' with '=' in
the [ ] tests).
| if [ ! -d "${PREFIX}/rustup" ] || [ ! -d "${PREFIX}/cargo" ] || [ ! -f "${PREFIX}/cargo/bin/cargo" ]; then | ||
| echo "Installing Rust..." | ||
| export RUSTUP_HOME="${PREFIX}/rustup" | ||
| export CARGO_HOME="${PREFIX}/cargo" | ||
| curl --retry 5 --retry-delay 10 --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh -s -- -y --no-modify-path | ||
| export PATH="${PREFIX}/cargo/bin:${PATH}" | ||
| rustup install ${RUST_VERSION} nightly | ||
| rustup component add --toolchain ${RUST_VERSION} clippy | ||
| log "rust" | ||
| else | ||
| echo "Rust already installed, skipping..." | ||
| fi |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Make the installer idempotent and version‑pin aware even when Rust is preinstalled.
If cargo exists, the script skips without ensuring the requested toolchain/clippy is present. Ensure the pinned toolchain and clippy are installed in the skip path.
- if [ ! -d "${PREFIX}/rustup" ] || [ ! -d "${PREFIX}/cargo" ] || [ ! -f "${PREFIX}/cargo/bin/cargo" ]; then
+ if [ ! -d "${PREFIX}/rustup" ] || [ ! -d "${PREFIX}/cargo" ] || [ ! -f "${PREFIX}/cargo/bin/cargo" ]; then
echo "Installing Rust..."
export RUSTUP_HOME="${PREFIX}/rustup"
export CARGO_HOME="${PREFIX}/cargo"
curl --retry 5 --retry-delay 10 --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh -s -- -y --no-modify-path
export PATH="${PREFIX}/cargo/bin:${PATH}"
- rustup install ${RUST_VERSION} nightly
- rustup component add --toolchain ${RUST_VERSION} clippy
+ rustup toolchain install "${RUST_VERSION}" --profile minimal
+ rustup component add --toolchain "${RUST_VERSION}" clippy
log "rust"
else
- echo "Rust already installed, skipping..."
+ echo "Rust already installed, ensuring pinned toolchain..."
+ export PATH="${PREFIX}/cargo/bin:${PATH}"
+ rustup toolchain install "${RUST_VERSION}" --profile minimal
+ rustup component add --toolchain "${RUST_VERSION}" clippy
fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if [ ! -d "${PREFIX}/rustup" ] || [ ! -d "${PREFIX}/cargo" ] || [ ! -f "${PREFIX}/cargo/bin/cargo" ]; then | |
| echo "Installing Rust..." | |
| export RUSTUP_HOME="${PREFIX}/rustup" | |
| export CARGO_HOME="${PREFIX}/cargo" | |
| curl --retry 5 --retry-delay 10 --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh -s -- -y --no-modify-path | |
| export PATH="${PREFIX}/cargo/bin:${PATH}" | |
| rustup install ${RUST_VERSION} nightly | |
| rustup component add --toolchain ${RUST_VERSION} clippy | |
| log "rust" | |
| else | |
| echo "Rust already installed, skipping..." | |
| fi | |
| if [ ! -d "${PREFIX}/rustup" ] || [ ! -d "${PREFIX}/cargo" ] || [ ! -f "${PREFIX}/cargo/bin/cargo" ]; then | |
| echo "Installing Rust..." | |
| export RUSTUP_HOME="${PREFIX}/rustup" | |
| export CARGO_HOME="${PREFIX}/cargo" | |
| curl --retry 5 --retry-delay 10 --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh -s -- -y --no-modify-path | |
| export PATH="${PREFIX}/cargo/bin:${PATH}" | |
| rustup toolchain install "${RUST_VERSION}" --profile minimal | |
| rustup component add --toolchain "${RUST_VERSION}" clippy | |
| log "rust" | |
| else | |
| echo "Rust already installed, ensuring pinned toolchain..." | |
| export PATH="${PREFIX}/cargo/bin:${PATH}" | |
| rustup toolchain install "${RUST_VERSION}" --profile minimal | |
| rustup component add --toolchain "${RUST_VERSION}" clippy | |
| fi |
🤖 Prompt for AI Agents
In scripts/devenv/install-rust.sh around lines 7 to 18, the script currently
skips installation if cargo exists but doesn't ensure the requested RUST_VERSION
toolchain and clippy are present; modify the skip path so that if cargo (or
rustup) is present you still export RUSTUP_HOME and CARGO_HOME as above, ensure
PATH includes ${PREFIX}/cargo/bin, and then run rustup install ${RUST_VERSION}
(and rustup component add --toolchain ${RUST_VERSION} clippy); if rustup is
missing in that branch, run the rustup installer first, then install the pinned
toolchain and clippy so the script is idempotent and version‑pin aware even when
Rust is already installed.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.github/actions/setup-and-build/action.yml(3 hunks).github/workflows/cli-v1.yml(1 hunks).github/workflows/cli-v2.yml(1 hunks).github/workflows/js-v2.yml(1 hunks).github/workflows/js.yml(1 hunks).github/workflows/light-system-programs-tests.yml(2 hunks).github/workflows/lint.yml(1 hunks).github/workflows/rust.yml(3 hunks).github/workflows/sdk-tests.yml(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- .github/workflows/sdk-tests.yml
- .github/workflows/js.yml
- .github/workflows/lint.yml
- .github/workflows/js-v2.yml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
- GitHub Check: stateless-js-v2
- GitHub Check: system-programs (system-cpi-test, ["cargo-test-sbf -p system-cpi-test", "cargo test -p light-syst...
- GitHub Check: system-programs (account-compression-and-registry, ["cargo-test-sbf -p account-compression-test",...
- GitHub Check: system-programs (light-compressed-token, ["cargo-test-sbf -p compressed-token-test"])
- GitHub Check: system-programs (system-cpi-test-v2-functional, ["cargo-test-sbf -p system-cpi-v2-test -- functio...
- GitHub Check: system-programs (light-system-program, ["cargo-test-sbf -p system-test"])
- GitHub Check: system-programs (random-e2e-test, ["cargo-test-sbf -p e2e-test"])
- GitHub Check: Test program-libs-fast
- GitHub Check: Test batched-merkle-tree-simulate
- GitHub Check: Test program-libs-slow
- GitHub Check: stateless-js-v1
- GitHub Check: cli-v1
- GitHub Check: lint
- GitHub Check: cli-v2
- GitHub Check: Forester e2e test
ananas-block
force-pushed
the
jorrit/perf-optimize-ci-build-time
branch
from
a53994e to
583530c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/actions/setup-and-build/action.yml (1)
5-8: Implement skip-components gating consistently (Go/Solana/Anchor/Photon).The input advertises skipping components, but only disk-cleanup and proving-keys are gated. Go, Solana, Anchor, Photon steps still run. Align behavior to input by adding if guards.
Apply diffs:
@@ - - name: Setup Go + - name: Setup Go + if: "!contains(inputs.skip-components, 'go')" uses: actions/setup-go@v5 @@ - - name: Cache Solana CLI + - name: Cache Solana CLI + if: "!contains(inputs.skip-components, 'solana')" id: cache-solana @@ - - name: Install Solana CLI - if: steps.cache-solana.outputs.cache-hit != 'true' + - name: Install Solana CLI + if: "!contains(inputs.skip-components, 'solana') && steps.cache-solana.outputs.cache-hit != 'true'" shell: bash @@ - - name: Cache Anchor CLI + - name: Cache Anchor CLI + if: "!contains(inputs.skip-components, 'anchor')" id: cache-anchor @@ - - name: Install Anchor CLI - if: steps.cache-anchor.outputs.cache-hit != 'true' + - name: Install Anchor CLI + if: "!contains(inputs.skip-components, 'anchor') && steps.cache-anchor.outputs.cache-hit != 'true'" shell: bash @@ - - name: Cache Photon indexer + - name: Cache Photon indexer + if: "!contains(inputs.skip-components, 'photon')" id: cache-photon @@ - - name: Install Photon indexer - if: steps.cache-photon.outputs.cache-hit != 'true' + - name: Install Photon indexer + if: "!contains(inputs.skip-components, 'photon') && steps.cache-photon.outputs.cache-hit != 'true'" shell: bash @@ - - name: Generate Solana keypair + - name: Generate Solana keypair + if: "!contains(inputs.skip-components, 'solana')" shell: bash @@ - - name: Copy spl_noop.so to target/deploy + - name: Copy spl_noop.so to target/deploy + if: "!contains(inputs.skip-components, 'solana')" shell: bashAlso applies to: 52-57, 69-84, 85-98, 99-110, 175-181, 183-188
♻️ Duplicate comments (1)
.github/actions/setup-and-build/action.yml (1)
216-221: Fallback build logic fixed; resolves prior always-fail behavior.The retry now only fails the step if the verbose attempt fails. Good.
Also applies to: 226-230
🧹 Nitpick comments (1)
.github/actions/setup-and-build/action.yml (1)
14-21: Gate Linux-only steps by OS to avoid non-Ubuntu failures.These steps use apt-get and Linux-specific paths. Add an OS condition.
- - name: Free Disk Space (Ubuntu) - if: "!contains(inputs.skip-components, 'disk-cleanup')" + - name: Free Disk Space (Ubuntu) + if: "runner.os == 'Linux' && !contains(inputs.skip-components, 'disk-cleanup')" @@ - - name: Install system dependencies + - name: Install system dependencies + if: "runner.os == 'Linux'" shell: bash @@ - - name: Generate Solana keypair + - name: Generate Solana keypair + if: "runner.os == 'Linux' && !contains(inputs.skip-components, 'solana')" shell: bashConfirm this action is only invoked on ubuntu-latest; otherwise these guards are necessary.
Also applies to: 22-27, 175-181
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/actions/setup-and-build/action.yml(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
- GitHub Check: cli-v1
- GitHub Check: stateless-js-v1
- GitHub Check: cli-v2
- GitHub Check: system-programs (random-e2e-test, ["cargo-test-sbf -p e2e-test"])
- GitHub Check: system-programs (account-compression-and-registry, ["cargo-test-sbf -p account-compression-test",...
- GitHub Check: system-programs (system-cpi-test, ["cargo-test-sbf -p system-cpi-test", "cargo test -p light-syst...
- GitHub Check: system-programs (light-compressed-token, ["cargo-test-sbf -p compressed-token-test"])
- GitHub Check: system-programs (light-system-program, ["cargo-test-sbf -p system-test"])
- GitHub Check: system-programs (system-cpi-test-v2-functional, ["cargo-test-sbf -p system-cpi-v2-test -- functio...
- GitHub Check: Test program-libs-slow
- GitHub Check: Test program-libs-fast
- GitHub Check: Test batched-merkle-tree-simulate
- GitHub Check: Forester e2e test
- GitHub Check: stateless-js-v2
- GitHub Check: lint
| - name: Cache proving keys | ||
| if: "!contains(inputs.skip-components, 'proving-keys')" | ||
| id: cache-keys | ||
| uses: actions/cache@v4 | ||
| with: | ||
| path: prover/server/proving-keys | ||
| key: ${{ runner.os }}-proving-keys-${{ hashFiles('prover/server/scripts/download_keys.sh') }} | ||
|
|
||
| echo "=== Environment validation complete ===" | ||
| - name: Download proving keys | ||
| if: "!contains(inputs.skip-components, 'proving-keys') && steps.cache-keys.outputs.cache-hit != 'true'" | ||
| shell: bash | ||
| run: bash scripts/devenv/download-gnark-keys.sh | ||
|
|
There was a problem hiding this comment.
Fix proving-keys cache key: wrong file hashed causes stale cache.
You restore/save based on prover/server/scripts/download_keys.sh, but the download step uses scripts/devenv/download-gnark-keys.sh. Cache won’t bust when the invoked script changes.
- key: ${{ runner.os }}-proving-keys-${{ hashFiles('prover/server/scripts/download_keys.sh') }}
+ key: ${{ runner.os }}-proving-keys-${{ hashFiles('scripts/devenv/download-gnark-keys.sh') }}Optionally, also hash a manifest/version file for the keys if available (more stable than script changes).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Cache proving keys | |
| if: "!contains(inputs.skip-components, 'proving-keys')" | |
| id: cache-keys | |
| uses: actions/cache@v4 | |
| with: | |
| path: prover/server/proving-keys | |
| key: ${{ runner.os }}-proving-keys-${{ hashFiles('prover/server/scripts/download_keys.sh') }} | |
| echo "=== Environment validation complete ===" | |
| - name: Download proving keys | |
| if: "!contains(inputs.skip-components, 'proving-keys') && steps.cache-keys.outputs.cache-hit != 'true'" | |
| shell: bash | |
| run: bash scripts/devenv/download-gnark-keys.sh | |
| - name: Cache proving keys | |
| if: "!contains(inputs.skip-components, 'proving-keys')" | |
| id: cache-keys | |
| uses: actions/cache@v4 | |
| with: | |
| path: prover/server/proving-keys | |
| key: ${{ runner.os }}-proving-keys-${{ hashFiles('scripts/devenv/download-gnark-keys.sh') }} | |
| - name: Download proving keys | |
| if: "!contains(inputs.skip-components, 'proving-keys') && steps.cache-keys.outputs.cache-hit != 'true'" | |
| shell: bash | |
| run: bash scripts/devenv/download-gnark-keys.sh |
🤖 Prompt for AI Agents
.github/actions/setup-and-build/action.yml lines 111-123: the cache key hashes
the wrong script (prover/server/scripts/download_keys.sh) while the actual
download step runs scripts/devenv/download-gnark-keys.sh, causing stale cache;
update the cache key to hash the actual download script path
scripts/devenv/download-gnark-keys.sh so the key changes when that script
changes, and optionally include a stable manifest/version file (if one exists)
in the hash to decouple cache invalidation from script edits.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.github/workflows/cli-v1.yml(2 hunks).github/workflows/cli-v2.yml(2 hunks).github/workflows/forester-tests.yml(3 hunks).github/workflows/js-v2.yml(2 hunks).github/workflows/js.yml(2 hunks).github/workflows/light-system-programs-tests.yml(2 hunks).github/workflows/lint.yml(1 hunks).github/workflows/rust.yml(3 hunks).github/workflows/sdk-tests.yml(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- .github/workflows/js-v2.yml
- .github/workflows/js.yml
- .github/workflows/lint.yml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
- GitHub Check: system-programs (light-system-program, ["cargo-test-sbf -p system-test"])
- GitHub Check: system-programs (light-compressed-token, ["cargo-test-sbf -p compressed-token-test"])
- GitHub Check: system-programs (system-cpi-test, ["cargo-test-sbf -p system-cpi-test", "cargo test -p light-syst...
- GitHub Check: system-programs (system-cpi-test-v2-functional, ["cargo-test-sbf -p system-cpi-v2-test -- functio...
- GitHub Check: system-programs (account-compression-and-registry, ["cargo-test-sbf -p account-compression-test",...
- GitHub Check: system-programs (random-e2e-test, ["cargo-test-sbf -p e2e-test"])
- GitHub Check: Test batched-merkle-tree-simulate
- GitHub Check: Test program-libs-fast
- GitHub Check: Test program-libs-slow
- GitHub Check: stateless-js-v1
- GitHub Check: Forester e2e test
- GitHub Check: cli-v2
- GitHub Check: lint
- GitHub Check: stateless-js-v2
- GitHub Check: cli-v1
ananas-block
force-pushed
the
jorrit/perf-optimize-ci-build-time
branch
from
40c9d91 to
c7ba64c
Compare
ananas-block
force-pushed
the
jorrit/perf-optimize-ci-build-time
branch
from
c7ba64c to
5613c2d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
.github/workflows/light-system-programs-tests.yml (1)
90-98: Trim sub-test entries before comparison to avoid missing the e2e branch.
join(..., ', ')injects a leading space, so$subtestbecomes" cargo-test-sbf -p e2e-test"and the equality check never fires; the extratest_10_allrun is skipped. Either drop the space in the join delimiter or trim each entry before theif.- IFS=',' read -r -a sub_tests <<< "${{ join(fromJSON(matrix['sub-tests']), ', ') }}" + IFS=',' read -r -a sub_tests <<< "${{ join(fromJSON(matrix['sub-tests']), ',') }}" for subtest in "${sub_tests[@]}" do - echo "$subtest" + subtest="$(echo "$subtest" | xargs)" + echo "$subtest" RUSTFLAGS="-D warnings" eval "$subtest" if [ "$subtest" == "cargo-test-sbf -p e2e-test" ]; then.github/actions/setup-and-build/action.yml (1)
120-123: Proving-keys cache never invalidates on script changes.The cache key hashes
prover/server/scripts/download_keys.sh, but the download step runsscripts/devenv/download-gnark-keys.sh. Updates to the actual script won’t bump the key, so CI can serve stale proving keys. Hash the invoked script (or another version manifest) instead.- key: ${{ runner.os }}-proving-keys-${{ inputs.cache-suffix }}${{ inputs.cache-suffix && '-' || '' }}${{ hashFiles('prover/server/scripts/download_keys.sh') }} + key: ${{ runner.os }}-proving-keys-${{ inputs.cache-suffix }}${{ inputs.cache-suffix && '-' || '' }}${{ hashFiles('scripts/devenv/download-gnark-keys.sh') }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
.github/actions/setup-and-build/action.yml(2 hunks).github/workflows/cli-v1.yml(1 hunks).github/workflows/cli-v2.yml(1 hunks).github/workflows/forester-tests.yml(2 hunks).github/workflows/js-v2.yml(1 hunks).github/workflows/js.yml(1 hunks).github/workflows/light-system-programs-tests.yml(2 hunks).github/workflows/lint.yml(1 hunks).github/workflows/rust.yml(3 hunks).github/workflows/sdk-tests.yml(2 hunks)cli/package.json(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- .github/workflows/lint.yml
- .github/workflows/forester-tests.yml
- .github/workflows/js.yml
- .github/workflows/cli-v1.yml
- .github/workflows/js-v2.yml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
- GitHub Check: stateless-js-v2
- GitHub Check: stateless-js-v1
- GitHub Check: system-programs (system-cpi-test, ["cargo-test-sbf -p system-cpi-test", "cargo test -p light-syst...
- GitHub Check: cli-v1
- GitHub Check: cli-v2
- GitHub Check: system-programs (light-system-program, ["cargo-test-sbf -p system-test"])
- GitHub Check: system-programs (system-cpi-test-v2-functional, ["cargo-test-sbf -p system-cpi-v2-test -- functio...
- GitHub Check: system-programs (light-compressed-token, ["cargo-test-sbf -p compressed-token-test"])
- GitHub Check: system-programs (random-e2e-test, ["cargo-test-sbf -p e2e-test"])
- GitHub Check: system-programs (account-compression-and-registry, ["cargo-test-sbf -p account-compression-test",...
- GitHub Check: lint
- GitHub Check: Forester e2e test
- GitHub Check: Test program-libs-fast
- GitHub Check: Test program-libs-slow
- GitHub Check: Test batched-merkle-tree-simulate
ananas-block
force-pushed
the
jorrit/perf-optimize-ci-build-time
branch
2 times, most recently
from
18059fe to
5cdaf2b
Compare
ananas-block
force-pushed
the
jorrit/perf-optimize-ci-build-time
branch
from
5cdaf2b to
f856edd
Compare
ananas-block
force-pushed
the
jorrit/perf-optimize-ci-build-time
branch
from
e9dd4ff to
b6dcf46
Compare
ananas-block
force-pushed
the
jorrit/perf-optimize-ci-build-time
branch
from
2d7e806 to
452bd87
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
.github/workflows/js.yml (2)
68-81: Retry loop is fine; consider adding failure logs and a verbose retry.Loop semantics are correct. For easier debugging:
- On the final attempt, run with verbosity (e.g., add --verbose).
- Add a follow-up step to print relevant logs on failure (parity with v2 workflow’s prover logs step).
Example tweak within the loop:
- until npx nx test-ci @lightprotocol/stateless.js; do + until npx nx test-ci @lightprotocol/stateless.js; do attempt=$((attempt + 1)) if [ $attempt -gt $max_attempts ]; then echo "Tests failed after $max_attempts attempts" exit 1 fi echo "Attempt $attempt/$max_attempts failed, retrying..." - sleep 5 + sleep 5 + # add verbosity on the last attempt + if [ $attempt -eq $max_attempts ]; then + export NX_VERBOSE_LOGGING=true + fi doneOptional: add a final “Display prover logs on failure” step similar to js-v2.yml.
84-96: Mirror retry improvements for compressed-token tests.Same suggestions as above: add verbose on last attempt and a failure-log step for parity.
.github/workflows/js-v2.yml (2)
68-80: Retry loop is correct; add verbosity on last attempt.Consider enabling verbose logs on the final attempt to aid debugging.
- until npx nx test-ci @lightprotocol/stateless.js; do + until npx nx test-ci @lightprotocol/stateless.js; do attempt=$((attempt + 1)) if [ $attempt -gt $max_attempts ]; then echo "Tests failed after $max_attempts attempts" exit 1 fi echo "Attempt $attempt/$max_attempts failed, retrying..." sleep 5 + if [ $attempt -eq $max_attempts ]; then + export NX_VERBOSE_LOGGING=true + fi done
84-96: Apply the same verbose-on-last-attempt pattern here.Improves failure diagnosability for compressed-token tests.
js/stateless.js/package.json (1)
117-118: Deduplicate build logic; make build-ci a thin alias to buildbuild and build-ci are identical. Alias build-ci to build to avoid drift and simplify maintenance. Also, the POSIX shell conditional may fail on Windows runners; aliasing helps, and if Windows is needed, consider a Node-based switch or cross-env.
Apply this diff:
- "build-ci": "if [ \"$LIGHT_PROTOCOL_VERSION\" = \"V2\" ]; then LIGHT_PROTOCOL_VERSION=V2 pnpm build:bundle; else LIGHT_PROTOCOL_VERSION=V1 pnpm build:bundle; fi", + "build-ci": "pnpm run build",If Windows support is required later, replace the conditional in "build" with a Node one-liner:
- "build": "if [ \"$LIGHT_PROTOCOL_VERSION\" = \"V2\" ]; then LIGHT_PROTOCOL_VERSION=V2 pnpm build:bundle; else LIGHT_PROTOCOL_VERSION=V1 pnpm build:bundle; fi", + "build": "node -e \"process.exitCode=0;const v=process.env.LIGHT_PROTOCOL_VERSION==='V2'?'build:v2':'build:v1';require('child_process').spawnSync('pnpm',['run',v],{stdio:'inherit',shell:false});\"",Please confirm CI runs only on Linux/macOS, or if Windows support is needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/js-v2.yml(1 hunks).github/workflows/js.yml(1 hunks)cli/package.json(3 hunks)js/compressed-token/package.json(2 hunks)js/stateless.js/package.json(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- cli/package.json
- js/compressed-token/package.json
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (17)
- GitHub Check: system-programs (system-cpi-test-v2-functional-read-only, ["cargo-test-sbf -p system-cpi-v2-test ...
- GitHub Check: system-programs (system-cpi-test, ["cargo-test-sbf -p system-cpi-test", "cargo test -p light-syst...
- GitHub Check: system-programs (system-cpi-test-v2-functional-account-infos, ["cargo-test-sbf -p system-cpi-v2-t...
- GitHub Check: system-programs (compressed-token-batched-tree, ["cargo-test-sbf -p compressed-token-test -- test...
- GitHub Check: system-programs (light-system-program-compression, ["cargo-test-sbf -p system-test -- test_with_c...
- GitHub Check: system-programs (compressed-token-and-e2e, ["cargo-test-sbf -p compressed-token-test -- --skip te...
- GitHub Check: system-programs (account-compression-and-registry, ["cargo-test-sbf -p account-compression-test",...
- GitHub Check: system-programs (light-system-program-address, ["cargo-test-sbf -p system-test -- test_with_addre...
- GitHub Check: lint
- GitHub Check: cli-v2
- GitHub Check: stateless-js-v2
- GitHub Check: Test program-libs-slow
- GitHub Check: Test program-libs-fast
- GitHub Check: Test batched-merkle-tree-simulate
- GitHub Check: Forester e2e test
- GitHub Check: cli-v1
- GitHub Check: stateless-js-v1
🔇 Additional comments (4)
.github/workflows/js-v2.yml (2)
49-51: Composite action inputs and workflow usage are correct
skip-components and cache-suffix are declared in setup-and-build/action.yml and consistently passed across all workflows.
62-65: CLI test coverage confirmed CI workflow already runsnpx nx test-ci @lightprotocol/zk-compression-cli(.github/workflows/cli-v2.yml:68)..github/workflows/js.yml (2)
49-51: Composite action inputsskip-componentsandcache-suffixare defined in.github/actions/setup-and-build/action.ymland used consistently across all workflows.
62-65: nx build-ci usage is appropriate; CLI tests remain covered
CLI tests execute in.github/workflows/cli-v1.ymland.github/workflows/cli-v2.ymlvianpx nx test-ci @lightprotocol/zk-compression-cli.
2 participants
Summary by CodeRabbit