Fixes Building in Docker

[Lebensgefahr]

User description

build.sh: fixed errors and added parameters valid_targets, release_targets to get a shortened list of available targets.
cmake/docker.sh: avoid initializing cmake on every run of ./build.sh.
Dockerfile: fixed syntax and removed unnecessary checks and instructions.

---

PR Type

Bug fix, Enhancement

---

Description

This description is generated by an AI tool. It may have inaccuracies

• Fixed Docker build script errors and improved functionality

• Added valid_targets and release_targets parameters for listing targets

• Optimized Docker image rebuilding to avoid unnecessary CMake initialization

• Enhanced error handling and output formatting

---

Diagram Walkthrough

flowchart LR
  A["build.sh"] --> B["Docker Image Check"]
  B --> C["Target Processing"]
  C --> D["valid_targets/release_targets"]
  C --> E["Build Execution"]
  F["cmake/docker.sh"] --> G["CMake Initialization Check"]
  H["Dockerfile"] --> I["User/Group Setup"]

Loading

File Walkthrough

	Relevant files
Enhancement	build.sh Enhanced build script with target listing                                build.sh Enhanced bash error handling with set -euo pipefail Added valid_targets and release_targets commands for listing available targets Improved Docker image building with quiet mode and error handling Added build output summary showing built hex files with timestamps +34/-10
Bug fix	docker.sh Fixed CMake initialization detection                                          cmake/docker.sh Fixed CMake initialization check from Makefile to build.ninja Prevents unnecessary CMake re-initialization on every build run +1/-1      Dockerfile Fixed Dockerfile syntax and user setup                                      Dockerfile Fixed user and group creation by removing error suppression Added proper GDB argument declaration Improved environment variable syntax Removed unnecessary VOLUME directive +7/-6

---

[qodo-code-review]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪

⚡ Recommended focus areas for review Logic Error The condition for checking built hex files is inverted. The script checks if files exist but then shows output regardless, and the original logic was checking if no files exist before showing the list. if ls ./build/*.hex &> /dev/null; then echo -e "\n*** Built targets in ./build:" stat -c "%n (%.19y)" ./build/*.hex fi Build Failure Removing the error suppression from user/group creation commands will cause Docker build to fail if the user or group already exists, breaking the build process. RUN addgroup --gid $GROUP_ID inav RUN adduser --disabled-password --gecos '' --uid $USER_ID --gid $GROUP_ID inav Missing Validation The new target listing commands don't validate if the Docker container runs successfully before processing output, which could lead to silent failures or incorrect output. release_targets) run_docker targets | sed -n 's/^Release targets: \(.*\)/\1/p'|tr ' ' '\n' ;; valid_targets) run_docker targets | sed -n 's/^Valid targets: \(.*\)/\1/p'|tr ' ' '\n'

[qodo-code-review]

PR Code Suggestions ✨

Latest suggestions up to 020c1ca

Category	Suggestion	Impact
Incremental [*]	Add version validation after installation Check if the version was successfully retrieved after installation before returning. If the version is still not found, the function should handle this failure case appropriately. cmake/arm-none-eabi-checks.cmake [128-133] gcc_get_version(version TRIPLET ${arm_none_eabi_triplet} PROGRAM_NAME prog PROGRAM_PATH prog_path ) +if(NOT version) + message(FATAL_ERROR "Failed to install or find ${prog} after installation") +endif() return() Apply / Chat Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies that if arm_none_eabi_gcc_install fails, the subsequent gcc_get_version will not set version, leading to silent failure; adding a check and a FATAL_ERROR is a critical improvement for robustness.	Medium
	Validate version after cache clear Similar to the first installation case, verify that the version was successfully retrieved after clearing the cache and reinstalling. Handle the case where installation fails. cmake/arm-none-eabi-checks.cmake [139-145] unset(gcc CACHE) gcc_get_version(version TRIPLET ${arm_none_eabi_triplet} PROGRAM_NAME prog PROGRAM_PATH prog_path ) +if(NOT version) + message(FATAL_ERROR "Failed to install or find ${prog} after cache clear and reinstallation") +endif() return() Apply / Chat Suggestion importance[1-10]: 8 __ Why: This suggestion correctly points out a missing check after reinstalling the compiler due to a version mismatch; adding a FATAL_ERROR ensures the build fails explicitly if the reinstallation is unsuccessful, which is a critical improvement.	Medium
Update

---

Previous suggestions

Suggestions up to commit 930d513

Category	Suggestion	Impact
General	Fix Docker image rebuild logic The quiet build attempt should only run when the image exists but may be outdated. Currently it runs in the else block when image exists, but should check if rebuild is actually needed first. build.sh [36-39] -docker build -q -t inav-build \ - --build-arg USER_ID="$(id -u)" \ - --build-arg GROUP_ID="$(id -g)" . >/dev/null || -{ echo "*** Building Docker image: ERROR"; exit 1; } +echo "*** Checking Docker image is up to date" +if ! docker build -q -t inav-build \ + --build-arg USER_ID="$(id -u)" \ + --build-arg GROUP_ID="$(id -g)" . >/dev/null; then + echo "*** Building Docker image" + docker build -t inav-build \ + --build-arg USER_ID="$(id -u)" \ + --build-arg GROUP_ID="$(id -g)" . +fi Suggestion importance[1-10]: 7 __ Why: The suggestion improves the Docker image update logic by performing a verbose build if the quiet update fails, which provides better error feedback to the user.	Medium

[Lebensgefahr]

The condition for checking built hex files is inverted. The script checks if files exist but then shows output regardless, and the original logic was checking if no files exist before showing the list.

ls returns 0 if at least one file matching the pattern exists, and 2 if none do. Therefore, the block inside if is executed only when matching files are present. In the previous version, it was always executed because ls ./build/*.hex &> /dev/null always returns an empty result due to both stderr and stdout being redirected to /dev/null.

Removing the error suppression from user/group creation commands will cause Docker build to fail if the user or group already exists, breaking the build process.

The user inav does not exist in the ubuntu:jammy image, and if you suppress error output during the group or user creation steps, the error will instead appear at the USER inav instruction. This makes it harder to understand why the user is missing.

The new target listing commands don't validate if the Docker container runs successfully before processing output, which could lead to silent failures or incorrect output.

For this reason, -o pipefail was added.

The quiet build attempt should only run when the image exists but may be
outdated. Currently it runs in the else block when image exists, but should
check if rebuild is actually needed first.

There is a different logic here. In the previous version, docker build was run only if the image did not exist. However, for example, when the Dockerfile changed, the build was not triggered again. Building the image when the Dockerfile hasn’t changed takes less than a second, so I don’t see a reason to skip it. But I don’t want to see the output every time. Therefore, the first build shows the full output, and subsequent builds only display build errors.

[sensei-hacker]

Thanks for commenting to note you looked at the things the AI bot pointed at. As you know, the bot can only point at things that might be worth looking at. It doesn't actually know anything. It's often helpful, but often not.

[sensei-hacker]

Fixes #10816 ?

[Lebensgefahr]

Fixes #10816 ?

Yes it is.

[Lebensgefahr]

Fixes #10816 ?

There is a commit to fix #10847 too.