ci: add --no-system-deps flag to setup script and use in jobs#234
Conversation
📝 WalkthroughWalkthroughAdd a --no-system-deps flag to scripts/setup_dependencies.sh to skip interactive system-package installs, update six CI "Setup External Dependencies" steps to make the script executable and run it with that flag, and add a Bash test that verifies the skip message. ChangesCentralize external dependency setup
🎯 3 (Moderate) | ⏱️ ~20 minutes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/setup_dependencies.sh (1)
107-107: 💤 Low valueConsider using consistent bracket syntax.
Line 8 uses
[[ ]]with==, while line 107 uses[ ]with=. Both are valid, but consistency improves readability.♻️ Proposed consistency fix
-if [ "$SKIP_SYSTEM_DEPS" = true ]; then +if [[ "$SKIP_SYSTEM_DEPS" == true ]]; then🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/setup_dependencies.sh` at line 107, The conditional using single-bracket test for SKIP_SYSTEM_DEPS is inconsistent with the earlier double-bracket style; change the line that currently reads using [ "$SKIP_SYSTEM_DEPS" = true ] to use the same double-bracket form and operator as the earlier check (use [[ "$SKIP_SYSTEM_DEPS" == true ]]) while keeping the variable quoted, so replace the single-bracket check for SKIP_SYSTEM_DEPS with a double-bracket equality check for consistency.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@scripts/setup_dependencies.sh`:
- Line 107: The conditional using single-bracket test for SKIP_SYSTEM_DEPS is
inconsistent with the earlier double-bracket style; change the line that
currently reads using [ "$SKIP_SYSTEM_DEPS" = true ] to use the same
double-bracket form and operator as the earlier check (use [[
"$SKIP_SYSTEM_DEPS" == true ]]) while keeping the variable quoted, so replace
the single-bracket check for SKIP_SYSTEM_DEPS with a double-bracket equality
check for consistency.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0cd96ee1-ba71-417d-a90e-e01b4aa192c8
📒 Files selected for processing (2)
.github/workflows/ci.ymlscripts/setup_dependencies.sh
|
@ArjunKalirana please check the CI failures |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
402-408: 💤 Low valueOptional: pin the action to a commit SHA (flagged by zizmor).
zizmor reports
actions/upload-artifact@v7as unpinned per a blanket pinning policy. Note this is consistent with the rest of the file, which uses floating@vNtags throughout, so fixing only this new step is inconsistent — consider addressing pinning repo-wide in a separate change if the policy is intended to be enforced.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml around lines 402 - 408, The workflow step named "Upload Playwright test results" uses the floating tag actions/upload-artifact@v7; replace that floating tag with a specific commit SHA for the actions/upload-artifact repository (e.g., actions/upload-artifact@<commit-sha>) so the step is pinned; update the uses line in that step to the chosen full commit SHA while keeping the step name, with the same inputs (name, path, retention-days) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 402-408: The workflow step named "Upload Playwright test results"
uses the floating tag actions/upload-artifact@v7; replace that floating tag with
a specific commit SHA for the actions/upload-artifact repository (e.g.,
actions/upload-artifact@<commit-sha>) so the step is pinned; update the uses
line in that step to the chosen full commit SHA while keeping the step name,
with the same inputs (name, path, retention-days) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9c26435d-5752-4cdb-ab25-fc612dcf86b0
📒 Files selected for processing (3)
.github/workflows/ci.ymlscripts/setup_dependencies.shtests/scripts/test_setup_dependencies.sh
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/scripts/test_setup_dependencies.sh
- scripts/setup_dependencies.sh
Code Coverage Report 📊Line Coverage: 80.7% ✅ Coverage meets threshold: 80.7% >= 60% Full Coverage Summary |
PR Preview RemovedThe GitHub Pages preview for this PR has been removed because the PR was closed. |
…mondal-2002#234) * ci: add --no-system-deps flag to setup script and use in jobs * checkout: temporary commit for worktree checkout * style: use consistent double bracket syntax in setup script * docs: add docstring to setup script and unit test for flag detection * fix: replace C-style block comment with shell comments in setup script * Delete tests/scripts/test_setup_dependencies.sh --------- Co-authored-by: SUDIP MONDAL <sudipmondal.2002@rediffmail.com>
What does this PR do?
This PR adds an explicit
--no-system-depsflag toscripts/setup_dependencies.shto allow skipping the interactive system package installation block. It also updates all 6 "Setup External Dependencies" steps in the CI workflow (.github/workflows/ci.yml) to use this flag, ensuring robust non-interactive builds across all platforms (Linux, macOS, Windows, Web, Android, and iOS).Related Issue
Fixes #144
Type of Change
How Was This Tested?
bash ./scripts/setup_dependencies.sh --no-system-depsinstall_depsprompt when the flag is present and continues to fetch theexternal/folder components (ImGui, kiss_fft, etc.).Checklist
./amplitron-tests)Screenshots / Demo
(N/A - This is a backend/CI optimization change)
Summary by CodeRabbit