feat: build.sh script #19
Conversation
|
Warning Rate limit exceeded@nerdCopter has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 7 minutes and 0 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant build as "build.sh"
participant Network as "curl"
participant Archive as "tar"
participant Python as "python (make.py)"
participant File as "src/version.h"
participant Output as "output/F3.bin"
User->>build: Run build.sh
rect rgb(235,245,255)
Note over build: Toolchain provisioning (Linux/macOS)
build->>build: Detect OS
alt toolchain missing
build->>Network: Download toolchain archive
Network-->>build: Archive stream
build->>Archive: Extract archive
Archive-->>build: Toolchain dir
build->>build: Prepend toolchain/bin to PATH
end
end
rect rgb(235,255,235)
Note over build: Build execution
build->>Python: Run make.py -C -T F3
Python-->>Output: Produce output/F3.bin
end
rect rgb(255,248,230)
Note over build: Post-build staging
build->>File: Read FIRMWARE_VERSION
File-->>build: Version string
build->>build: Validate version & construct DEST_FILE
build->>Output: Verify existence of output/F3.bin
build->>Output: Copy to output/IMUF_<VERSION>.bin (preserve metadata)
end
build-->>User: Success or failure message
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
build.sh (4)
36-36: Clarify tar invocation for portability.Using
tar xjfwithout an explicit-to indicate stdin works but relies on implicit behavior. For clarity and to explicitly document stdin usage, consider:curl -L -s "$DOWNLOAD_URL" | tar -xjf -This makes the intent clearer and is more explicitly portable across BSD and GNU tar variants.
56-56: Separate declaration and assignment to avoid masking errors.Combining
exportwith command substitution can mask failures in thepwdcommand. Separate them for better error detection and clarity:-export PATH="$PATH:$(pwd)/$TOOLCHAIN_DIR/bin" +TOOLCHAIN_BIN_PATH="$(pwd)/$TOOLCHAIN_DIR/bin" +export PATH="$PATH:$TOOLCHAIN_BIN_PATH"This follows shell best practices and makes errors in subshells more visible.
85-91: Strengthen version extraction and validation.The grep/awk pattern assumes a specific format (
#define FIRMWARE_VERSION <number>) and the numeric validation usingexpr "$FIRMWARE_VERSION" + 1may have edge cases (e.g., hex strings like "0x123" fail the check, but strings like "1abc" may be interpreted unexpectedly by some expr implementations).Consider a more explicit validation pattern:
-FIRMWARE_VERSION=$(grep '#define FIRMWARE_VERSION' "$VERSION_FILE" | awk '{print $3}') - -# Basic validation for the extracted version -if [ -z "$FIRMWARE_VERSION" ] || ! expr "$FIRMWARE_VERSION" + 1 >/dev/null 2>&1; then +FIRMWARE_VERSION=$(grep '#define FIRMWARE_VERSION' "$VERSION_FILE" | awk '{print $3}') + +# Basic validation: ensure extracted version is a non-empty decimal number +if [ -z "$FIRMWARE_VERSION" ] || ! expr "$FIRMWARE_VERSION" : '^[0-9]\+$' >/dev/null; then echo "Error: Could not extract a valid numerical FIRMWARE_VERSION from '$VERSION_FILE'." exit 1 fiThis explicitly validates that the version contains only decimal digits (no hex, floats, or alphanumeric strings).
Stricter validation prevents misnamed binaries and improves reproducibility.
99-102: Verify output directory exists before copying.The script assumes the
output/directory exists. While the.gitignoreentry suggests it's tracked, consider adding a defensive check to create it if missing:# 3d. Check if the built binary exists if [ ! -f "$BUILD_OUTPUT_FILE" ]; then echo "Error: Build output file '$BUILD_OUTPUT_FILE' not found. Cannot proceed with copying." exit 1 fi +# 3e. Ensure output directory exists for the versioned binary +if [ ! -d "output" ]; then + mkdir -p output +fi - -# 3e. Copy the binary, preserving modification time, access time, and modes (-p)This prevents runtime errors if the output directory is absent (e.g., fresh clone).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.gitignore(1 hunks)build.sh(1 hunks)
🧰 Additional context used
🪛 Shellcheck (0.11.0)
build.sh
[warning] 56-56: Declare and assign separately to avoid masking return values.
(SC2155)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (2)
.gitignore (1)
9-9: Appropriate addition for build artifacts.Adding
tmp/to.gitignorecorrectly excludes temporary build artifacts generated during the toolchain download and extraction process inbuild.sh.build.sh (1)
61-61: Verify Python interpreter availability.The script invokes
python make.py, but many systems now default topython3. Consider adding a version check or fallback, or document that Python 2.7+ is required (or specify the intended version).Can you confirm whether
make.pyis compatible with both Python 2 and Python 3, and whether the repository has any documented Python version requirements? If Python 3 is preferred, consider usingpython3explicitly or adding a check like:command -v python3 >/dev/null && python3 make.py || python make.py.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
build.sh (1)
56-56: Separate declaration and assignment to clarify intent.Assigning the result of a command substitution in the same statement as
exportcan mask errors from the substitution. Consider splitting this into two steps for clarity and to follow shellcheck best practices (SC2155).Apply this diff:
-export PATH="$PATH:$(pwd)/$TOOLCHAIN_DIR/bin" +TOOLCHAIN_BIN="$(pwd)/$TOOLCHAIN_DIR/bin" +export PATH="$PATH:$TOOLCHAIN_BIN"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
build.sh(1 hunks)
🧰 Additional context used
🪛 Shellcheck (0.11.0)
build.sh
[warning] 56-56: Declare and assign separately to avoid masking return values.
(SC2155)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (3)
build.sh (3)
11-27: OS detection logic is robust.The case statement properly handles Linux and macOS detection with fallback error handling for unsupported platforms. Good defensive coding.
85-97: Version extraction and validation are well-implemented.Using POSIX-compliant patterns (
grep,awk, andcasestatement) ensures portability. The validation check for non-empty, numeric-only values prevents downstream issues.
52-71: Build execution has solid error handling.The script properly:
- Sets up the PATH with an absolute path reference
- Checks the exit code of the Python build script
- Provides clear feedback on success or failure
Good practices for robust shell scripting.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
build.sh (2)
36-36: Use-sfor maximum curl portability instead of-#.The
-#flag (hash progress meter) is not universally supported across all curl installations. For better portability in build scripts, prefer-s(silent mode) which is standardly available and still provides reasonable feedback.Apply this diff:
- if ! curl -L -# "$DOWNLOAD_URL" | tar -xjf -; then + if ! curl -L -s "$DOWNLOAD_URL" | tar -xjf -; then
6-6: Add defensive checks for working directory.The script assumes it is executed from the repository root (e.g.,
src/version.hon line 6 and line 79,pwdon line 56). If run from a different directory, the script will fail with cryptic errors. Consider validating or documenting the expected execution context.Add a directory check at the beginning:
+#!/bin/sh + +# Ensure the script runs from the repository root +if [ ! -f "src/version.h" ]; then + echo "Error: This script must be run from the repository root." + exit 1 +fiAlternatively, document this requirement prominently in a
README.mdor inline comment.Also applies to: 56-56
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
build.sh(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (2)
build.sh (2)
62-62: Consider specifying the Python version explicitly.The script calls
python make.pywhich may invoke Python 2 on some systems (depending onPATHand system configuration). For clarity and to avoid surprises, consider specifyingpython3if the project requires Python 3.Verify whether the project targets Python 2, Python 3, or either. If Python 3 is required, update line 62 to use
python3explicitly.
1-121: Overall implementation looks solid.The script demonstrates good error handling, clear messaging, and proper exit codes. The structure is logical and the post-build versioning logic is well thought out. The fix from the previous review (explicit stdin for tar on line 36) has been correctly applied.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
build.sh(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (5)
build.sh (5)
1-7: Strong root validation and portable shebang. The/bin/shensures wide compatibility, and thesrc/version.hcheck effectively prevents execution from incorrect directories with a clear error message.
9-13: Clear configuration setup. Constants are well-named and appropriately scoped. The hardcoded toolchain version ensures reproducible builds, though note this tightly couples the script to GCC 6 2017-Q1.
17-53: Solid toolchain provisioning with robust error handling. OS detection is portable, and the tar command correctly uses explicit stdin (-) as noted in the previous review. Error messages clearly explain what went wrong and hint at root causes (missing curl/tar/network).
56-78: Build execution is clean and defensive. Using$(pwd)for the absolute toolchain path ensures it works from any invocation directory. Proper error checking and clear messaging on build failure. The environment variable export is correctly scoped.
105-127: Robust post-build copy with proper validation. File existence checks are in the right order, output directory creation is safe withmkdir -p, andcp -pcorrectly preserves metadata. Error handling for the copy operation is clear.
…, and better extraction
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
build.sh (1)
17-53: Consider verifying toolchain integrity beyond directory existence.The check on line 17 only verifies that
$TOOLCHAIN_DIRexists, not that it contains expected files (e.g., arm-none-eabi-gcc binary). If extraction fails partially or creates an unexpected structure, the script will skip re-download on subsequent runs and fail during the build step with a confusing error.Consider adding a sanity check for a key binary within the toolchain. For example:
-if [ ! -d "$TOOLCHAIN_DIR" ]; then +if [ ! -d "$TOOLCHAIN_DIR" ] || [ ! -f "$TOOLCHAIN_DIR/bin/arm-none-eabi-gcc" ]; thenThis ensures the toolchain is not only present but also properly extracted.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
build.sh(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-25T17:53:38.853Z
Learnt from: nerdCopter
PR: emuflight/imu-f#19
File: build.sh:90-98
Timestamp: 2025-10-25T17:53:38.853Z
Learning: In the emuflight/imu-f repository, the FIRMWARE_VERSION in src/version.h is typically a 3-digit integer, but can be a 4-digit integer for testing purposes.
Applied to files:
build.sh
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (4)
build.sh (4)
1-13: Clear structure and good defensive checks.The use of POSIX
sh, root-directory validation, and upfront configuration constants demonstrate solid scripting practices.
65-77: Verify python invocation compatibility across supported systems.Line 68 hardcodes
python make.py -C -T F3, which assumes thepythoncommand is available. On many modern systems (especially CI/CD environments), onlypython3may be available.Please confirm:
- Do all supported target systems have
pythonavailable, or should the script detect and usepython3?- Is there a
makewrapper that handles this, or should the script add a fallback check?For example, you might add:
+PYTHON_CMD=$(command -v python3 >/dev/null && echo "python3" || echo "python") -if ! python make.py -C -T F3; then +if ! $PYTHON_CMD make.py -C -T F3; then
85-101: Version extraction and validation are robust.The anchored grep pattern correctly targets only uncommented #define statements, and the decimal validation aligns with your version format (3–4 digit integers). The defensive checks provide clear error messages.
103-127: Binary copy and output handling are solid.The versioned filename, defensive existence checks, and use of
cp -pto preserve metadata follow best practices. Error reporting is consistent and clear.
1 participant
Summary by CodeRabbit
Chores
Build Infrastructure