build: pass/fail summary after multi-target make#1288
Conversation
When running `make all` (or `make supported`) across all supported targets, print a final summary line once every target has finished. Changes: - $(VALID_TARGETS) recipe: &&-chain replaced with if/else; per-target pass/fail appended to obj/.build_passed or obj/.build_failed (O_APPEND writes < PIPE_BUF are atomic; no flock needed under -j) - build_summary_init: order-only prereq for $(SUPPORTED_TARGETS); clears marker files at the start of each all/supported run - build_summary: depends on $(SUPPORTED_TARGETS), prints summary line and exits non-zero if any target failed; failed-target list shown only when failed count ≤ 10 (avoids flooding on broken-toolchain runs) - clean_all: removes marker files alongside obj/main/ Closes emuflight#1287 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Previous design absorbed per-target failures (if/else without exit 1),
effectively forcing keep-going behavior for all builds including single
targets like `make HELIOSPRING`.
Fix:
- Add `exit 1` to the failure branch so individual target builds exit
non-zero correctly.
- Remove $(SUPPORTED_TARGETS): | build_summary_init order-only prereq;
it's no longer needed and caused build_summary_init to run for every
single-target invocation.
- Change `all supported:` to a 3-line recipe:
@$(MAKE) build_summary_init # clear markers
-@$(MAKE) $(SUPPORTED_TARGETS) # build; errors absorbed here only
@$(MAKE) build_summary # always prints summary, exits ≠0 on fail
The `-` on the middle line absorbs the sub-make exit code so
build_summary always runs. MAKEFLAGS propagates automatically, so
user-supplied -k / -j are honored by the sub-make.
- Change `build_summary:` to have no prerequisites; it now reads
marker files written during the sub-make.
- Add BUILD_SUMMARY_FAIL_LIST_MAX ?= 10 for user tuning.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
build_summary_init and build_summary were phony targets reachable by any user. Inline both into the all/supported recipe so the summary machinery is not a user-surfaced interface — it's an implementation detail of make all. clean_all still references the marker file variables directly, so cleanup is unaffected. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Define summary_build macro that takes a target list, runs it through a sub-make (inheriting MAKEFLAGS so -k/-j propagate naturally), and always prints the pass/fail report regardless of build outcome. Apply to every multi-target recipe: all, supported, all_with_unsupported, unsupported, targets-group-1..11, targets-group-rest For make HELIOSPRING FOXEERF722V4 (raw arbitrary invocations) the exit code is already correct; summary requires going through one of the above named targets, which cover all CI-relevant use cases. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
When all MAKECMDGOALS are VALID_TARGETS and there are 2+, set
IN_SUMMARY_OUTER at parse time and restructure the build graph:
- $(VALID_TARGETS): becomes a no-op (@:)
- _auto_summary is injected as a prerequisite of the first goal;
its recipe calls summary_build which does the real work
- summary_build passes IN_SUMMARY_BUILD=1 to its sub-make so
the inner invocation skips detection and uses the real recipe
Result:
make HELIOSPRING FOXEERF722V4 KAKUTEF7 -> summary + correct exit
make HELIOSPRING -> real recipe, correct exit
make all / targets-group-* -> unchanged
Conditions that do NOT trigger interception (IN_SUMMARY_OUTER stays unset):
- Single target (words == 1)
- Mixed goals where some are not VALID_TARGETS (e.g. make HELIOSPRING all)
- Inner sub-make (IN_SUMMARY_BUILD=1 set)
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Relax the IN_SUMMARY_OUTER detection from 'all goals must be valid AND 2+' to '2+ total goals AND at least one is a valid target'. This lets make HELIOSPRING FOXEERF405 FOOBAR -j 6 be intercepted even though FOOBAR is not a known target. In the outer make (IN_SUMMARY_OUTER branch): - Compute _INVALID_GOALS = goals not in VALID_TARGETS - Add a no-op recipe for them so make does not emit a redundant 'no rule to make target' error (the inner make already reports it) In summary_build: - Replace -@$(MAKE) (which discarded the exit code) with _exit=0; $(MAKE) ... || _exit=$$? so the inner make exit code is captured alongside the marker-based failed count - Final exit condition: [ _exit -eq 0 ] && [ failed -eq 0 ] This exits non-zero for both recipe failures (marker-based) and 'no rule' / other make errors (exit-code-based) Summary for make HELIOSPRING FOXEERF405 FOOBAR -j 6: - inner make: HELIOSPRING + FOXEERF405 build, FOOBAR 'no rule' -> _exit=2 - summary: '2 succeeded, 0 failed' - exit code: non-zero (from _exit) ✓ Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Replace the firstword-dependency trick with a flat rule:
$(MAKECMDGOALS): _auto_summary
@:
This makes ALL command-line goals depend on _auto_summary, so when
_auto_summary exits non-zero (either recipe failure or 'no rule to
make target' for an unknown goal), every goal fails and the outer
make exits non-zero. Previously only the first goal depended on
_auto_summary, letting the remaining no-op goals 'succeed' and
causing make to exit 0 despite the error.
The _INVALID_GOALS workaround is no longer needed — unknown goals get
a recipe (@:) from the multi-target rule itself, suppressing the
double 'no rule' error while the inner make's 'no rule' is still
reported and captured in _exit.
Also remove BUILD_SUMMARY_FAIL_LIST_MAX: always print failed targets,
no artificial truncation threshold.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…4.4+ In GNU Make 4.4+, when a non-phony target has a trivial (@:) recipe and its prerequisite fails, make reports the error but still exits 0. It considers the target 'not needing a rebuild' rather than 'failed'. Fix: declare $(MAKECMDGOALS) as .PHONY inside the IN_SUMMARY_OUTER block. Phony targets must always be rebuilt, so a failed prerequisite (_auto_summary) marks them as failed and make exits non-zero. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
GNU Make 4.4+ does not reliably propagate a failed prerequisite to the outer make's exit code when the dependent goals have trivial (@:) recipes — make reports the error but exits 0. This was true even with .PHONY declarations on the goals. Fix: abandon the _auto_summary prerequisite pattern entirely. Instead, put summary_build directly in the FIRST command-line goal's recipe, and make the remaining goals @: no-ops. When a goal's OWN recipe exits non-zero, make unconditionally exits non-zero — this is a direct path with no dependency-propagation ambiguity. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughMakefile now tracks per-target pass/fail results with marker files, summarizes multi-target builds through a guarded sub-make flow, rewires aggregate and direct multi-goal target handling, and removes the summary markers during ChangesBuild summary and multi-goal execution
Sequence Diagram(s)sequenceDiagram
participant User
participant OuterMake
participant InnerMake
participant TargetRecipe
participant BUILD_SUMMARY_PASSED
participant BUILD_SUMMARY_FAILED
User->>OuterMake: make T1 T2
OuterMake->>InnerMake: summary_build runs targets in a guarded sub-make
loop each requested target
InnerMake->>TargetRecipe: run target recipe
alt success
TargetRecipe->>BUILD_SUMMARY_PASSED: append target name
else failure
TargetRecipe->>BUILD_SUMMARY_FAILED: append target name
end
end
InnerMake-->>OuterMake: exit status and summary counts
OuterMake-->>User: print pass/fail summary
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai, AI generated comment — please review this PR. It modifies only |
|
@coderabbitai, AI generated comment — please review this PR. It modifies only |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== repo =="
git remote -v | head -n 5 || true
echo
echo "== changed files in PR branch vs base =="
git diff --name-only origin/master...HEAD || git diff --name-only HEAD~1...HEAD || true
echo
echo "== Makefile diff =="
git diff --unified=80 origin/master...HEAD -- Makefile || git diff --unified=80 HEAD~1...HEAD -- Makefile || true
echo
echo "== search for summary-related identifiers in Makefile =="
rg -n -C4 'summary_build|IN_SUMMARY_OUTER|build_passed|build_failed|MAKECMDGOALS|unknown|notdir|filter' Makefile || true
echo
echo "== first 260 lines of Makefile =="
sed -n '1,260p' Makefile
echo
echo "== lines 260-520 of Makefile =="
sed -n '260,520p' MakefileLength of output: 39519
Other than that, the ✅ Action performedReview finished.
|
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== PR metadata =="
gh pr view 1288 --json title,baseRefName,headRefName,files,body,url
echo
echo "== Changed files =="
gh pr diff 1288 --name-only
echo
echo "== Makefile summary-related symbols =="
rg -n -C3 'summary_build|IN_SUMMARY_OUTER|build_passed|build_failed|MAKECMDGOALS|wordlist|firstword|targets-group|all_with_unsupported|unsupported|supported' Makefile || true
echo
echo "== Diff for Makefile =="
gh pr diff 1288 -- Makefile
echo
echo "== Top of Makefile =="
sed -n '1,260p' Makefile
echo
echo "== Mid/Late Makefile around group targets =="
sed -n '260,520p' MakefileLength of output: 8828 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== Makefile lines 335-395 =="
sed -n '335,395p' Makefile | nl -ba
echo
echo "== Makefile lines 472-575 =="
sed -n '472,575p' Makefile | nl -ba
echo
echo "== Search for marker writes =="
rg -n -C3 '\.build_passed|\.build_failed|BUILD_SUMMARY_PASSED|BUILD_SUMMARY_FAILED' Makefile
echo
echo "== PR files patch via GitHub API =="
gh api repos/emuflight/EmuFlight/pulls/1288/files --paginate \
--jq '.[] | select(.filename=="Makefile") | .patch'Length of output: 887 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== Makefile lines 335-395 =="
sed -n '335,395p' Makefile | cat -n
echo
echo "== Makefile lines 472-575 =="
sed -n '472,575p' Makefile | cat -n
echo
echo "== Search for marker writes =="
rg -n -C4 '\.build_passed|\.build_failed|BUILD_SUMMARY_PASSED|BUILD_SUMMARY_FAILED' Makefile || true
echo
echo "== PR Makefile patch via GitHub API =="
gh api repos/emuflight/EmuFlight/pulls/1288/files --paginate \
--jq '.[] | select(.filename=="Makefile") | .patch // "<no patch>"'Length of output: 15916
Blocking issue
One possible direction is to only enable the outer path when the goals are board targets plus optional unknown names, but not when any known non-board target is present. For example: # sketch: keep interception for `make HELIOSPRING FOXEERF405 [FOOBAR]`
# but skip it for `make clean HELIOSPRING` / `make supported HELIOSPRING`
SUMMARY_META_GOALS := all supported all_with_unsupported unsupported \
targets-group-1 targets-group-2 targets-group-3 targets-group-4 \
targets-group-5 targets-group-6 targets-group-7 targets-group-8 \
targets-group-9 targets-group-10 targets-group-11 targets-group-rest \
clean clean_all all_clean clean_test flash st-flash binary hex help targets
ifndef IN_SUMMARY_BUILD
_DIRECT_VALID := $(filter $(VALID_TARGETS),$(MAKECMDGOALS))
_DIRECT_META := $(filter $(SUMMARY_META_GOALS),$(MAKECMDGOALS))
ifneq ($(words $(MAKECMDGOALS)),1)
ifneq ($(words $(_DIRECT_VALID)),0)
ifeq ($(words $(_DIRECT_META)),0)
IN_SUMMARY_OUTER := 1
endif
endif
endif
endifIf you want, I can do another pass after that is tightened up. ✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Makefile`:
- Around line 346-354: The build summary in the Makefile only counts entries
from BUILD_SUMMARY_FAILED, so direct unknown goals can make the summary show
zero failures even though _exit is non-zero. Update the summary logic around the
passed/failed counts so direct unknown goals are added to the failed marker
list, while still excluding known non-build goals handled by the outer guard;
use the existing summary block and failure markers to locate the fix.
- Around line 324-329: The current IN_SUMMARY_OUTER guard in the Makefile still
activates for mixed goal invocations when _DIRECT_VALID finds any VALID_TARGETS
entry, which can interfere with non-build goals like clean. Update the
IN_SUMMARY_BUILD block so IN_SUMMARY_OUTER is set only for direct build-target
commands, or explicitly filter out known non-summary goals before assigning it.
Make the same adjustment in the duplicated guard logic referenced by the later
Makefile section so both paths behave consistently.
- Around line 313-345: The summary marker files used by summary_build are shared
across invocations, so concurrent top-level runs can interfere with each other’s
pass/fail tracking. Update summary_build to create a unique per-invocation
marker directory or file prefix, and pass those marker paths into the recursive
$(MAKE) call. Make sure the existing BUILD_SUMMARY_PASSED and
BUILD_SUMMARY_FAILED handling in the Makefile summary flow uses the
invocation-specific paths instead of the global $(BIN_DIR)/.build_passed and
.build_failed markers.
- Around line 488-489: The build recipe for VALID_TARGETS has an extra redundant
@ after $(V0), which expands to an invalid `@ `@echo`` when verbosity is off.
Update the target rule to use $(V0) with echo directly in the Makefile recipe so
the command remains valid for both V=0 and normal verbosity. Use the
VALID_TARGETS rule and the $(V0) verbosity helper as the key locations to
adjust.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b625f53f-eccf-4981-8713-c9731799a5ad
📒 Files selected for processing (1)
Makefile
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: Codacy Static Code Analysis
🧰 Additional context used
🪛 checkmake (0.3.2)
Makefile
[warning] 488-488: Target body for "$(VALID_TARGETS)" exceeds allowed length of 5 lines (9).
(maxbodylength)
[warning] 479-479: Required target "all" is missing from the Makefile.
(minphony)
[warning] 479-479: Required target "test" is missing from the Makefile.
(minphony)
🔇 Additional comments (2)
Makefile (2)
411-470: LGTM!
537-537: LGTM!
Four fixes from PR emuflight#1288 inline review: 1. Concurrent marker collision (major): replace global .build_passed / .build_failed files with a per-invocation mktemp directory. Paths are passed to the sub-make via BUILD_SUMMARY_PASSED / BUILD_SUMMARY_FAILED command-line overrides so they land in each invocation's own temp dir. A trap ensures cleanup on EXIT even when the build is interrupted. 2. Non-build goal interception (major): add _SUMMARY_SKIP_GOALS and _DIRECT_NON_BUILD to the detection block. If any goal in MAKECMDGOALS is a known non-build target (clean, clean_test, clean_all, all_clean, binary_hex, test, or NOBUILD_TARGETS), IN_SUMMARY_OUTER is not set so mixed invocations like `make clean HELIOSPRING` fall through to normal make processing instead of being redirected through summary_build. 3. Unknown goals not counted (major): compute _INVALID_GOALS inside the ifdef IN_SUMMARY_OUTER block and pass them as the new $(2) argument to summary_build. The macro pre-populates the failed marker with each invalid goal before the sub-make runs so they appear in the `N failed` count and the failed-target listing. 4. $(V0) @echo double-@ bug (nitpick): remove the redundant @ from $(V0) @echo in the $(VALID_TARGETS) recipe. When V=0, V0=@, making the expanded line `@ @echo`; make strips the first @ then passes `@echo` to the shell, which fails. Correct form is $(V0) echo. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
@coderabbitai, AI generated comment Addressed all four inline findings from this review round. Fixed in commit e19d53e: Concurrent marker collision ( Non-build goal interception ( Unknown goals not counted in failed summary (
|
|
Tip For best results, initiate chat on the files or code changes.
The per-invocation No further concerns on those points from me. 🐇✅ If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
Replace $(V0) with @ in the $(VALID_TARGETS) per-target recipe.
With V unset (the common default), V0 is empty — no @ prefix — so
make echoes the entire if/then/else block before running it. The
visible progress messages ("Building X", "Building X succeeded.",
etc.) come from the echo commands inside the recipe, not from make's
recipe echo. Using @ always suppresses the recipe echo without
affecting any of those messages, matching the pattern used by every
other progress line in the Makefile (e.g. @echo "Linking...").
The previous $(V0) approach was a left-over from the verbosity
system used for compiler/linker lines: those use V0 so that the
command itself is hidden in quiet mode (V=0) but visible in verbose
mode (V=1). That convention does not apply here — we want the
recipe silent at all verbosity levels.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Add an ifdef IN_SUMMARY_BUILD split inside the else branch so that
every invocation of a VALID_TARGET — single or multi — goes through
the summary_build macro and prints a consistent:
=== Build summary: 1 succeeded, 0 failed ===
line at the end.
Structure:
ifdef IN_SUMMARY_OUTER → multi-target outer: first goal calls
summary_build(MAKECMDGOALS,_INVALID_GOALS)
else
ifdef IN_SUMMARY_BUILD → inner sub-make: run the actual build and
write to marker files (no summary printed
here — outer summary_build does that)
else → direct single-target: call summary_build($@)
which runs a sub-make with IN_SUMMARY_BUILD=1,
then prints the 1-target summary
endif
endif
Named-group targets (all, supported, targets-group-*) already call
summary_build directly and are unaffected.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
@coderabbitai, AI generated comment Two follow-up commits since the last review round:
Previously,
No new code paths — all invocations now funnel through the single |
|
Tip For best results, initiate chat on the files or code changes.
Using plain ✏️ Learnings added
|
|
@coderabbitai, AI generated comment Two additional commits since the last review:
The
The
No new logic — single targets now use the same macro as every other path. Verified 2026-06-26:
|
|
Tip For best results, initiate chat on the files or code changes.
Using plain I don’t have further concerns on these follow-ups. |
IN_SUMMARY_OUTER was suppressed for clean/utility targets but not for
named-group targets (all, supported, targets-group-*, etc.) or other
phony targets (flash, help, version, etc.).
A mixed invocation like `make all HELIOSPRING` would incorrectly
trigger IN_SUMMARY_OUTER: `all` is not in VALID_TARGETS and not in
the old _SUMMARY_SKIP_GOALS, so _DIRECT_NON_BUILD was empty and
IN_SUMMARY_OUTER was set. The outer summary_build would then call the
inner make with `all HELIOSPRING`, where `all`'s own recipe would
call summary_build again — nested and incorrect.
Fix: enumerate all known non-board-build targets in _SUMMARY_SKIP_GOALS:
- named-group: all, supported, all_with_unsupported, unsupported,
targets-group-1 through targets-group-11, targets-group-rest
- utility: flash, st-flash, binary, hex, unbrick, version, help,
targets, target-mcu, targets-by-mcu, cppcheck,
check-target-independence, check-fastram-usage-correctness
- existing: clean, clean_test, clean_all, all_clean, binary_hex,
test, $(NOBUILD_TARGETS)
Any goal in MAKECMDGOALS that matches _SUMMARY_SKIP_GOALS causes
_DIRECT_NON_BUILD to be non-empty, suppressing IN_SUMMARY_OUTER and
falling through to normal make processing.
Addresses CodeRabbit finding: emuflight#1288 (comment)
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
@coderabbitai, AI generated comment Fixed in Extended
Any goal in |
|
Tip For best results, initiate chat on the files or code changes.
Expanding Looks good from my side. ✏️ Learnings added
|
|
@coderabbitai, AI generated comment Please do a full review pass on the current branch tip (
|
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== HEAD =="
git rev-parse --short=12 HEAD
git log --oneline -n 5
echo
echo "== Makefile summary-related matches =="
rg -n -C4 'summary_build|IN_SUMMARY_OUTER|IN_SUMMARY_BUILD|BUILD_SUMMARY_PASSED|BUILD_SUMMARY_FAILED|_SUMMARY_SKIP_GOALS|_DIRECT_NON_BUILD|_INVALID_GOALS|VALID_TARGETS|binary_hex|MAKECMDGOALS|firstword|wordlist' Makefile || true
echo
echo "== Makefile lines 330-430 =="
sed -n '330,430p' Makefile | cat -n
echo
echo "== Makefile lines 470-560 =="
sed -n '470,560p' Makefile | cat -n
echo
echo "== Makefile lines 560-640 =="
sed -n '560,640p' Makefile | cat -nLength of output: 22879
1. Prior concerns
2. Complete solution
I do not see a remaining logic gap in the intended paths you listed. 3. CI failure propagation
That means CI should correctly fail for both:
Bottom line: from my review, the implementation at ✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Makefile`:
- Around line 329-339: The summary interception logic is treating per-target
clean aliases as build targets because _SUMMARY_SKIP_GOALS and the
_DIRECT_NON_BUILD filter do not include clean_<target> and <target>_clean.
Update the Makefile goal filtering around _SUMMARY_SKIP_GOALS/_DIRECT_NON_BUILD
so these aliases are recognized as non-build goals, ensuring mixed commands like
make HELIOSPRING clean_HELIOSPRING do not enter IN_SUMMARY_OUTER or get counted
as invalid targets.
- Around line 501-508: The Makefile summary-build wrapper is still forwarding
invalid goals into the inner make invocation, which causes the first bad target
to stop processing the rest. Update the logic around the summary_build call so
the first target only passes valid goals, using the existing _INVALID_GOALS and
VALID_TARGETS filtering in the Makefile target handling. Keep the placeholder
targets and the summary_build entrypoint intact, but ensure the inner make
receives only the filtered valid goals.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4b93c67d-49d9-4b44-8739-ad76dc8e8b24
📒 Files selected for processing (1)
Makefile
📜 Review details
⏰ Context from checks skipped due to timeout. (13)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build (targets-group-3)
- GitHub Check: build (targets-group-5)
- GitHub Check: build (targets-group-rest)
- GitHub Check: build (targets-group-4)
- GitHub Check: build (targets-group-7)
- GitHub Check: build (targets-group-11)
- GitHub Check: build (targets-group-6)
- GitHub Check: build (targets-group-10)
- GitHub Check: build (targets-group-8)
- GitHub Check: build (targets-group-1)
- GitHub Check: build (targets-group-9)
- GitHub Check: build (targets-group-2)
🧰 Additional context used
🪛 checkmake (0.3.2)
Makefile
[warning] 529-529: Target "$(VALID_TARGETS)" defined multiple times (lines 515 and 529).
(uniquetargets)
[warning] 515-515: Target body for "$(VALID_TARGETS)" exceeds allowed length of 5 lines (9).
(maxbodylength)
[warning] 503-503: Required target "all" is missing from the Makefile.
(minphony)
[warning] 503-503: Required target "test" is missing from the Makefile.
(minphony)
🔇 Additional comments (4)
Makefile (4)
351-376: LGTM!
432-492: LGTM!
515-530: LGTM!
571-571: LGTM!
Two problems with the previous hardcoded _SUMMARY_SKIP_GOALS list: 1. Brittle — new phony targets added to the Makefile required a manual entry here or they would incorrectly trigger IN_SUMMARY_OUTER. 2. Incomplete — openocd-gdb, targets-f3/f4/f7/h7, and junittest were missing, leaving those mixed invocations unguarded. Fix: consolidate ALL phony target declarations into the single .PHONY line at the top of the summary section (before the detection block). Then derive _SUMMARY_SKIP_GOALS from it: _SUMMARY_SKIP_GOALS := $(filter-out $(VALID_TARGETS), $(.PHONY)) $(NOBUILD_TARGETS) $(.PHONY) is fully populated by the time ":=" evaluates it because the declaration appears before the detection block. $(NOBUILD_TARGETS) covers board names with .nomk files (not phony, not in VALID_TARGETS). Any future phony target added to .PHONY is automatically excluded from IN_SUMMARY_OUTER with no further changes required. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
@coderabbitai, AI generated comment Please do a thorough inspection of commit What changed and why The previous hardcoded The fix: all phony targets are now declared in a single consolidated _SUMMARY_SKIP_GOALS := $(filter-out $(VALID_TARGETS), $(.PHONY)) $(NOBUILD_TARGETS)
Please specifically verify:
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== HEAD and target commit =="
git rev-parse --short=12 HEAD
git log --oneline -n 8
echo
git show --stat --oneline --no-patch 2117b263a6 || true
echo
echo "== Makefile diff for commit 2117b263a6 =="
git show --unified=80 --format=medium 2117b263a6 -- Makefile || true
echo
echo "== summary-related locations in current Makefile =="
rg -n -C4 '\.PHONY|_SUMMARY_SKIP_GOALS|IN_SUMMARY_OUTER|_DIRECT_NON_BUILD|_DIRECT_VALID|_INVALID_GOALS|summary_build|VALID_TARGETS|NOBUILD_TARGETS|targets-group|all_with_unsupported|supported|unsupported' Makefile || true
echo
echo "== current Makefile around consolidated .PHONY and detection block =="
sed -n '285,370p' Makefile | cat -n
echo
echo "== current Makefile around named-group targets and summary_build outer interception =="
sed -n '430,560p' Makefile | cat -n
echo
echo "== current Makefile around VALID_TARGETS rule / IN_SUMMARY_BUILD branch =="
sed -n '560,635p' Makefile | cat -n
echo
echo "== isolated GNU make experiment: does \$(.PHONY) expand to declared phony targets? =="
tmpdir="$(mktemp -d)"
trap 'rm -rf "$tmpdir"' EXIT
cat > "$tmpdir/Makefile" <<'EOF'
.PHONY: all clean foo bar
NOW_SIMPLE := $(.PHONY)
NOW_RECURSIVE = $(.PHONY)
print:
`@echo` "NOW_SIMPLE=[$(NOW_SIMPLE)]"
`@echo` "NOW_RECURSIVE=[$(NOW_RECURSIVE)]"
`@echo` "DIRECT=[$(.PHONY)]"
later:
@:
.PHONY: later baz
AFTER_SIMPLE := $(.PHONY)
AFTER_RECURSIVE = $(.PHONY)
print-after:
`@echo` "AFTER_SIMPLE=[$(AFTER_SIMPLE)]"
`@echo` "AFTER_RECURSIVE=[$(AFTER_RECURSIVE)]"
`@echo` "DIRECT_AFTER=[$(.PHONY)]"
EOF
echo "-- make version --"
make --version | head -n 1
echo "-- running print target --"
make -s -f "$tmpdir/Makefile" print || true
echo "-- running print-after target --"
make -s -f "$tmpdir/Makefile" print-after || true
echo
echo "== isolated GNU make experiment: can make database reveal phony targets? =="
make -pn -f "$tmpdir/Makefile" print 2>/dev/null | rg -n '^[.]PHONY|^NOW_SIMPLE|^AFTER_SIMPLE|^NOW_RECURSIVE|^AFTER_RECURSIVE' || trueLength of output: 35913 🌐 Web query:
Length of output: 12770 🌐 Web query:
Length of output: 12676
Main concernI do not think From the GNU Make docs, That makes this line at Line 339 look unsafe: _SUMMARY_SKIP_GOALS := $(filter-out $(VALID_TARGETS), $(.PHONY)) $(NOBUILD_TARGETS)If Answers to your checklist1. GNU Make
|
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…Y_PHONY_GOALS $(.PHONY) is a special *target*, not a special variable — it expands to empty in GNU Make 4.4.1 (verified). The previous commit's derivation collapsed _SUMMARY_SKIP_GOALS to just $(NOBUILD_TARGETS), re-opening all mixed-goal interception bugs (make all HELIOSPRING, make clean HELIOSPRING, etc.). Fix using a single-source-of-truth variable SUMMARY_PHONY_GOALS that drives both the .PHONY declaration and _SUMMARY_SKIP_GOALS: SUMMARY_PHONY_GOALS := clean ... all supported targets-group-* ... .PHONY: $(SUMMARY_PHONY_GOALS) _SUMMARY_SKIP_GOALS := $(SUMMARY_PHONY_GOALS) $(NOBUILD_TARGETS) Maintainability is preserved: new phony targets are added to SUMMARY_PHONY_GOALS once and both .PHONY and _SUMMARY_SKIP_GOALS pick them up automatically. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
@coderabbitai, AI generated comment You are correct — Fixed in SUMMARY_PHONY_GOALS := \
clean clean_test clean_all all_clean binary_hex \
all supported all_with_unsupported unsupported \
targets-group-1 ... targets-group-rest \
flash st-flash binary hex unbrick openocd-gdb \
version help targets target-mcu targets-by-mcu \
targets-f3 targets-f4 targets-f7 targets-h7 \
cppcheck test junittest \
check-target-independence check-fastram-usage-correctness
.PHONY: $(SUMMARY_PHONY_GOALS)
_SUMMARY_SKIP_GOALS := $(SUMMARY_PHONY_GOALS) $(NOBUILD_TARGETS)Single source of truth: |
|
Tip For best results, initiate chat on the files or code changes.
Using ✏️ Learnings added
|
Finding 1 — pass only valid goals to the inner make (Makefile:516) summary_build was called with the full $(MAKECMDGOALS) including invalid/unknown goals as $(1). Without -k the inner make stops on the first unknown goal and never builds the valid targets that follow. Invalid goals are already pre-counted in the failed marker via $(2); they do not need to reach the sub-make. Fix: compute _VALID_GOALS and pass that as $(1) instead of $(MAKECMDGOALS). Finding 2 — exclude per-target clean aliases from interception clean_<target> and <target>_clean are generated by CLEAN_TARGETS / TARGETS_CLEAN but are not in SUMMARY_PHONY_GOALS. A command like `make HELIOSPRING clean_HELIOSPRING` triggered IN_SUMMARY_OUTER and counted the clean alias as a failed build target. Fix: add clean_% and %_clean patterns to the $(filter) call for _DIRECT_NON_BUILD. GNU Make's $(filter) supports % wildcards, so these match any clean_<name> or <name>_clean goal without enumerating all targets. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
@coderabbitai, AI generated comment Both findings addressed in Finding 1 — pass only valid goals to the inner make (line 516) Finding 2 — per-target clean aliases not in |
|
Tip For best results, initiate chat on the files or code changes.
Passing only Looks good from my side. 🐇 ✏️ Learnings added
|
|
@coderabbitai, are ALL concerns resolved? even nitpicks? |
|
Tip For best results, initiate chat on the files or code changes.
At this point I’m not seeing any remaining:
So on the current branch state discussed above, I consider the |
|
Tip For best results, initiate chat on the files or code changes.
That includes:
So at this point I’m not seeing any remaining blockers, majors, or nitpicks in the current approach. |
|
@coderabbitai, AI generated comment Both findings were already addressed in
Pass only valid goals to the inner make |
|
Tip For best results, initiate chat on the files or code changes.
Those two concerns are closed from my side:
No further concern on either point. 🐇 |
The old recipe called $(MAKE) TARGET=$${target} for each matching
target — a variable override, not a goal. This bypassed the
$(VALID_TARGETS) recipe entirely so summary_build was never invoked
and targets-f3/f4/f7/h7 never printed build statistics.
Fix: collect all matching targets into a shell variable first, then
call $(MAKE) $$_targets with them as goals. Two or more targets
triggers IN_SUMMARY_OUTER and an aggregate summary; a single match
takes the single-target summary_build path. Both print statistics.
Removes the old fail-fast exit-1-per-target logic; summary_build
already records and reports all failures via marker files.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
@coderabbitai, everything account for as of fc3b163 ? will ALL |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== HEAD =="
git rev-parse --short=12 HEAD
git log --oneline -n 8
echo
echo "== show target commit if present =="
git show --stat --oneline --no-patch fc3b163 || true
echo
echo "== summary-related sections in Makefile =="
rg -n -C4 'SUMMARY_PHONY_GOALS|_SUMMARY_SKIP_GOALS|_DIRECT_NON_BUILD|_DIRECT_VALID|_VALID_GOALS|_INVALID_GOALS|IN_SUMMARY_OUTER|IN_SUMMARY_BUILD|summary_build|MAKECMDGOALS|clean_%|%_clean|NOBUILD_TARGETS|VALID_TARGETS|targets-group|all_with_unsupported|supported|unsupported|openocd-gdb|targets-f3|targets-f4|targets-f7|targets-h7|junittest' Makefile || true
echo
echo "== Makefile around phony + detection block =="
sed -n '300,390p' Makefile | cat -n
echo
echo "== Makefile around summary_build and group targets =="
sed -n '430,560p' Makefile | cat -n
echo
echo "== Makefile around VALID_TARGETS/NOBUILD_TARGETS rules =="
sed -n '560,640p' Makefile | cat -n
echo
echo "== phony targets declared in Makefile =="
rg -n '^[.]PHONY:' Makefile || trueLength of output: 27369
What now looks covered:
The one place I’d still not call “all options” in the literal sense is the caveat already documented in the file: Line 437 through Line 438 still warns that combining multiple group/meta targets in one command, such as So my bottom line is:
I’m not seeing any remaining blocker, major, or nitpick beyond that explicit documented caveat. |
|
@coderabbitai approve |
✅ Action performedComments resolved and changes approved. |
1 participant
AI Generated pull-request
Closes #1287
Summary
make HELIOSPRING), named-group (make all,make supported, alltargets-group-*,make all_with_unsupported,make unsupported), and arbitrary multi-target (make HELIOSPRING FOXEERF405) — prints=== Build summary: N succeeded, M failed ===via a singlesummary_buildcanned recipemake HELIOSPRING FOOBAR) are counted in the failed total, listed by name, and cause a non-zero exit-kremains user-controlled; not forced by the Makefilemake build_summaryis not a user-callable targetImplementation notes
A
define summary_buildcanned recipe is the single implementation point for allsummary logic. Named-group targets call it directly; multi-target direct invocations
are intercepted at parse time via
$(MAKECMDGOALS)and routed through it; single-targetdirect invocations call it from the
$(VALID_TARGETS)rule.The recipe uses a per-invocation
mktemp -ddirectory for marker files (concurrent-CI-safe),pre-populates invalid goals into the failed marker before the inner make runs, and captures
the inner make exit code for "no rule to make target" errors. A
trapensures temp dircleanup on EXIT.
Test plan
make HELIOSPRING(single target) — 1 succeeded, 0 failed, exit 0make targets-group-1 -j 48— 39 succeeded, 0 failed, exit 0make HELIOSPRING FOXEERF405 FOORBAR -j 48— 2 succeeded, 1 failed (FOORBAR listed), exit 1make FOOBAR— make native "no rule" error, exit 2make all -j 48— 459 succeeded, 0 failed, exit 0Summary by CodeRabbit
makebehavior for multiple targets so results are summarized clearly instead of failing silently or inconsistently.clean_allnow clears build summary state to avoid stale results on later runs.