Open
Conversation
Xeratec
changed the title
Member
Author
|
@Victor-Jung Currently, the I see two ways forward (or sequentially):
|
Member
Author
I have now created a pre-compiled version of GAP9 GCC for
|
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:
📝 WalkthroughWalkthroughAdds GAP9 support: CI workflow guards and runner tweaks, new multi-arch Docker build + manifest jobs, GAP9 container/toolchain Dockerfiles and Make targets, zsh container config, GAP9 build/run scripts and patches, docs and pre-commit updates, and small workflow env removals. Changes
Sequence Diagram(s)sequenceDiagram
participant Trigger as User
participant Prepare as prepare job
participant Build as build-deeploy-gap9 job
participant GHCR as GitHub Container Registry
participant Merge as merge-deeploy-gap9-images job
Trigger->>Prepare: workflow_dispatch / push
Prepare->>Build: provide docker_tag, matrix(platforms), inputs
Build->>GHCR: build per-platform image (buildx, cache, --ssh) and push per-arch digest
Build-->>Prepare: output digest-amd64, digest-arm64
Merge->>GHCR: create multi-arch manifest (latest, docker_tag) and push manifest
Merge->>Trigger: emit completion/status
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![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: 10
🤖 Fix all issues with AI agents
In `@Container/Dockerfile.deeploy-gap9`:
- Line 33: Change the misspelled package name in the Dockerfile: replace the
incorrect token "ibglib2.0-dev" with the correct Debian package "libglib2.0-dev"
in the apt-get install list so the image build no longer fails; update the
package entry where "ibglib2.0-dev" appears in the Dockerfile.
In `@Container/gap9-amd64.patch`:
- Around line 32-34: Update README_GAP9.md to document the Linux compatibility
workaround for the change from 'localhost' to 'host.docker.internal' used when
spawning telnet (see the self.telnet pexpect spawn change); explicitly state
that host.docker.internal is Docker Desktop-only on macOS/Windows and that Linux
users must either start Docker with --add-host=host.docker.internal:host-gateway
(Docker 20.10+) or run the provided gap9-run.sh with the USBIP_HOST override
(e.g., USBIP_HOST=localhost ./scripts/gap9-run.sh start or use the script's -h
flag to set the host) to avoid connection failures. Ensure the README includes
both command examples and a brief note about which approach to use on Linux.
In `@Container/Makefile`:
- Line 66: The clean target in the Makefile uses misspelled variables
DEELPOY_IMAGE and DEELPOY_GAP9_IMAGE so docker rmi expands to the wrong values
and images are never removed; fix by renaming those references to the correct
variable names (DEPLOY_IMAGE and DEPLOY_GAP9_IMAGE, matching how the images are
defined) and ensure TOOLCHAIN_IMAGE/DEPLOY_IMAGE/DEPLOY_GAP9_IMAGE are defined;
optionally remove or handle the trailing "|| true" so failures are surfaced
instead of silently ignored.
In `@Makefile`:
- Around line 437-444: The gap9-sdk Makefile target invokes
scripts/gap9-build_sdk.sh which currently runs git apply unconditionally and
will fail on re-runs; modify the patch-application logic in
scripts/gap9-build_sdk.sh to first test the patch (use git apply --check <patch>
and if that fails try git apply --reverse --check <patch> to detect
already-applied state) and only run git apply <patch> when --check succeeds,
otherwise skip applying the patch and log that it was already applied; reference
the existing git apply invocation in scripts/gap9-build_sdk.sh and keep behavior
idempotent under set -e.
In `@README_GAP9.md`:
- Line 6: Fix the grammar in the README_GAP9.md sentence by replacing "does yet
not include" with "does not yet include" so the line reads: "To use Deeploy with
GAP9, a custom Docker container is required because the official Deeploy Docker
image does not yet include the necessary SDKs and dependencies for GAP9
development, because they are not publicly available."
In `@scripts/gap9-build_sdk.sh`:
- Line 17: The default assignment for GAP_SDK_URL embeds literal single-quote
characters which end up in the expanded value and break git clone; change the
default so it does not include inner single quotes. Update the GAP_SDK_URL
assignment (the line setting
GAP_SDK_URL="${GAP_SDK_URL:-'git@iis-git.ee.ethz.ch:wiesep/gap9_sdk.git'}") to
use an unquoted URL inside the parameter expansion, e.g.
GAP_SDK_URL="${GAP_SDK_URL:-git@iis-git.ee.ethz.ch:wiesep/gap9_sdk.git}",
ensuring the variable remains double-quoted when used elsewhere.
In `@scripts/gap9-run.sh`:
- Around line 123-169: The case branches in the while loop (handling flags like
-i/--image, -d/--work-dir, -k/--ssh-key, --platform, --shell, etc.) access
"${args[$((idx + 1))]}" without verifying a next argument; with set -u this will
fail on a trailing flag. Add a bounds check before each case arm (or centrally
before consuming a value) ensuring idx+1 < ${`#args`[@]} and if not, print a clear
error/usage message and exit non‑zero; then only assign variables (e.g.
GAP9_IMAGE, WORK_DIR, CACHE_FOLDER, SSH_PRIVATE_KEY, USBIP_HOST, USBIP_VENDOR,
USBIP_PRODUCT, DOCKER_PLATFORM, DOCKER_SHELL) and append to opts after the
check. Ensure any shared parsing helper updates idx by 2 only after validation.
- Around line 305-328: The cmd_attach_usbip (and similarly cmd_detach_usbip)
function relies on checking $? after calling docker exec, but with set -euo
pipefail enabled those failing commands will terminate the script before the if
block runs; update the functions to capture the command's exit status
immediately (e.g., run docker exec in a subshell or append "|| true" and assign
its exit code to a variable) and then test that variable, or locally disable
errexit around the docker exec call and restore it afterward; specifically
modify the docker exec invocation inside cmd_attach_usbip/cmd_detach_usbip and
use a captured EXIT_CODE variable (or temporary errexit toggling) to drive the
success/failure log paths rather than relying on $?.
- Around line 244-247: The ShellCheck disable directive for SC1091 is placed
mid-&& chain which causes a parse error; move the directive so it appears before
the compound command or remove the inline comment and place it on the preceding
line. Specifically, adjust the block that uses PYUSBIP_DIR, the ".
.venv/bin/activate" activation and the "python pyusbip.py" invocation so the "#
shellcheck disable=SC1091" comment precedes the entire cd/activate/python
sequence (or place it on the script header) rather than between the "cd
\"$PYUSBIP_DIR\" &&" and ". .venv/bin/activate &&" tokens.
- Around line 442-447: The loop that iterates over the opts array uses
"${opts[@]:-}", which still triggers "unbound variable" under set -u for empty
arrays; update the iteration to safely handle an empty array by either (a) using
the POSIX-safe expansion for arrays: for opt in ${opts[@]+"${opts[@]}"}; do ...
done, or (b) checking the array length first with [[ ${`#opts`[@]} -gt 0 ]] and
only then iterating; update the block that constructs opts_escaped (references:
opts array and opts_escaped variable in scripts/gap9-run.sh) to use one of these
patterns so the script no longer errors when opts is empty under set -u.
🧹 Nitpick comments (12)
.pre-commit-config.yaml (1)
75-78: Missingstagesconfiguration — inconsistent with other hooks.Every other hook in this file explicitly sets
stages: [pre-commit, pre-merge-commit, pre-push, manual]. Theshfmthook omits this, so it will only run on the default stage (pre-commit), skippingpre-pushand others.Proposed fix
- repo: https://github.com/scop/pre-commit-shfmt rev: v3.12.0-2 hooks: - id: shfmt + name: Autoformat Shell Files + stages: [pre-commit, pre-merge-commit, pre-push, manual].gitignore (1)
57-59:.cache/pattern is broad — verify it won't hide wanted files.
.cache/(without a leading/) matches any directory named.cacheat any depth in the repo. If this is only intended for the root-level build cache, consider anchoring it as/.cache/to avoid accidentally ignoring nested.cache/directories in submodules or vendor trees..github/workflows/infra-generate-ccache-gap9.yml (1)
30-36:pip install -e . || truesilently swallows installation failures.If the editable install fails, the subsequent CCache generation step will run against a potentially broken environment, producing a cache that may not match real CI builds. Consider at least logging a warning or failing the step so stale/wrong caches don't persist.
Container/.zshrc (1)
1-116: Consider trimming the boilerplate comments.This file is the stock oh-my-zsh template with ~100 lines of commented-out defaults. Keeping only the active configuration (theme, plugins, history options,
unalias gap) would make it easier to maintain and review.Container/Dockerfile.deeploy (1)
129-132: Oh My Zsh install fetches from the internet at build time — minor reproducibility note.Fetching the install script via
curlfromraw.githubusercontent.comduring the build means the result depends on upstream availability and content at build time. This is a common pattern, but consider pinning to a specific commit hash for full reproducibility if needed.Also, there's a double
#in the comment on Line 130 (# # Install Oh My ZSH).Minor comment fix
-# # Install Oh My ZSH +# Install Oh My ZSHContainer/Dockerfile.deeploy-gap9 (2)
26-74: Duplicate packages and missing--no-install-recommends.Two issues in the package list:
sudois listed twice (lines 60 and 64).libftdi-devis listed twice (lines 43 and 70).- Adding
--no-install-recommendswould reduce image size, as flagged by static analysis.♻️ Proposed fix (showing key changes)
- apt-get install -y \ + apt-get install -y --no-install-recommends \ git-lfs \ ... - libftdi-dev \ ... - sudo \ ... - sudo \ + sudo \ ... - libftdi-dev \ + libftdi-dev \Remove the duplicate
sudo(line 64) andlibftdi-dev(line 70) entries, and add--no-install-recommends.
18-21: SSH known_hosts setup — consider pinning host keys.The
ssh-keyscanapproach works but is vulnerable to MITM during the build. For a more secure setup, you could embed the known host key fingerprints directly. This is acceptable for a development container, but worth noting.scripts/gap9-build_sdk.sh (1)
48-67: Fragileset -e/set +etoggling — consider a more robust pattern.The repeated toggling of
set -eandset +e(lines 48, 55, 65, 67) is error-prone. If the script is modified later, it's easy to accidentally leave strict mode off during a critical section. The comment on line 55 says "to allow deactivation even if build fails," butdeactivate(line 69) is a Python venv function that's unlikely to fail.Consider either keeping
set -efor the entire script and using|| truefor the specific commands that are allowed to fail, or using a trap-based approach.♻️ Suggested approach: keep `set -e` throughout
+set -e + if [ -n "$PATCH" ] && [ -f "${ROOT_DIR}/${PATCH}" ]; then echo "Applying platform patch: $PATCH" - git apply "${ROOT_DIR}/${PATCH}" + git apply "${ROOT_DIR}/${PATCH}" || true else echo "No platform-specific patch to apply for architecture '$ARCH' (looked for ${ROOT_DIR}/${PATCH})" fi -set +e # Disable strict error handling to allow deactivation even if build fails echo "Setting up Python virtual environment and installing dependencies" python -m venv .gap9-venv . .gap9-venv/bin/activate pip install "numpy<2.0.0" echo "Sourcing GAP9 SDK environment" . configs/gap9_evk_audio.sh || true echo "Invoking make install_dependency cmake_sdk.build" -set -e # Enable strict error handling for the build process make install_dependency cmake_sdk.build openocd.all autotiler.all -set +e # Disable strict error handling to allow deactivation even if build fails -deactivate +deactivate || truescripts/gap9-run.sh (1)
500-503: Default (no command) silently starts tmux — consider showing help instead.When no command is provided, the script defaults to
cmd_start_tmux(line 502), but this behavior isn't documented in the help text. Users might expect help output when running with no arguments. Consider either documenting this default or changing it to show help..github/workflows/docker-build-deeploy-gap9.yml (1)
26-38: Shell quoting and grouping — low-risk in this context.The static analysis flags SC2086 (unquoted variables) and SC2129 (grouped redirects). In GitHub Actions
run:blocks the values originate from GitHub-provided environment variables, so globbing/splitting risk is minimal. Still, quoting is a good habit.♻️ Suggested cleanup
- name: Set up environment variables run: | - echo "BRANCH_NAME=${GITHUB_REF##*/}" >> $GITHUB_ENV - echo "TAG_NAME=${GITHUB_REF##*/}" >> $GITHUB_ENV - echo "IS_TAG=${GITHUB_REF_TYPE}" >> $GITHUB_ENV + { + echo "BRANCH_NAME=${GITHUB_REF##*/}" + echo "TAG_NAME=${GITHUB_REF##*/}" + echo "IS_TAG=${GITHUB_REF_TYPE}" + } >> "$GITHUB_ENV"Makefile (2)
59-62:GAP_RISCV_GCC_COMMIT_HASHis defined but unused;GAP_SDK_URLhas embedded single quotes.
GAP_RISCV_GCC_COMMIT_HASH(line 59) is never referenced — the${GAP_RISCV_GCC_INSTALL_DIR}target hardcodes the release URLv0.0.1. If the intent is to track provenance, add a comment; otherwise remove it to avoid confusion.
GAP_SDK_URLwraps the value in literal single quotes. This happens to work because Make passes them verbatim to the shell where they act as quoting, but it's unconventional for a Makefile variable and will break if the variable is used in a non-shell context (e.g., a Make function like$(shell ...)).♻️ Suggested cleanup
-GAP_RISCV_GCC_COMMIT_HASH ?= fbb9fa450d01c1c170f94af817490f41c5ef7971 +# Provenance only – not used in download (pinned to release v0.0.1) +GAP_RISCV_GCC_COMMIT_HASH ?= fbb9fa450d01c1c170f94af817490f41c5ef7971 # GAP9_SDK_COMMIT_HASH ?= 1796873cec9ca1feb352a6fe980b627df979bdd1 # v5.21.1 GAP9_SDK_COMMIT_HASH ?= 8c42b65338e554ac73c749f94ecddd23a9ee5490 # v5.21.1-staging-1 -GAP_SDK_URL ?= 'git@github.com:pulp-platform/gap-sdk.git' +GAP_SDK_URL ?= git@github.com:pulp-platform/gap-sdk.git
429-435: Partial download leaves a broken install directory that Make won't retry.
${GAP_RISCV_GCC_INSTALL_DIR}is a directory target. Ifcurlortarfails mid-way, the directory already exists so Make considers the target up-to-date on the next run. Download to a temporary location and move on success to make this robust.♻️ Proposed fix
${GAP_RISCV_GCC_INSTALL_DIR}: - mkdir -p ${GAP_RISCV_GCC_INSTALL_DIR} && cd ${GAP_RISCV_GCC_INSTALL_DIR} && \ - curl -LO https://github.com/pulp-platform/gap-riscv-gnu-toolchain/releases/download/v0.0.1/gap9-gcc-ubuntu22.04-$(TARGET_GAP9).tar.gz && \ - tar -xzf gap9-gcc-ubuntu22.04-$(TARGET_GAP9).tar.gz --strip-components=1 -C . && \ - rm gap9-gcc-ubuntu22.04-$(TARGET_GAP9).tar.gz + mkdir -p ${GAP_RISCV_GCC_INSTALL_DIR}.tmp && \ + curl -L -o ${GAP_RISCV_GCC_INSTALL_DIR}.tmp/gap9-gcc.tar.gz \ + https://github.com/pulp-platform/gap-riscv-gnu-toolchain/releases/download/v0.0.1/gap9-gcc-ubuntu22.04-$(TARGET_GAP9).tar.gz && \ + tar -xzf ${GAP_RISCV_GCC_INSTALL_DIR}.tmp/gap9-gcc.tar.gz --strip-components=1 -C ${GAP_RISCV_GCC_INSTALL_DIR}.tmp && \ + rm ${GAP_RISCV_GCC_INSTALL_DIR}.tmp/gap9-gcc.tar.gz && \ + mv ${GAP_RISCV_GCC_INSTALL_DIR}.tmp ${GAP_RISCV_GCC_INSTALL_DIR}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@README_GAP9.md`:
- Around line 74-79: The README incorrectly states the tmux session has two
panes; update the documentation to reflect that cmd_start_tmux in gap9-run.sh
creates three panes (one for the GAP9 container, one for the USB/IP host, and
one for the USB/IP daemon), or alternatively modify cmd_start_tmux to actually
create only two panes—choose the intended behavior and either edit
README_GAP9.md to list three panes with their purposes or adjust the
cmd_start_tmux implementation to remove the extra pane so the README remains
accurate.
- Around line 100-103: The documentation incorrectly shows `dockerd
--add-host=host.docker.internal:host-gateway`; replace that with instructions to
pass the host mapping to container runs (e.g., use the `docker run
--add-host=host.docker.internal:host-gateway` form) so the `--add-host` flag is
used with `docker run`/`docker create` rather than the `dockerd` daemon command;
update the example line that currently reads `dockerd
--add-host=host.docker.internal:host-gateway` to the correct `docker run
--add-host=host.docker.internal:host-gateway ...` usage and keep a brief note
that this applies to Docker 20.10+.
In `@scripts/gap9-build_sdk.sh`:
- Around line 48-55: The script currently runs git apply under set -e which
aborts if the patch is already applied; modify the logic around PATCH/ROOT_DIR
and the git apply call to first detect whether the patch can be applied (use git
apply --check) and if not try git apply --reverse --check to detect
already-applied patches, skipping actual git apply when the patch is already
present; ensure set -e remains for build steps but perform these checks and
conditional skipping of git apply (or log that the patch is already applied)
before invoking git apply to avoid aborts on re-run.
- Line 14: The script's default ROOT_DIR ("ROOT_DIR" variable) points to the
script directory which causes patch lookup at "${ROOT_DIR}/${PATCH}" to fail
because patches live under Container/; update the script so ROOT_DIR is either
required from the caller or has a safer fallback: set the default to the
repository root (e.g., parent of scripts) or check for "${ROOT_DIR}/${PATCH}"
and if missing try "${ROOT_DIR}/Container/${PATCH}" before failing, and update
the logged path accordingly; touch the logic around the variable ROOT_DIR and
the lookup at "${ROOT_DIR}/${PATCH}" to implement this fallback/resolution
and/or add a documented usage note that ROOT_DIR must be passed.
- Around line 21-35: The script uses cd/gitrepo operations around
GAP9_SDK_INSTALL_DIR and commands like git clone, git fetch, git checkout, git
stash and git submodule update without failing fast, so a failed cd or git
command can cause later commands to run in the wrong directory; fix by enabling
strict error handling (add set -e at the top) or by appending explicit failure
checks (e.g., || exit 1) to the critical commands (the cd into
${GAP9_SDK_INSTALL_DIR}, git clone "${GAP_SDK_URL}" "${GAP9_SDK_INSTALL_DIR}",
git fetch --all --tags, git stash, git checkout "${GAP9_SDK_COMMIT_HASH}", and
git submodule update --init --recursive) so any failure aborts the script
immediately.
In `@scripts/gap9-run.sh`:
- Line 44: The SSH_PRIVATE_KEY variable is set with a literal tilde and later
expanded with `eval`, which is a command injection risk; change the assignment
of SSH_PRIVATE_KEY to use ${HOME} (e.g.,
SSH_PRIVATE_KEY="${HOME}/.ssh/id_ed25519") and remove any use of `eval` to
expand it (replace the `eval echo "$SSH_PRIVATE_KEY"` usage with safe parameter
expansion or direct variable use such as "$SSH_PRIVATE_KEY" or printf '%s'
"$SSH_PRIVATE_KEY"); update both the declaration (SSH_PRIVATE_KEY) and the
expansion site(s) that currently use eval so no user-controlled input is
executed.
🧹 Nitpick comments (4)
Container/Makefile (1)
51-59: Leakedssh-agentprocess after build.
eval $$(ssh-agent)spawns a background agent, but it is never terminated after the build completes. Each invocation ofmake deeploy-gap9leaves an orphanedssh-agentprocess. Use a trap or explicitly kill the agent.♻️ Proposed fix — kill ssh-agent after build
# Build the GAP9 Deeploy image deeploy-gap9: eval $$(ssh-agent) && \ ssh-add $(SSH_PRIVATE_KEY) && \ DOCKER_BUILDKIT=1 docker build \ --ssh default \ -f Dockerfile.deeploy-gap9 \ --build-arg DEEPLOY_IMAGE=$(DEEPLOY_IMAGE) \ - -t $(DEEPLOY_GAP9_IMAGE) .. + -t $(DEEPLOY_GAP9_IMAGE) .. ; \ + status=$$? ; \ + ssh-agent -k > /dev/null 2>&1 ; \ + exit $$statusAdditionally, consider adding
deeployas a prerequisite so the base image is guaranteed to exist:-deeploy-gap9: +deeploy-gap9: deeployscripts/gap9-build_sdk.sh (1)
57-62: Minor: usespythoninstead ofpython3, and SDK env source failure is silently ignored.Line 58 uses
pythonwhich may not exist or may point to Python 2 on some systems. Line 62 sources the GAP9 environment config with|| true, silently swallowing any failure — if the config is required for the build on line 66, the build will fail with a confusing error.♻️ Suggested improvement
-python -m venv .gap9-venv +python3 -m venv .gap9-venv . .gap9-venv/bin/activate -pip install "numpy<2.0.0" +pip install "numpy<2.0.0" echo "Sourcing GAP9 SDK environment" -. configs/gap9_evk_audio.sh || true +. configs/gap9_evk_audio.shscripts/gap9-run.sh (2)
498-502: No-argument default silently launches tmux — could be surprising.When no command is provided, the script silently launches
cmd_start_tmux(line 500) instead of showing help. This contradicts the help text pattern and could surprise users. Theshow_helpfunction exists and is already wired tohelp | --help.♻️ Suggestion — show help on no command
if [[ -z "$command" ]]; then - cmd_start_tmux - exit 0 + show_help + exit 1 fi
289-298: Privileged container with--pid=hostandnsenteris a significant security surface.The
usbip-devmgrcontainer runs with--privilegedand--pid=host, then usesnsenter -t1 -mto enter the host's mount namespace as PID 1. This effectively gives the container full root access to the host. While this may be necessary for USB/IP passthrough, it's worth documenting this risk explicitly and ensuring this is only used in development environments.
Xeratec
force-pushed
the
pr/gap9-docker
branch
from
62cd77b to
019e529
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
Container/gap9-arm64.patch (1)
47-58: Telnet targethost.docker.internalcouples the SDK to Docker Desktop.Changing telnet from
localhosttohost.docker.internalmeans the patched SDK only works inside Docker containers with that hostname available. On Linux without Docker Desktop, this requires--add-host=host.docker.internal:host-gatewayat container runtime. The README documents this, but it's worth noting that this patch permanently alters SDK behavior — anyone running the patched SDK outside Docker will get a connection failure.Consider making this configurable via an environment variable fallback (e.g.,
${TELNET_HOST:-host.docker.internal}) in the patch, so it works both inside and outside Docker.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Container/gap9-arm64.patch` around lines 47 - 58, The change in Openocd that hardcodes telnet target in self.telnet = pexpect.spawn(...) to 'host.docker.internal' breaks non-Docker-Desktop usage; update Openocd so the telnet host is configurable via an environment variable (e.g., read TELNET_HOST with a fallback to 'localhost') before calling pexpect.spawn, and use that variable in the pexpect.spawn invocation (refer to class Openocd and the self.telnet = pexpect.spawn(...) call) so the SDK works both inside Docker and on native hosts.Makefile (1)
62-62: Literal single quotes in Make variable are unconventional and fragile.
GAP_SDK_URL ?= 'git@github.com:pulp-platform/gap-sdk.git'stores literal single-quote characters in the Make variable. This happens to work because when the variable is expanded in the shell recipe on line 442, the shell interprets the quotes as shell quoting. However, this is non-idiomatic for Makefiles and will break if the variable is used in contexts where the shell doesn't strip the quotes (e.g., in non-recipe contexts or if the value is interpolated inside double quotes).♻️ Proposed fix — remove the extraneous quotes
-GAP_SDK_URL ?= 'git@github.com:pulp-platform/gap-sdk.git' +GAP_SDK_URL ?= git@github.com:pulp-platform/gap-sdk.git🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` at line 62, The Make variable GAP_SDK_URL currently contains literal single-quote characters; update its assignment (GAP_SDK_URL) to remove the extraneous single quotes so the value is git@github.com:pulp-platform/gap-sdk.git (no surrounding quotes). Locate the GAP_SDK_URL declaration and change it to an unquoted value, then run through usages (e.g., recipe expansions) to ensure shell recipes still quote/escape as needed rather than relying on embedded quotes.Container/Dockerfile.deeploy (1)
129-132: Oh My ZSH install fetches from an unpinned external URL at build time.The
curltoraw.githubusercontent.com/ohmyzsh/ohmyzsh/master/tools/install.shfetches whatever is onmasterat build time, making builds non-reproducible. Consider pinning to a specific commit hash for deterministic builds:-RUN sh -c "$(curl -fsSL https://raw.githubusercontent.com/ohmyzsh/ohmyzsh/master/tools/install.sh)" "" --unattended && \ +RUN sh -c "$(curl -fsSL https://raw.githubusercontent.com/ohmyzsh/ohmyzsh/refs/tags/v0.1/tools/install.sh)" "" --unattended && \Or pin to a specific commit SHA. This is a nice-to-have for build reproducibility.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Container/Dockerfile.deeploy` around lines 129 - 132, The Dockerfile's RUN step currently downloads the Oh My ZSH installer from the unpinned URL "https://raw.githubusercontent.com/ohmyzsh/ohmyzsh/master/tools/install.sh", which makes builds non-deterministic; update the RUN command that invokes curl (the Oh My ZSH install step) to fetch a pinned release by replacing the "master" URL with a specific release tag or commit SHA (or reference an official release tarball) and verify checksum if possible, keeping the existing unattended install and the subsequent copy of .zshrc to /root/.zshrc intact..github/workflows/docker-build-deeploy-gap9.yml (1)
60-60: Consider pinning third-party actions to commit SHAs.
jlumbroso/free-disk-space@v1.3.1,webfactory/ssh-agent@v0.9.0, andNoelware/docker-manifest-action@v1are pinned to version tags. For supply-chain security, pinning to full commit SHAs (with a version comment) is recommended, especially for actions that receive secrets (likessh-agentwithSSH_PRIVATE_KEY).Also applies to: 100-100, 142-142
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/docker-build-deeploy-gap9.yml at line 60, The workflow currently references third-party actions by tags (jlumbroso/free-disk-space@v1.3.1, webfactory/ssh-agent@v0.9.0, Noelware/docker-manifest-action@v1); replace each tag with the corresponding full commit SHA pin (e.g., jlumbroso/free-disk-space@<commit-sha>) and add a trailing comment indicating the original tag for clarity, ensuring you update all occurrences of those action usages so secrets-consuming actions (like webfactory/ssh-agent) are pinned to a specific commit for supply-chain security.Container/Makefile (1)
44-49:deeploytarget uses--ssh defaultbut doesn't start an ssh-agent.The
deeploy-gap9target explicitly startsssh-agentand adds the key (lines 53-54), butdeeployuses--ssh default(line 46) without doing the same. This will fail if no SSH agent is running in the user's environment. Consider either documenting this prerequisite or adding the same agent setup.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Container/Makefile` around lines 44 - 49, The deeploy Makefile target uses --ssh default but doesn't ensure an ssh-agent is running; update the deeploy target to mirror deeploy-gap9 by starting ssh-agent and adding the private key before invoking docker build (i.e., run ssh-agent -s and ssh-add "$(SSH_KEY)" or similar within the deeploy recipe), or alternatively add a clear comment/documentation above the deeploy target stating the prerequisite that an SSH agent with the key loaded must be running; modify the deeploy target or docs accordingly and reference the existing deeploy-gap9 logic as the pattern to follow.Container/Dockerfile.deeploy-gap9 (1)
26-74: Consider adding--no-install-recommendsto reduce image size.This is a large package list. Adding
--no-install-recommendstoapt-get installcan significantly reduce the final image size by avoiding pulling in optional recommended packages.Proposed fix
- apt-get install -y \ + apt-get install -y --no-install-recommends \🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Container/Dockerfile.deeploy-gap9` around lines 26 - 74, The RUN apt-get ... && apt-get install -y \ block in the Dockerfile currently installs many packages and pulls recommended extras; update the apt-get install invocation used in the RUN command (the piped RUN that includes "apt-get update && apt-get upgrade -y && apt-get install -y \") to include the --no-install-recommends flag (i.e., change apt-get install -y to apt-get install -y --no-install-recommends) to reduce image size and avoid unnecessary recommended packages, keeping the trailing rm -rf /var/lib/apt/lists/* cleanup as-is.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Container/Dockerfile.deeploy-gap9`:
- Around line 26-74: The apt-get install list in the Dockerfile RUN command
contains duplicate package entries (sudo and libftdi-dev); edit the RUN ...
apt-get install -y block to remove the repeated occurrences so each package
appears only once (keep one sudo and one libftdi-dev), ensure
spacing/commas/backslashes remain consistent and the final rm -rf
/var/lib/apt/lists/* step is preserved, and run a quick build lint to confirm no
syntax/line-continuation errors in the modified RUN command.
In `@Makefile`:
- Around line 437-444: The gap9-sdk target passes ROOT_DIR to scripts but the
patches live under Container/, causing gap9-build_sdk.sh to look in the wrong
place; fix by either updating the Makefile's gap9-sdk recipe to pass
ROOT_DIR=${ROOT_DIR}/Container when invoking bash
${ROOT_DIR}/scripts/gap9-build_sdk.sh, or modify gap9-build_sdk.sh to resolve
patch paths by prefixing "Container/" to the patch filenames before the file
existence check (ensure changes reference the gap9-sdk make target and the
gap9-build_sdk.sh script).
In `@scripts/gap9-run.sh`:
- Around line 498-501: The comment above the check for [[ -z "$command" ]] is
inaccurate: it currently says "If no command provided, show help" but the code
calls cmd_start_tmux which starts the tmux orchestration; update the comment to
accurately describe the behavior (e.g., "If no command provided, start the tmux
orchestration by calling cmd_start_tmux") or, if you intended to show help,
replace the cmd_start_tmux call with the proper help function; locate the
conditional using the "$command" variable and the cmd_start_tmux invocation to
make the fix.
---
Duplicate comments:
In `@Container/Dockerfile.deeploy-gap9`:
- Line 33: The Dockerfile contains a typo in the apt package list: replace the
incorrect package name "ibglib2.0-dev" with the correct "libglib2.0-dev" so
apt-get install succeeds; locate the package entry (string "ibglib2.0-dev") in
the Dockerfile.deeploy-gap9 Dockerfile and update it to "libglib2.0-dev".
In `@README_GAP9.md`:
- Around line 100-103: The README incorrectly shows using the daemon binary for
an option that belongs to the container runtime: replace the incorrect "dockerd
--add-host=host.docker.internal:host-gateway" with the proper container run
usage by instructing readers to use "docker run
--add-host=host.docker.internal:host-gateway" (i.e., move the --add-host flag to
the docker run command) and update any surrounding text to reflect that
--add-host is a docker run flag rather than a dockerd option.
- Around line 74-79: The README claims the tmux session has "two panes" for
./scripts/gap9-run.sh start-tmux but the script actually creates three panes;
either update the README_GAP9.md text to describe "three panes" and what each
pane is for, or modify the script's start-tmux branch in scripts/gap9-run.sh to
create only two panes (adjust the tmux split-window / select-layout commands) so
the docs match behavior; reference the start-tmux command in scripts/gap9-run.sh
and the README_GAP9.md section titled "Start the board workflow (recommended)"
when making the edit.
In `@scripts/gap9-build_sdk.sh`:
- Around line 21-35: The two plain cd invocations in the script (cd
"${GAP9_SDK_INSTALL_DIR}" used before updating the repo and again after cloning)
lack error handling and can leave the script running in the wrong directory;
update these to fail fast by adding explicit checks such as making each cd a
guarded call (e.g., cd "${GAP9_SDK_INSTALL_DIR}" || exit 1) or enable errexit
(set -e) earlier in the script, so subsequent git commands that reference
GAP9_SDK_INSTALL_DIR, git remote set-url, git fetch, git stash, git checkout and
git submodule update won't run on failure.
- Around line 48-55: The patch application with git apply is not idempotent and
will fail if the patch was already applied; modify the block that references
PATCH/ROOT_DIR and the git apply call to first run git apply --check
"${ROOT_DIR}/${PATCH}" and only run git apply if the check succeeds, otherwise
log that the patch appears already applied and skip; ensure the conditional
handles git apply --check failures without exiting the script (use an if/then
around git apply --check/git apply so set -e won't abort) and keep the existing
logging messages for success/skip cases.
- Around line 42-54: The script constructs PATCH as a bare filename so the file
existence check and git apply use ${ROOT_DIR}/${PATCH} and miss the actual files
under Container/; update the case that sets PATCH (or the subsequent path
references) to include the Container/ prefix (e.g., set PATCH to
Container/gap9-amd64.patch and Container/gap9-arm64.patch) so the [ -f
"${ROOT_DIR}/${PATCH}" ] test and git apply "${ROOT_DIR}/${PATCH}" locate and
apply the correct patch files for ARCH; keep the rest of the logic (ARCH case,
$ROOT_DIR usage, git apply and echo messages) unchanged.
In `@scripts/gap9-run.sh`:
- Around line 303-326: The script currently relies on post-command "$?" checks
after Docker invocations, but with set -e the script will exit before those
checks; update the cmd_attach_usbip function (the docker exec ... usbip attach
block) and the other block that runs docker run to capture failures by using an
inline conditional (if docker exec ...; then log_success ...; else log_error
...; fi) or by temporarily disabling errexit around the Docker call (set +e;
docker ...; rc=$?; set -e) and then checking rc; ensure you modify the code
paths that call docker exec and docker run so the success/failure logging uses
the captured exit code instead of relying on "$?" after a possibly-exiting
command.
- Around line 440-445: The for-loop that iterates over opts (for opt in
"${opts[@]:-}") isn't safe under set -u; change the loop to use the guarded
array expansion form so empty/unset opts won't trigger an error (replace the
iterator with the form that uses ${opts[@]+"${opts[@]}"}), keeping the inner
body that quotes and escapes each opt and appends to opts_escaped unchanged.
- Line 44: The SSH_PRIVATE_KEY variable and its later use via eval echo is a
command-injection risk and prevents proper tilde expansion; set the default to
use ${HOME} (e.g., SSH_PRIVATE_KEY="${HOME}/.ssh/id_ed25519") and remove the
insecure eval echo "$SSH_PRIVATE_KEY" usage—instead directly expand the variable
(or normalize a leading ~ safely with parameter expansion like
"${SSH_PRIVATE_KEY/#\~/$HOME}") wherever the script reads the path (refer to the
SSH_PRIVATE_KEY assignment and the location that currently runs eval echo
"$SSH_PRIVATE_KEY").
---
Nitpick comments:
In @.github/workflows/docker-build-deeploy-gap9.yml:
- Line 60: The workflow currently references third-party actions by tags
(jlumbroso/free-disk-space@v1.3.1, webfactory/ssh-agent@v0.9.0,
Noelware/docker-manifest-action@v1); replace each tag with the corresponding
full commit SHA pin (e.g., jlumbroso/free-disk-space@<commit-sha>) and add a
trailing comment indicating the original tag for clarity, ensuring you update
all occurrences of those action usages so secrets-consuming actions (like
webfactory/ssh-agent) are pinned to a specific commit for supply-chain security.
In `@Container/Dockerfile.deeploy`:
- Around line 129-132: The Dockerfile's RUN step currently downloads the Oh My
ZSH installer from the unpinned URL
"https://raw.githubusercontent.com/ohmyzsh/ohmyzsh/master/tools/install.sh",
which makes builds non-deterministic; update the RUN command that invokes curl
(the Oh My ZSH install step) to fetch a pinned release by replacing the "master"
URL with a specific release tag or commit SHA (or reference an official release
tarball) and verify checksum if possible, keeping the existing unattended
install and the subsequent copy of .zshrc to /root/.zshrc intact.
In `@Container/Dockerfile.deeploy-gap9`:
- Around line 26-74: The RUN apt-get ... && apt-get install -y \ block in the
Dockerfile currently installs many packages and pulls recommended extras; update
the apt-get install invocation used in the RUN command (the piped RUN that
includes "apt-get update && apt-get upgrade -y && apt-get install -y \") to
include the --no-install-recommends flag (i.e., change apt-get install -y to
apt-get install -y --no-install-recommends) to reduce image size and avoid
unnecessary recommended packages, keeping the trailing rm -rf
/var/lib/apt/lists/* cleanup as-is.
In `@Container/gap9-arm64.patch`:
- Around line 47-58: The change in Openocd that hardcodes telnet target in
self.telnet = pexpect.spawn(...) to 'host.docker.internal' breaks
non-Docker-Desktop usage; update Openocd so the telnet host is configurable via
an environment variable (e.g., read TELNET_HOST with a fallback to 'localhost')
before calling pexpect.spawn, and use that variable in the pexpect.spawn
invocation (refer to class Openocd and the self.telnet = pexpect.spawn(...)
call) so the SDK works both inside Docker and on native hosts.
In `@Container/Makefile`:
- Around line 44-49: The deeploy Makefile target uses --ssh default but doesn't
ensure an ssh-agent is running; update the deeploy target to mirror deeploy-gap9
by starting ssh-agent and adding the private key before invoking docker build
(i.e., run ssh-agent -s and ssh-add "$(SSH_KEY)" or similar within the deeploy
recipe), or alternatively add a clear comment/documentation above the deeploy
target stating the prerequisite that an SSH agent with the key loaded must be
running; modify the deeploy target or docs accordingly and reference the
existing deeploy-gap9 logic as the pattern to follow.
In `@Makefile`:
- Line 62: The Make variable GAP_SDK_URL currently contains literal single-quote
characters; update its assignment (GAP_SDK_URL) to remove the extraneous single
quotes so the value is git@github.com:pulp-platform/gap-sdk.git (no surrounding
quotes). Locate the GAP_SDK_URL declaration and change it to an unquoted value,
then run through usages (e.g., recipe expansions) to ensure shell recipes still
quote/escape as needed rather than relying on embedded quotes.
- Cleanup Docker flow to use a temporary build folder - Install zsh and oh-my-zsh plugin - Adapt GAP9 Docker Flow to support ARM64 - Add GAP9 Docker GitHub Build Flow - Add GAP9 Run script to use real hardware - Update README - Fix missing pre-commit dependency WIP
We still need x86 compilation for Autotiler
Xeratec
force-pushed
the
pr/gap9-docker
branch
from
0d8d78e to
4fa4a7c
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
.github/workflows/infra-generate-ccache-gap9.yml (1)
33-36:pip installfailure is silently swallowed.
pip install -e . || trueon Line 35 will mask genuine installation failures (e.g., missing dependencies, brokensetup.py). If Deeploy fails to install, the subsequent pytest steps will fail with a confusing import error rather than a clear build error.Consider removing
|| truefrom thepip installline so the job fails fast with an actionable error, or at least echo a warning.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/infra-generate-ccache-gap9.yml around lines 33 - 36, The pip install step currently swallows failures via "pip install -e . || true" so change the pipeline to fail fast: remove the "|| true" from the "pip install -e ." command (the line after "source /app/install/gap9-sdk/configs/gap9_evk_audio.sh") so any installation error stops the job and surfaces the actual failure; alternatively, if you must tolerate failures, capture the exit code and explicitly echo a clear error and exit non‑zero after "pip install -e ." to avoid obscuring broken installs.Container/Dockerfile.deeploy-gap9 (1)
26-72: Consider adding--no-install-recommendsto reduce image size.The
apt-get installcall pulls in recommended packages that are likely unnecessary for a build container. Adding the flag can meaningfully reduce the final image size.♻️ Proposed change
- apt-get install -y \ + apt-get install -y --no-install-recommends \🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Container/Dockerfile.deeploy-gap9` around lines 26 - 72, Change the RUN that performs "apt-get update && apt-get upgrade -y && apt-get install -y" so the apt-get install step includes the --no-install-recommends flag (i.e., replace apt-get install -y with apt-get install -y --no-install-recommends) for the long package list; keep the existing rm -rf /var/lib/apt/lists/* cleanup and other commands intact to reduce image size without altering installed package names.Container/Dockerfile.deeploy (1)
129-132: Oh My Zsh installed frommasterbranch — builds are not fully reproducible.The
curlfetches the install script from themasterbranch at build time. A future change to the upstream script could break builds. Consider pinning to a specific commit or release tag for reproducibility.Additionally, line 130 has a double comment prefix (
# #).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Container/Dockerfile.deeploy` around lines 129 - 132, The Dockerfile currently installs Oh My Zsh by curling the installer from the master branch in the RUN that follows COPY Container/.zshrc ./; change that RUN to fetch a specific release tag or commit SHA (pin the URL to a tag or commit hash for the Oh My Zsh installer) so the build is reproducible, and also remove the duplicate comment prefix on the line currently starting with “# #” (keep a single “#” or remove it entirely); keep the existing copying of .zshrc and the subsequent cp to /root/.zshrc in the same RUN sequence but replace the raw master URL with the pinned release/commit URL.
🤖 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/docker-build-deeploy-gap9.yml:
- Around line 104-117: The workflow step with id "build" uses
docker/build-push-action@v6 and currently sets both "push: true" and an
"outputs" block that includes push=true; remove the redundant "push: true" key
from the "Build and push final Deeploy image" step so the push behavior is
controlled solely by the "outputs" configuration (keep the "outputs" line as-is
and ensure no other push flags remain in the same step).
In `@Makefile`:
- Line 62: The GAP_SDK_URL Makefile variable is set with literal single quotes
which become part of the value and break git clone; update the assignment for
GAP_SDK_URL in the Makefile to remove the surrounding single quotes (or use
unquoted/double-quoted value) so the value passed into gap9-build_sdk.sh is a
valid git URL (ensure references to GAP_SDK_URL elsewhere remain unchanged).
In `@scripts/gap9-build_sdk.sh`:
- Line 23: The cd commands that use the pipeline operator are wrong: replace the
pipe `|` with the logical OR `||` so failures abort the script; specifically
update the two cd invocations referencing the GAP9_SDK_INSTALL_DIR variable (the
lines with cd "${GAP9_SDK_INSTALL_DIR}" | exit 1 and the other cd that uses the
same pattern) to use cd "${GAP9_SDK_INSTALL_DIR}" || exit 1 so the script exits
on a failed directory change.
---
Duplicate comments:
In `@Container/Dockerfile.deeploy-gap9`:
- Line 44: The package name in the Dockerfile package list is misspelled as
"ibglib2.0-dev"; update the apt package list entry (in
Container/Dockerfile.deeploy-gap9) to "libglib2.0-dev" so apt-get install
succeeds, ensuring the change is applied where the package list for RUN apt-get
install is defined.
In `@Makefile`:
- Around line 437-444: The Makefile target gap9-sdk uses ROOT_DIR to build
scripts/gap9-build_sdk.sh but patches live under Container/, causing a path
mismatch; update the target to reference the correct patch directory (e.g., use
CONTAINER_DIR or PATCH_DIR instead of ROOT_DIR) so the call to
scripts/gap9-build_sdk.sh runs in the Container context, and adjust any
environment variables passed (ROOT_DIR -> CONTAINER_DIR/PATCH_DIR) in the
gap9-sdk recipe to point to the Container/ location where the patches are
applied.
In `@scripts/gap9-build_sdk.sh`:
- Around line 48-55: The script currently runs git apply on
"${ROOT_DIR}/${PATCH}" inside a set -e block which aborts if the patch is
already applied; change this to first test applicability with git apply --check
(or git apply --reverse --check to detect already-applied hunks) and only run
git apply when the check succeeds, otherwise log that the patch was skipped;
update the block around the PATCH/ROOT_DIR variables so git apply is guarded by
the check and the script continues without error if the patch is already
applied.
In `@scripts/gap9-run.sh`:
- Around line 440-445: The loop using "${opts[@]:-}" can trigger unbound
variable errors under set -u for bash <4.4; update the loop to safely expand the
array by replacing the expansion "${opts[@]:-}" with the portable form
${opts[@]+"${opts[@]}"} so the for loop iterates zero times on an empty array
while preserving values of the opts variable and keeping the inner logic that
quotes and appends each element (referencing the opts array and opts_escaped
variable in the existing loop).
- Around line 303-354: The script uses set -e, so the post-command $? checks in
cmd_attach_usbip and cmd_detach_usbip are unreachable; change each docker
exec/docker run invocation to use an if ...; then ... else ... fi control flow
(i.e. if docker exec -it usbip-devmgr ...; then log_success ...; else log_error
...; fi in cmd_attach_usbip, and similarly wrap the docker run in
cmd_detach_usbip) or temporarily disable errexit around those calls (set +e
before the docker call and restore with set -e after) so failures don’t abort
the script before you can inspect the exit status and call log_error/log_warn.
Ensure you update the blocks referencing BUSID/PORT extraction so their failure
paths are handled inside the chosen if/else or with restored errexit.
- Around line 123-169: The case branches inside the while loop consume
args[$((idx+1))] without validating that a next element exists, which breaks
under set -u when a flag is last; add a bounds check before every assignment
(e.g., in the -i/--image, -d/--work-dir, -c/--cache-dir, -k/--ssh-key,
-h/--host, -v/--vendor, -p/--product, --platform, --shell branches) that
verifies (( idx + 1 < ${`#args`[@]} )) and prints a clear error/usage message and
exits if false (or implement a small helper like get_next_arg that performs this
check and returns the value), then use that safe value to set GAP9_IMAGE,
WORK_DIR, CACHE_FOLDER, SSH_PRIVATE_KEY, USBIP_HOST, USBIP_VENDOR,
USBIP_PRODUCT, DOCKER_PLATFORM, DOCKER_SHELL and append to opts and advance idx
as before.
- Line 44: The SSH_PRIVATE_KEY variable currently stores a tilde literal and
later uses an unsafe "eval echo" to expand it, which is a command-injection
vector when users supply -k; replace the eval-based expansion with a safe,
explicit tilde expansion and validate the -k input: set SSH_PRIVATE_KEY to a
default literal (SSH_PRIVATE_KEY="~/.ssh/id_ed25519") but immediately
sanitize/expand it with parameter substitution like
SSH_PRIVATE_KEY="${SSH_PRIVATE_KEY/#\~/$HOME}" (or use realpath -m
"$SSH_PRIVATE_KEY" for canonicalization), remove any usage of eval echo, and
ensure the code path that accepts -k validates it is a simple filename (no shell
metacharacters) before using it to override SSH_PRIVATE_KEY.
---
Nitpick comments:
In @.github/workflows/infra-generate-ccache-gap9.yml:
- Around line 33-36: The pip install step currently swallows failures via "pip
install -e . || true" so change the pipeline to fail fast: remove the "|| true"
from the "pip install -e ." command (the line after "source
/app/install/gap9-sdk/configs/gap9_evk_audio.sh") so any installation error
stops the job and surfaces the actual failure; alternatively, if you must
tolerate failures, capture the exit code and explicitly echo a clear error and
exit non‑zero after "pip install -e ." to avoid obscuring broken installs.
In `@Container/Dockerfile.deeploy`:
- Around line 129-132: The Dockerfile currently installs Oh My Zsh by curling
the installer from the master branch in the RUN that follows COPY
Container/.zshrc ./; change that RUN to fetch a specific release tag or commit
SHA (pin the URL to a tag or commit hash for the Oh My Zsh installer) so the
build is reproducible, and also remove the duplicate comment prefix on the line
currently starting with “# #” (keep a single “#” or remove it entirely); keep
the existing copying of .zshrc and the subsequent cp to /root/.zshrc in the same
RUN sequence but replace the raw master URL with the pinned release/commit URL.
In `@Container/Dockerfile.deeploy-gap9`:
- Around line 26-72: Change the RUN that performs "apt-get update && apt-get
upgrade -y && apt-get install -y" so the apt-get install step includes the
--no-install-recommends flag (i.e., replace apt-get install -y with apt-get
install -y --no-install-recommends) for the long package list; keep the existing
rm -rf /var/lib/apt/lists/* cleanup and other commands intact to reduce image
size without altering installed package names.
4fa4a7c to
65b8ad7
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
Container/Dockerfile.deeploy-gap9 (1)
26-72: Consider adding--no-install-recommendsto reduce image size.Trivy flags the missing
--no-install-recommends. For a toolchain container with many packages, skipping recommended (but unnecessary) transitive dependencies can noticeably shrink the final image.Proposed change
- apt-get install -y \ + apt-get install -y --no-install-recommends \🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Container/Dockerfile.deeploy-gap9` around lines 26 - 72, The RUN instruction that performs apt-get update/upgrade/install should add the --no-install-recommends flag to the apt-get install invocation to avoid installing recommended packages and reduce image size; update the RUN block that contains "apt-get install -y \ autoconf \ ..." so the install command becomes "apt-get install -y --no-install-recommends" (keeping the existing package list and the final rm -rf /var/lib/apt/lists/*).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Container/Dockerfile.deeploy-gap9`:
- Line 8: The Dockerfile declares ARG TARGETPLATFORM but never uses it; either
remove the unused ARG TARGETPLATFORM line from Container/Dockerfile.deeploy-gap9
to clean up the file, or if it is intended for multi-platform builds, reference
it where platform-specific logic is required (for example in FROM or conditional
steps) so TARGETPLATFORM is actually consumed.
In `@scripts/gap9-run.sh`:
- Around line 440-445: The loop iterating over "${opts[@]:-}" still triggers an
error under set -u on bash <4.4 when the array is empty; replace the unsafe
expansion with a guard that checks the array length before looping: use a
conditional like "if [[ ${`#opts`[@]} -gt 0 ]]; then for opt in "${opts[@]}"; do
...; done; fi" to protect the for-loop that populates opts_escaped and keep the
printf -v '%q' "$opt" logic unchanged.
---
Duplicate comments:
In `@Container/Dockerfile.deeploy-gap9`:
- Line 44: Replace the misspelled package name "ibglib2.0-dev" in the apt
package list inside the Dockerfile (the incorrect token appears in the install
list) with the correct "libglib2.0-dev" so the image build can find and install
the package; ensure the package list formatting (backslashes/newlines) remains
intact.
In `@scripts/gap9-run.sh`:
- Line 44: The script currently uses eval on the SSH_PRIVATE_KEY value (set via
-k) which allows command injection; locate SSH_PRIVATE_KEY and the eval usage
(the eval echo at line 192) and remove any eval. Instead implement safe
handling: directly use the variable (e.g., use printf '%s' "$SSH_PRIVATE_KEY" or
pass it quoted) and add explicit tilde expansion when needed by checking if
SSH_PRIVATE_KEY begins with '~' and replacing that prefix with "$HOME" (do not
perform any shell evaluation or substitutions on the variable). Ensure all
downstream uses of SSH_PRIVATE_KEY are quoted to avoid word-splitting.
- Around line 123-169: The option-parsing loop in scripts/gap9-run.sh (the while
loop consuming args with idx and the case branches for flags like -i/--image,
-d/--work-dir, -c/--cache-dir, etc.) lacks a bounds check before reading
${args[$((idx + 1))]}, which will fail under set -u if a flag is the last
argument; update the loop to verify that idx+1 is less than ${`#args`[@]} (or that
a next argument exists) before assigning to variables like GAP9_IMAGE, WORK_DIR,
CACHE_FOLDER, SSH_PRIVATE_KEY, USBIP_HOST, USBIP_VENDOR, USBIP_PRODUCT,
DOCKER_PLATFORM, DOCKER_SHELL and before appending to opts, and if the next
argument is missing emit a clear error and exit non‑zero.
- Around line 303-354: The failure checks using "$?" after commands in
cmd_attach_usbip and cmd_detach_usbip are ineffective under set -e; change each
call that runs docker (the docker exec in cmd_attach_usbip and the docker run in
cmd_detach_usbip) to use an if ...; then ...; else ...; fi pattern (e.g., if
docker exec ...; then log_success ...; else log_error/log_warn ...; fi) so the
script branches on the command result instead of relying on $? after a command
that would abort the script; update the blocks around the docker exec in
cmd_attach_usbip and the docker run in cmd_detach_usbip accordingly and remove
the trailing "$?" checks.
---
Nitpick comments:
In `@Container/Dockerfile.deeploy-gap9`:
- Around line 26-72: The RUN instruction that performs apt-get
update/upgrade/install should add the --no-install-recommends flag to the
apt-get install invocation to avoid installing recommended packages and reduce
image size; update the RUN block that contains "apt-get install -y \ autoconf \
..." so the install command becomes "apt-get install -y --no-install-recommends"
(keeping the existing package list and the final rm -rf /var/lib/apt/lists/*).
65b8ad7 to
6db1c52
Compare
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
1 participant
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 adds comprehensive GAP9 container support with ARM64 compatibility. It uses the latest GAP SDK version (
v5.21.1-staging-1).Added
v5.21.1-staging-1).github/workflows/docker-build-deeploy-gap9.yml)scripts/gap9-run.sh)README_GAP9.md)Changed
Fixed
PR Merge Checklist
develcommit and pointing todevel.CHANGELOG.mdfile has been updated.