Open
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughSummary by CodeRabbit
WalkthroughStandardizes CI workflows to use the Changes
Sequence Diagram(s)sequenceDiagram
participant Script as gap9-build_sdk.sh
participant FS as Filesystem
participant GitRemote as Git Remote
participant GitLFS as Git LFS
participant Builder as Make/CMake/Build
Script->>FS: check install dir and for .git
alt .git missing or not a repo
Script->>GitRemote: init repo, add remote, fetch commit
else .git present
Script->>GitRemote: set remote URL, fetch commit, stash changes, checkout commit
end
GitRemote-->>Script: commit fetched
Script->>FS: update submodules, checkout target commit
Script->>GitLFS: git lfs pull (retrieve .so objects)
GitLFS-->>Script: LFS objects available
Script->>FS: locate/apply patch (ROOT_DIR/${PATCH} or ROOT_DIR/Container/${PATCH})
Script->>Builder: run install_dependency, cmake_sdk.build, make install (with adjusted error handling)
Builder-->>Script: build result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip CodeRabbit can generate a title for your PR based on the changes.Add |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/ci-platform-gap9-tiled.yml (1)
31-31: Minor inconsistency: missing parentheses in fallback expression.This expression differs from
ci-platform-gap9.yml(line 32) which uses parentheses around the conditional:# ci-platform-gap9.yml docker_image_deeploy: ${{ github.event.inputs.docker_image_deeploy || (github.repository == 'pulp-platform/Deeploy' && 'ghcr.io/pulp-platform/deeploy-gap9:devel') }}For consistency and clarity, consider adding parentheses here as well.
Suggested fix
- docker_image_deeploy: ${{ github.event.inputs.docker_image_deeploy || github.repository == 'pulp-platform/Deeploy' && 'ghcr.io/pulp-platform/deeploy-gap9:devel'}} + docker_image_deeploy: ${{ github.event.inputs.docker_image_deeploy || (github.repository == 'pulp-platform/Deeploy' && 'ghcr.io/pulp-platform/deeploy-gap9:devel') }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci-platform-gap9-tiled.yml at line 31, The docker_image_deeploy fallback expression is missing parentheses around the conditional part; update the value for docker_image_deeploy to wrap the conditional (github.repository == 'pulp-platform/Deeploy' && 'ghcr.io/pulp-platform/deeploy-gap9:devel') in parentheses so it matches the style and precedence used in ci-platform-gap9.yml and prevents ambiguity when evaluating the fallback.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/gap9-build_sdk.sh`:
- Around line 26-32: The block assumes "${GAP9_SDK_INSTALL_DIR}" exists and uses
"git reset --soft" + "git add ." which is confusing; before cd into
GAP9_SDK_INSTALL_DIR validate existence (mkdir -p if you intend to create it, or
emit a clear error and exit if it must pre-exist) so the cd won't fail silently,
and either replace the "git reset --soft" + "git add ." sequence with a command
that actually aligns the working tree to the target commit (e.g., use "git reset
--hard ${GAP9_SDK_COMMIT_HASH}" or perform an explicit checkout) or add a short
comment above that sequence explaining the exact intent to "adopt existing files
into git tracking" so future readers understand why soft-reset + add is used
(referencing GAP9_SDK_INSTALL_DIR, git reset --soft, git add ., and the later
git checkout).
---
Nitpick comments:
In @.github/workflows/ci-platform-gap9-tiled.yml:
- Line 31: The docker_image_deeploy fallback expression is missing parentheses
around the conditional part; update the value for docker_image_deeploy to wrap
the conditional (github.repository == 'pulp-platform/Deeploy' &&
'ghcr.io/pulp-platform/deeploy-gap9:devel') in parentheses so it matches the
style and precedence used in ci-platform-gap9.yml and prevents ambiguity when
evaluating the fallback.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 71d6bde1-9470-48bc-a36d-fde87d563bd6
📒 Files selected for processing (6)
.github/workflows/ci-platform-gap9-tiled.yml.github/workflows/ci-platform-gap9.yml.github/workflows/infra-generate-ccache-gap9.ymlREADME.mdpyproject.tomlscripts/gap9-build_sdk.sh
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/gap9-build_sdk.sh`:
- Line 47: The git LFS pull step currently swallows errors via the "git lfs pull
--include=\"*.so\" || true" construct; remove the "|| true" so the command fails
on error and stops the script, ensuring missing .so artifacts fail fast; update
the invocation of "git lfs pull --include=\"*.so\"" in scripts/gap9-build_sdk.sh
(the git lfs pull line) accordingly and do not suppress its exit status.
- Around line 26-38: Move the strict shell mode activation so git failures are
caught immediately: enable set -euo pipefail near the top of the script (after
variable definitions) rather than later, and then remove any masking of git
errors (e.g. remove or avoid using "|| true" on git fetch/fetch-related
commands); ensure the git preparation block that uses GAP9_SDK_INSTALL_DIR,
GAP_SDK_URL and GAP9_SDK_COMMIT_HASH runs under strict mode so commands like git
clone, git init, git remote add origin, git fetch --all --tags, git reset --soft
and git add . will fail the script on error.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 73b16fc9-80fa-4127-acee-16f14892ceef
📒 Files selected for processing (1)
scripts/gap9-build_sdk.sh
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/infra-generate-ccache-gap9.yml:
- Line 14: The workflow currently uses a personal fork image
"ghcr.io/xeratec/deeploy-gap9:fix_ci"; revert both occurrences (the default
image entry and the other occurrence at lines mentioned) to the official
registry (e.g., "ghcr.io/pulp-platform/deeploy-gap9:latest" or a stable release
tag) in the workflow file, and preferably replace the floating tag with a pinned
digest (image:tag@sha256:...) to ensure reproducibility and prevent tag-swap
attacks; update the default key value and any other image string occurrences so
they point to the official registry/digest rather than the xeratec fork.
In `@CHANGELOG.md`:
- Around line 24-25: The changelog entry "Use by default `devel` container for
GAP9 CI" must reflect the final merged state rather than the temporary hotfix
tag; update that line to match the actual container/tag that will be used
post-merge (e.g., replace the sentence with the final tag name or a neutral
phrasing), and if the workflows currently reference
`ghcr.io/xeratec/deeploy-gap9:fix_ci` either change the changelog to mention the
temporary hotfix explicitly or wait until the registry/tag is updated and then
set the changelog entry to the final `devel` tag—ensure the text on Line 24
matches the actual state after merging.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e643af09-81bf-47ec-bb8e-79597d0b74b1
📒 Files selected for processing (4)
.github/workflows/ci-platform-gap9-tiled.yml.github/workflows/ci-platform-gap9.yml.github/workflows/infra-generate-ccache-gap9.ymlCHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/ci-platform-gap9-tiled.yml
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
scripts/gap9-build_sdk.sh (1)
28-42: Consider enabling strict mode earlier to catch git failures.The git initialization and fetch operations (lines 28-42) run without
set -e, so failures ingit init,git remote add,git fetch, orgit reset --hardwon't halt the script. These are critical setup steps that should fail fast.🛠️ Suggested approach
Move
set -eto after variable definitions (around line 23) and selectively use|| trueonly where failures are acceptable (like the stash operation mentioned above):GAP9_SDK_COMMIT_HASH="${GAP9_SDK_COMMIT_HASH:?GAP9_SDK_COMMIT_HASH must be set}" GAP_SDK_URL="${GAP_SDK_URL:?GAP_SDK_URL must be set}" + +set -e # Fail fast on errors echo "Preparing GAP9 SDK in: ${GAP9_SDK_INSTALL_DIR}"Then add explicit
|| trueonly to the stash operation where failure is expected.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/gap9-build_sdk.sh` around lines 28 - 42, The script runs critical git commands (git init, git remote add origin, git fetch, GIT_LFS_SKIP_SMUDGE=1 git reset --hard) without strict error handling; move set -e to earlier (immediately after variable definitions) so failures in those commands abort the script, and remove any blanket suppression of errors there; keep git stash but make it tolerant by adding an explicit || true to the git stash command only (leave other git commands to fail fast).pyproject.toml (1)
21-21: Pinningsetuptoolsas a runtime dependency is a fragile workaround.While this addresses the immediate CI breakage (GVSoC uses
pkg_resourcesremoved in setuptools 82.0.0), pinning setuptools as a runtime dependency is unusual and creates maintenance burden:
- This version pin will likely conflict with other packages that depend on newer setuptools.
- The
[build-system]section already specifiessetuptools>=42, creating inconsistent version requirements.Consider adding a code comment explaining why this pin exists so future maintainers understand the constraint. Ideally, track an upstream issue with GVSoC to migrate away from deprecated
pkg_resources.📝 Suggested documentation
dependencies = [ -'setuptools==81.0.0', +# Pin setuptools to 81.0.0: GVSoC uses pkg_resources which was removed in 82.0.0 +# TODO: Remove once GVSoC migrates to importlib.resources +'setuptools==81.0.0', 'protobuf==4.23.3',🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pyproject.toml` at line 21, The pinned runtime dependency 'setuptools==81.0.0' is a fragile workaround; update the pyproject.toml by adding an inline comment next to the 'setuptools==81.0.0' entry that explains the reason for the pin (GVSoC uses pkg_resources removed in setuptools 82.0.0), include a TODO with a tracked upstream issue URL or issue number (or a placeholder if unknown), and state the intended future action (remove pin once GVSoC migrates); also add a brief note pointing out the inconsistency with the [build-system] setuptools>=42 spec so maintainers know to reconcile these when the pin is removed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/gap9-build_sdk.sh`:
- Around line 40-41: Replace the unconditional git stash after the git add .
step with a guarded stash: check whether there are uncommitted changes in the
working tree or index and only run the stash when changes exist (e.g., use git
diff and git diff --cached to detect changes), so the script doesn't fail when
the tree is clean; update the sequence around the git add . / git stash commands
accordingly.
- Around line 25-26: Ensure the script stops if changing into the install
directory fails: after creating the directory with mkdir -p
"${GAP9_SDK_INSTALL_DIR}", run cd "${GAP9_SDK_INSTALL_DIR}" and immediately
check its exit status; if cd fails, print an error mentioning
GAP9_SDK_INSTALL_DIR and exit non‑zero. Update the block around mkdir -p and cd
to perform this check so the script cannot continue running in the wrong
directory.
---
Nitpick comments:
In `@pyproject.toml`:
- Line 21: The pinned runtime dependency 'setuptools==81.0.0' is a fragile
workaround; update the pyproject.toml by adding an inline comment next to the
'setuptools==81.0.0' entry that explains the reason for the pin (GVSoC uses
pkg_resources removed in setuptools 82.0.0), include a TODO with a tracked
upstream issue URL or issue number (or a placeholder if unknown), and state the
intended future action (remove pin once GVSoC migrates); also add a brief note
pointing out the inconsistency with the [build-system] setuptools>=42 spec so
maintainers know to reconcile these when the pin is removed.
In `@scripts/gap9-build_sdk.sh`:
- Around line 28-42: The script runs critical git commands (git init, git remote
add origin, git fetch, GIT_LFS_SKIP_SMUDGE=1 git reset --hard) without strict
error handling; move set -e to earlier (immediately after variable definitions)
so failures in those commands abort the script, and remove any blanket
suppression of errors there; keep git stash but make it tolerant by adding an
explicit || true to the git stash command only (leave other git commands to fail
fast).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f2440e46-7062-4057-83f5-3c3d5a878a7e
📒 Files selected for processing (7)
.github/workflows/ci-platform-gap9-tiled.yml.github/workflows/ci-platform-gap9.yml.github/workflows/infra-generate-ccache-gap9.ymlCHANGELOG.mdREADME.mdpyproject.tomlscripts/gap9-build_sdk.sh
🚧 Files skipped from review as they are similar to previous changes (2)
- CHANGELOG.md
- .github/workflows/ci-platform-gap9.yml
Member
Author
|
Changes are tested in these two flows: |
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/gap9-build_sdk.sh (1)
76-83:⚠️ Potential issue | 🟠 MajorDo not suppress failures during venv/dependency/environment setup.
Line 76 switches to
set +ebeforepython -m venv, activation, andpip install. If any of these fail, the script continues and can build with a broken or wrong environment.Suggested fix
-set +e # Relax mode so cleanup/deactivate still runs on non-critical failures. +# Keep strict mode for environment setup too. @@ echo "Setting up Python virtual environment and installing dependencies" python -m venv .gap9-venv . .gap9-venv/bin/activate pip install "numpy<2.0.0" @@ make install_dependency cmake_sdk.build openocd.all autotiler.all -set +e # Allow deactivation even if previous step failed. +set +e # Only relax for cleanup. -deactivate +deactivate || true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/gap9-build_sdk.sh` around lines 76 - 83, The script currently disables error exits with set +e before creating the venv and installing deps and also suppresses sourcing errors with ". configs/gap9_evk_audio.sh || true"; change this so environment setup failures abort the script: remove or move the set +e so that the block running "python -m venv .gap9-venv", ". .gap9-venv/bin/activate", and "pip install 'numpy<2.0.0'" runs with exit-on-error (e.g., ensure set -e is in effect for that section or explicitly check $? after each command), and remove the "|| true" from ". configs/gap9_evk_audio.sh" so sourcing failures also propagate; keep any relaxed error handling only after cleanup/deactivate steps.
♻️ Duplicate comments (1)
scripts/gap9-build_sdk.sh (1)
29-43:⚠️ Potential issue | 🟠 MajorEnable strict mode before git preparation (not after it).
Git bootstrap/fetch/reset in Line 29–43 runs before strict mode is enabled (Line 61), so failures can be ignored and later steps run in an invalid repo state.
Suggested fix
ROOT_DIR="${ROOT_DIR:-$(cd "$(dirname "$0")" && pwd)}" GAP9_SDK_INSTALL_DIR="${GAP9_SDK_INSTALL_DIR:?GAP9_SDK_INSTALL_DIR must be set}" GAP9_SDK_COMMIT_HASH="${GAP9_SDK_COMMIT_HASH:?GAP9_SDK_COMMIT_HASH must be set}" GAP_SDK_URL="${GAP_SDK_URL:?GAP_SDK_URL must be set}" +set -euo pipefail @@ -set -e # Fail fast for patch/build operations. +# already in strict modeAlso applies to: 61-61
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/gap9-build_sdk.sh` around lines 29 - 43, Move the shell strict mode invocation (set -euo pipefail) to the very top of the script before any git operations so failures during git init/git remote/git fetch/GIT_LFS_SKIP_SMUDGE=1 git reset and git remote set-url are not ignored; ensure the git commands that must fail the script (git fetch, git reset, git init, git remote set-url) run under strict mode and remove or explicitly handle any swallowing of errors (e.g., avoid using "|| true" on critical commands like git stash unless intentionally optional), and keep existing env use of GAP_SDK_URL and GAP9_SDK_COMMIT_HASH intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@scripts/gap9-build_sdk.sh`:
- Around line 76-83: The script currently disables error exits with set +e
before creating the venv and installing deps and also suppresses sourcing errors
with ". configs/gap9_evk_audio.sh || true"; change this so environment setup
failures abort the script: remove or move the set +e so that the block running
"python -m venv .gap9-venv", ". .gap9-venv/bin/activate", and "pip install
'numpy<2.0.0'" runs with exit-on-error (e.g., ensure set -e is in effect for
that section or explicitly check $? after each command), and remove the "||
true" from ". configs/gap9_evk_audio.sh" so sourcing failures also propagate;
keep any relaxed error handling only after cleanup/deactivate steps.
---
Duplicate comments:
In `@scripts/gap9-build_sdk.sh`:
- Around line 29-43: Move the shell strict mode invocation (set -euo pipefail)
to the very top of the script before any git operations so failures during git
init/git remote/git fetch/GIT_LFS_SKIP_SMUDGE=1 git reset and git remote set-url
are not ignored; ensure the git commands that must fail the script (git fetch,
git reset, git init, git remote set-url) run under strict mode and remove or
explicitly handle any swallowing of errors (e.g., avoid using "|| true" on
critical commands like git stash unless intentionally optional), and keep
existing env use of GAP_SDK_URL and GAP9_SDK_COMMIT_HASH intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 42011d50-6f65-4819-930b-b651b213f97e
📒 Files selected for processing (1)
scripts/gap9-build_sdk.sh
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR fixes the currently broken CI. This had two reason:
setuptools 82.0.0recently removed thepkg_resourceslibrary which is used by GVSoCudma_v4_gap9_v2_impl.soThe GAP9 check will only pass on the fork! (See below for status)
Changed
develcontainer for GAP9 CIFixed
*.sogit lfs filessetuptoolsto81.0.0PR Merge Checklist
develcommit and pointing todevel.CHANGELOG.mdfile has been updated.Details
I am currently building the new containers on my fork to then retest the CI.
Broken CIs
Fixed CI