Harden ARM64 CEF bundle handling in deploy_cef_app.sh#18
Conversation
Reviewer's GuideShell deployment script added for building a minimal CEF application and hardened to avoid accidental use of x86_64 CEF binaries on ARM64, plus README replaced with a detailed, structured documentation template for generating an RK3588 deployment tutorial. Sequence diagram for architecture-specific CEF bundle validationsequenceDiagram
actor User
participant Script as deploy_cef_app.sh
participant Kernel as uname
participant Helper as download_prebuilt_cef_sh
participant Network as CEF_server
User->>Script: Run script
Script->>Kernel: uname -m
Kernel-->>Script: arch
alt ARM64 aarch64_or_arm64
Script->>Script: Check CEF_BINARY_URL
alt CEF_BINARY_URL empty
Script-->>User: Error: CEF_BINARY_URL required on ARM64
Script->>Script: Exit
else CEF_BINARY_URL equals default_linux64
Script-->>User: Error: linux64 bundle invalid on ARM64
Script->>Script: Exit
else CEF_BINARY_URL valid_custom
Script->>Network: Download ARM64 CEF bundle via curl
Network-->>Script: CEF archive
Script->>Script: Extract and move to SOURCE_DIR/cef_binary
end
else Non_ARM64
Script->>Script: If CEF_BINARY_URL empty set default_linux64
Script->>Script: Check for SOURCE_DIR/cef_binary
alt cef_binary exists
Script->>Script: Reuse existing bundle
else cef_binary missing
Script->>Helper: Run download_prebuilt_cef.sh (if present)
alt Helper succeeds
Helper-->>Script: ARM CEF bundle in cef_binary
else Helper missing_or_fails
Script->>Network: Download CEF bundle via curl using CEF_BINARY_URL
Network-->>Script: CEF archive
Script->>Script: Extract and move to SOURCE_DIR/cef_binary
end
end
end
Script-->>User: Build completed and run instructions
Flow diagram for deploy_cef_app.sh overall executionflowchart TD
Start([Start deploy_cef_app.sh])
CheckSource["Validate SOURCE_DIR and src directory"]
ArchCheck["Detect architecture and validate CEF_BINARY_URL"]
PkgUpdate["sudo apt update"]
EnsureTools["Ensure tools (curl, wget, git, cmake, make, pkg-config, tar)"]
EnsureX11["Ensure X11 dev packages (libx11-dev, libxext-dev, ...)"]
FetchCEF["Fetch or reuse CEF binary bundle"]
BuildDir["Create build directory src/build if missing"]
CMakeConfig["Run cmake .. in src/build"]
BuildMake["Run make -j$(nproc)"]
PrintRun["Print run instructions for built cefapp"]
End([Script completed successfully])
Start --> CheckSource
CheckSource --> ArchCheck
ArchCheck --> PkgUpdate
PkgUpdate --> EnsureTools
EnsureTools --> EnsureX11
EnsureX11 --> FetchCEF
FetchCEF --> BuildDir
BuildDir --> CMakeConfig
CMakeConfig --> BuildMake
BuildMake --> PrintRun
PrintRun --> End
Flow diagram for hardened CEF bundle selection and downloadflowchart TD
A["Start fetch_cef_binary"]
B{Does SOURCE_DIR/cef_binary exist?}
C["Reuse existing cef_binary directory"]
D{Is CEF_BINARY_URL set?}
E{Does download_prebuilt_cef.sh exist in SOURCE_DIR?}
F["Run download_prebuilt_cef.sh with CEF_VERSION"]
G["Fail: CEF_BINARY_URL empty and no helper script"]
H["Create TEMP_DIR with mktemp -d"]
I["Download archive with curl -Lfo"]
J["Extract archive with tar -xvf"]
K{Find extracted cef_binary_* directory}
L["Fail: extracted CEF directory not found"]
M["Move extracted directory to SOURCE_DIR/cef_binary"]
N["Fail: move of CEF binaries into source tree"]
O["Done"]
A --> B
B -- yes --> C --> O
B -- no --> D
D -- no --> E
E -- yes --> F --> O
E -- no --> G
D -- yes --> H --> I --> J --> K
K -- not found --> L
K -- found --> M
M -- fail --> N
M -- success --> O
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The build output path assumption in
print_run_instructions(${BUILD_DIR_REL}/${BUILD_TYPE}/cefapp) doesn’t match the CMake invocation (cmake ..without aCMAKE_BUILD_TYPEor subdir), so either pass-DCMAKE_BUILD_TYPE=${BUILD_TYPE}and adjust the build tree, or update the run instructions to reflect the actual binary location. - Calling
sudo apt updateand multiplesudo apt installcommands unconditionally inside the script can be disruptive on systems with restricted sudo or custom package flows; consider gating these behind a confirmation flag or environment variable so the script can be run in a ‘no-package-changes’ mode. - The
verify_architecturelogic enforces a non-defaultCEF_BINARY_URLon ARM64 but still allows thedownload_prebuilt_cef.shfallback infetch_cef_binary; if that helper script might download x86_64 binaries, it would be safer to either validate its output or explicitly disallow that fallback on ARM64.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The build output path assumption in `print_run_instructions` (`${BUILD_DIR_REL}/${BUILD_TYPE}/cefapp`) doesn’t match the CMake invocation (`cmake ..` without a `CMAKE_BUILD_TYPE` or subdir), so either pass `-DCMAKE_BUILD_TYPE=${BUILD_TYPE}` and adjust the build tree, or update the run instructions to reflect the actual binary location.
- Calling `sudo apt update` and multiple `sudo apt install` commands unconditionally inside the script can be disruptive on systems with restricted sudo or custom package flows; consider gating these behind a confirmation flag or environment variable so the script can be run in a ‘no-package-changes’ mode.
- The `verify_architecture` logic enforces a non-default `CEF_BINARY_URL` on ARM64 but still allows the `download_prebuilt_cef.sh` fallback in `fetch_cef_binary`; if that helper script might download x86_64 binaries, it would be safer to either validate its output or explicitly disallow that fallback on ARM64.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a deployment script deploy_cef_app.sh to automate the setup and build process for the CEF application on ARM64, and updates the README.md with a template for documentation generation. The script is well-structured and includes good practices like error handling and cleanup routines. My review focuses on improving the robustness of the script with a few suggestions: making package installations non-interactive, enhancing the architecture validation logic, and making the discovery of the extracted CEF binary directory safer.
| local package_name="$2" | ||
| if ! command -v "$tool_name" >/dev/null 2>&1; then | ||
| echo "[INFO] Tool '${tool_name}' not found. Installing '${package_name}'..." | ||
| if ! sudo apt install -y "$package_name"; then |
There was a problem hiding this comment.
For fully non-interactive execution, it's best practice to set DEBIAN_FRONTEND=noninteractive when running apt install. This prevents any potential interactive prompts during package installation, making the script more robust for automation.
| if ! sudo apt install -y "$package_name"; then | |
| if ! sudo DEBIAN_FRONTEND=noninteractive apt install -y "$package_name"; then |
| local package_name="$1" | ||
| if ! dpkg -s "$package_name" >/dev/null 2>&1; then | ||
| echo "[INFO] Package '${package_name}' not installed. Installing..." | ||
| if ! sudo apt install -y "$package_name"; then |
There was a problem hiding this comment.
For fully non-interactive execution, it's best practice to set DEBIAN_FRONTEND=noninteractive when running apt install. This prevents any potential interactive prompts during package installation, making the script more robust for automation.
| if ! sudo apt install -y "$package_name"; then | |
| if ! sudo DEBIAN_FRONTEND=noninteractive apt install -y "$package_name"; then |
| if [ "${CEF_BINARY_URL}" = "${CEF_BINARY_URL_DEFAULT}" ]; then | ||
| echo "[ERROR] The default CEF URL targets linux64 (x86_64)." >&2 | ||
| echo "[ERROR] Set CEF_BINARY_URL to an ARM64-compatible CEF minimal build archive." >&2 | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
The current check only prevents using the exact default x86_64 URL. A more robust approach would be to check if the provided URL contains substrings like 'linux64' or 'x86_64' to catch a wider range of incorrect URLs for the ARM64 architecture. This would make the guardrail more effective.
| if [ "${CEF_BINARY_URL}" = "${CEF_BINARY_URL_DEFAULT}" ]; then | |
| echo "[ERROR] The default CEF URL targets linux64 (x86_64)." >&2 | |
| echo "[ERROR] Set CEF_BINARY_URL to an ARM64-compatible CEF minimal build archive." >&2 | |
| exit 1 | |
| fi | |
| if [[ "${CEF_BINARY_URL}" == *linux64* || "${CEF_BINARY_URL}" == *x86_64* ]]; then | |
| echo "[ERROR] The provided CEF URL '${CEF_BINARY_URL}' appears to be for x86_64 (linux64)." >&2 | |
| echo "[ERROR] On ARM64, you must set CEF_BINARY_URL to an ARM64-compatible CEF minimal build archive." >&2 | |
| exit 1 | |
| fi |
| local extracted_dir | ||
| extracted_dir="$(find "${TEMP_DIR}" -maxdepth 1 -type d -name 'cef_binary_*' | head -n 1)" | ||
| if [ -z "${extracted_dir}" ]; then | ||
| error_exit "Failed to locate extracted CEF directory." | ||
| fi |
There was a problem hiding this comment.
The current method of finding the extracted directory using find ... | head -n 1 is not fully robust. If the archive contains multiple directories matching the pattern cef_binary_*, it will silently pick the first one, which could lead to using the wrong binaries. It's safer to verify that exactly one matching directory is found.
| local extracted_dir | |
| extracted_dir="$(find "${TEMP_DIR}" -maxdepth 1 -type d -name 'cef_binary_*' | head -n 1)" | |
| if [ -z "${extracted_dir}" ]; then | |
| error_exit "Failed to locate extracted CEF directory." | |
| fi | |
| local extracted_dirs | |
| mapfile -t extracted_dirs < <(find "${TEMP_DIR}" -maxdepth 1 -type d -name 'cef_binary_*') | |
| if [ "${#extracted_dirs[@]}" -ne 1 ]; then | |
| error_exit "Expected to find exactly one 'cef_binary_*' directory, but found ${#extracted_dirs[@]}." | |
| fi | |
| local extracted_dir="${extracted_dirs[0]}" |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 2 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="README.md">
<violation number="1" location="README.md:50">
P2: README is committed as an unresolved template (with `[[...]]` placeholders) instead of usable project documentation.</violation>
</file>
<file name="deploy_cef_app.sh">
<violation number="1" location="deploy_cef_app.sh:95">
P2: ARM64 URL validation runs before local bundle detection, so valid existing `cef_binary` directories are rejected when `CEF_BINARY_URL` is unset.</violation>
<violation number="2" location="deploy_cef_app.sh:107">
P2: The helper fallback for empty `CEF_BINARY_URL` is unreachable because `verify_architecture()` always fills or rejects empty values first.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| <variable name="[[task_request]]" required="true"> | ||
| <description>Create an ordered, detailed deployment tutorial for the minimal CEF application on the RK3588 board, covering all features and constraints.</description> | ||
| </variable> | ||
| <variable name="[[software_applications_sources]]" required="true"> |
There was a problem hiding this comment.
P2: README is committed as an unresolved template (with [[...]] placeholders) instead of usable project documentation.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At README.md, line 50:
<comment>README is committed as an unresolved template (with `[[...]]` placeholders) instead of usable project documentation.</comment>
<file context>
@@ -1 +1,153 @@
+ <variable name="[[task_request]]" required="true">
+ <description>Create an ordered, detailed deployment tutorial for the minimal CEF application on the RK3588 board, covering all features and constraints.</description>
+ </variable>
+ <variable name="[[software_applications_sources]]" required="true">
+ <description>Source inventory of the CEF sample app, build scripts, and socket client implementation.</description>
+ </variable>
</file context>
| fi | ||
| else | ||
| if [ -z "${CEF_BINARY_URL}" ]; then | ||
| CEF_BINARY_URL="${CEF_BINARY_URL_DEFAULT}" |
There was a problem hiding this comment.
P2: The helper fallback for empty CEF_BINARY_URL is unreachable because verify_architecture() always fills or rejects empty values first.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At deploy_cef_app.sh, line 107:
<comment>The helper fallback for empty `CEF_BINARY_URL` is unreachable because `verify_architecture()` always fills or rejects empty values first.</comment>
<file context>
@@ -0,0 +1,226 @@
+ fi
+ else
+ if [ -z "${CEF_BINARY_URL}" ]; then
+ CEF_BINARY_URL="${CEF_BINARY_URL_DEFAULT}"
+ fi
+ fi
</file context>
| echo "[INFO] Detected architecture: ${arch}" | ||
| if [ "${arch}" = "aarch64" ] || [ "${arch}" = "arm64" ]; then | ||
| echo "[INFO] ARM64 detected. Ensure the CEF bundle matches ARM64." | ||
| if [ -z "${CEF_BINARY_URL}" ]; then |
There was a problem hiding this comment.
P2: ARM64 URL validation runs before local bundle detection, so valid existing cef_binary directories are rejected when CEF_BINARY_URL is unset.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At deploy_cef_app.sh, line 95:
<comment>ARM64 URL validation runs before local bundle detection, so valid existing `cef_binary` directories are rejected when `CEF_BINARY_URL` is unset.</comment>
<file context>
@@ -0,0 +1,226 @@
+ echo "[INFO] Detected architecture: ${arch}"
+ if [ "${arch}" = "aarch64" ] || [ "${arch}" = "arm64" ]; then
+ echo "[INFO] ARM64 detected. Ensure the CEF bundle matches ARM64."
+ if [ -z "${CEF_BINARY_URL}" ]; then
+ echo "[ERROR] CEF_BINARY_URL is empty on ARM64." >&2
+ echo "[ERROR] Provide an ARM64-compatible CEF tarball URL." >&2
</file context>
1 participant
Motivation
cef_binarydirectory or the project'sdownload_prebuilt_cef.shhelper when available.Description
CEF_BINARY_URLon aarch64/arm64 and treat the shipped linux64 default URL as invalid on ARM64 targets, failing with a clear error message when no ARM64 bundle is provided.CEF_BINARY_URLdefault to empty and set the linux64 default only for non-ARM64 machines; on ARM64 the script now rejects the linux64 default.fetch_cef_binary()to skip downloads when${SOURCE_DIR}/cef_binaryalready exists and to fall back to running${SOURCE_DIR}/download_prebuilt_cef.shwhen no URL is provided (on non-ARM64 or when appropriate), with explicit error messages on failure.mktempandtraputilities.Testing
deploy_cef_app.shwithcat/nl, verified working tree status withgit status, staged and committed the updated script, and created the PR; these steps completed successfully.Codex Task
Summary by cubic
Hardened CEF bundle handling for ARM64 and added
deploy_cef_app.shto install deps, fetch the right CEF bundle, build the app, and print run steps. Updated README with a structured RK3588 ARM64 deployment tutorial outline for Debian 11.11.New Features
deploy_cef_app.sh:aarch64/arm64; rejects linux64 default.${SOURCE_DIR}/cef_binaryif present or runsdownload_prebuilt_cef.shwhen available.CEF_BINARY_URLto empty; sets linux64 default only on non-ARM64.mktempandtrap; clear errors and idempotent flow.cmake,build-essential,pkg-config,libx11-dev, etc.), builds, and prints run examples.Migration
CEF_BINARY_URLto an ARM64 CEF minimal tarball; linux64 URLs are rejected.SOURCE_DIR(defaults to~/renhiyama-cefapp-sample),CEF_VERSION.SOURCE_DIRandCEF_BINARY_URL, then run./deploy_cef_app.sh.Written for commit 21b0a71. Summary will update on new commits.