-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Update ci.yml #11288
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update ci.yml #11288
Conversation
Branch Targeting SuggestionYou've targeted the
If This is an automated suggestion to help route contributions to the appropriate branch. |
ⓘ Your approaching your monthly quota for Qodo. Upgrade your plan PR Compliance Guide 🔍All compliance sections have been disabled in the configurations. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
High-level Suggestion
The PR's removal of the matrix build strategy and SITL builds eliminates crucial compilation verification across multiple hardware targets and operating systems. This reduction to a single build target (MATEKH743) introduces a high risk of undetected regressions for other hardware. [High-level, importance: 10]
Solution Walkthrough:
Before:
# .github/workflows/ci.yml
jobs:
build:
strategy:
matrix:
id: [0, 1, ..., 14] # Build for 15 target groups
steps:
- name: Build targets (${{ matrix.id }})
run: cmake ... -DCI_JOB_INDEX=${{ matrix.id }} ... && ninja ci
upload-artifacts:
# ... aggregates matrix builds
build-SITL-Linux:
# ... builds SITL for Linux
build-SITL-Mac:
# ... builds SITL for macOS
build-SITL-Windows:
# ... builds SITL for Windows
After:
# .github/workflows/ci.yml
jobs:
build:
runs-on: ubuntu-latest
steps:
- name: Checkout source
- name: Install dependencies
- name: Configure build for MATEKH743
run: cmake -B build -G Ninja -D TARGET=MATEKH743 .
- name: Compile firmware
run: cmake --build build
- name: Upload firmware artifact
| - name: Configure build for MATEKH743 | ||
| run: cmake -B build -G Ninja -D TARGET=MATEKH743 . |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: Re-introduce the -DWARNINGS_AS_ERRORS=ON flag in the cmake configuration step to treat compiler warnings as errors, thereby improving code quality. [general, importance: 6]
| - name: Configure build for MATEKH743 | |
| run: cmake -B build -G Ninja -D TARGET=MATEKH743 . | |
| - name: Configure build for MATEKH743 | |
| run: cmake -B build -G Ninja -DWARNINGS_AS_ERRORS=ON -D TARGET=MATEKH743 . |
| - name: Compile firmware | ||
| run: cmake --build build |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: Add a parallel flag like -- -j$(nproc) to the cmake --build build command to speed up firmware compilation. [general, importance: 7]
| - name: Compile firmware | |
| run: cmake --build build | |
| - name: Compile firmware | |
| run: cmake --build build -- -j$(nproc) |
| - name: Upload firmware artifact | ||
| uses: actions/upload-artifact@v4 | ||
| with: | ||
| name: firmware-MATEKH743 | ||
| path: build/**/*.bin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: Configure the upload step to explicitly fail (or warn) when no matching firmware files are produced, so CI doesn’t “succeed” while uploading nothing. [Learned best practice, importance: 6]
| - name: Upload firmware artifact | |
| uses: actions/upload-artifact@v4 | |
| with: | |
| name: firmware-MATEKH743 | |
| path: build/**/*.bin | |
| - name: Upload firmware artifact | |
| uses: actions/upload-artifact@v4 | |
| with: | |
| name: firmware-MATEKH743 | |
| path: build/**/*.bin | |
| if-no-files-found: error |
| jobs: | ||
| build: | ||
| runs-on: ubuntu-latest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: Add a timeout-minutes to the job to prevent stalled builds from running indefinitely due to external dependency or toolchain hangs. [Learned best practice, importance: 5]
| jobs: | |
| build: | |
| runs-on: ubuntu-latest | |
| jobs: | |
| build: | |
| runs-on: ubuntu-latest | |
| timeout-minutes: 30 |
PR Type
Enhancement
Description
Simplified CI workflow to single target build for MATEKH743
Removed matrix strategy and multi-platform SITL builds
Reduced workflow from 277 to 33 lines for maintainability
Added workflow_dispatch trigger for manual builds
Diagram Walkthrough
File Walkthrough
ci.yml
Streamline CI to single MATEKH743 firmware build.github/workflows/ci.yml
Windows
branch