PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

if ls ./build/*.hex &> /dev/null; then
    echo -e "\n*** Built targets in ./build:"
    stat -c "%n (%.19y)" ./build/*.hex
fi

RUN addgroup --gid $GROUP_ID inav
RUN adduser --disabled-password --gecos '' --uid $USER_ID --gid $GROUP_ID inav

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'

PR Code Suggestions ✨

Latest suggestions up to 020c1ca

 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()

 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()

Previous suggestions

-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

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.

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.

Fixes #10816 ?

Fixes #10816 ?

Yes it is.

Fixes #10816 ?

There is a commit to fix #10847 too.