ci: an empty selection must build nothing, plus selector follow-ups - #3845
Merged
Conversation
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The new test helper emits unescaped shell variable assignments, which can make the test environment-dependent (e.g., TMPDIR paths with spaces) and should be fixed to avoid flaky failures.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR fixes a CI workflow edge case where a legitimate empty .build.families: [] selection (meaning “build nothing”) was incorrectly treated as “unusable,” causing the workflow to fall open and rebuild all CMake legs. It also adds targeted unit tests that execute the build.yml “build-axis extras” shell block to prevent regressions across the empty/scoped/rejected cases.
Changes:
- Update
.github/workflows/build.ymlto distinguish between an empty family list (keep the all-empty matrix; skip builds) and a rejected/unusable family regex (fall open to full matrix). - Rename
FAM_*variables toFAMILY_*for clarity in the workflow block. - Add tests in
test_ci_metrics.pythat extract and run the workflow shell block against representative selection shapes.
File summaries
| File | Description |
|---|---|
.github/workflows/build.yml |
Correctly branches between empty vs rejected family selections to avoid unintended full rebuilds. |
test/hil/test/test_ci_metrics.py |
Adds execution-based tests to validate the workflow’s build-extras block behavior for empty/scoped/rejected selections. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+468
to
+471
| with open(sh, 'w') as fh: | ||
| fh.write('BUILD_SELECT_FILE=' + selp + '\n') | ||
| fh.write("MATRIX_JSON='" + matrix + "'\n") | ||
| fh.write(block) |
Size Difference ReportBecause TinyUSB code size varies by port and configuration, the metrics below represent the averaged totals across all example builds. Note: If there is no change, only one value is shown. Changes >1% in sizeNo entries. Changes <1% in sizeNo entries. No changes
|
Contributor
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. |
Hardware-in-the-loop (HIL) Test Reporthfp-iar✅ 56 passed · ❌ 0 failed · ⚪ 0 skipped · blank not run
hfp.json✅ 56 passed · ❌ 0 failed · ⚪ 0 skipped · blank not run
tinyusb-esp.json✅ 21 passed · ❌ 3 failed · ⚪ 0 skipped · blank not run
tinyusb.json
✅ 345 passed · ❌ 36 failed · ⚪ 17 skipped · blank not run
|
|
hathach
added a commit
that referenced
this pull request
Aug 24, 2026
The extras-block probe wrote BUILD_SELECT_FILE and MATRIX_JSON by raw string concatenation, so a TMPDIR containing a space split the assignment and failed the test for a reason with nothing to do with the block under test. Reproduced with TMPDIR="/tmp/has space": test_a_real_family_list_stays_scoped fails before the fix and passes after, and the whole suite is green under both. Found by Copilot on #3845.
hathach
commented
Aug 24, 2026
| with open(sh, 'w') as fh: | ||
| # shlex.quote, not hand-rolled quoting: a TMPDIR with a space in it | ||
| # made this fail for a reason that had nothing to do with the block | ||
| fh.write('BUILD_SELECT_FILE=' + shlex.quote(selp) + '\n') |
Owner
Author
There was a problem hiding this comment.
Fixed in b7c2f80. Reproduced first: with TMPDIR="/tmp/has space", test_a_real_family_list_stays_scoped fails before the change and the whole suite is green after. Both assignments now go through shlex.quote.
MATRIX_JSON was the less likely of the two (it is JSON, so no single quotes to break out of), but quoting both is the right habit rather than reasoning about which values happen to be safe today.
.build.families is read twice in the build-extras block: as a `|`-joined regex for the metrics artifact pattern, and implicitly as "is anything selected". An EMPTY list and one REJECTED by the charset guard both leave the regex empty and mean opposite things, but the branch tested only `-z "$FAMILY_REGEX"` - so every nothing-selected PR took the fall-open path and DISCARDED the correct all-empty matrix ci_set_matrix had just produced. Both merged PRs show it: #3842 (a skill, README.rst and .gitignore) and #3840 (test/hil/** plus docs) each rebuilt all 74 cmake legs after the selector had correctly chosen no families. The HIL axis was right in both - rig jobs skipped on #3842, full rig on #3840 per rule 2 - because it never passes through this block, and no UNSCOPED marker appeared because ci_set_matrix behaved perfectly; the override happens in the shell afterwards. That combination made it look like the classifier was at fault when it was not. Now branches on which case it is: a rejected list falls open exactly as before (all three values drop together), while an empty list keeps the all-empty matrix so every leg skips, sets build_filtered=false because nothing was built and there is nothing to compare a baseline against, and leaves EXAMPLE_MAP at '{}'. Renames FAM_* to FAMILY_* - FAM meant hw/bsp board family, and the abbreviation read like it might be something else in a file that also says "family" for the per-family example map and the get_deps family tokens. The block had no test at all, which is how this shipped twice. test_ci_metrics now extracts it from build.yml and runs it for real against all three shapes; the empty case fails with `129 != 0` against master, and the scoped case with `'' != 'stm32f4|rp2040'`. The probe quotes its shell assignments with shlex.quote, so a TMPDIR containing a space cannot fail it for an unrelated reason.
Every repo-tree glob interpolated repo_root unescaped, so a checkout at a path containing a glob metacharacter resolved nothing and the selector answered "nothing to build" instead of falling open. Reproduced from /tmp/glob[1]test: families=0 where the same diff gives 30 from a normal path - every toolchain then gets [], the whole matrix compiles nothing, and the PR is green. A worktree named after a PR number or a CI workspace with brackets is enough. _rg() escapes the root and leaves the pattern parts alone; all five sites use it. Also anchors LICENSE in _NONCODE_RE. The bare alternative swallowed anything merely starting with it - a future LICENSE_extra.c would have been classified non-code and selected nothing, the same silent-under-selection direction. LICENSES/ is named explicitly because that directory really exists: anchoring alone sent LICENSES/MIT.txt to rule 17, which TestNoTrackedFileIsUnclassified caught immediately. espressif no longer triggers ci_set_matrix's all-miss fall-open. Its examples need the ESP-IDF environment (CLAUDE.md: `. "$IDF_PATH/export.sh"`), which the cmake legs do not have - that is why it is commented out of family_list. Coverage comes from hil-build-esp, which builds those boards by name in an IDF container: verified an espressif-only PR gets 2 esp-idf legs, so the fall-open was adding 74 cmake legs none of which can compile espressif. The other six unbuilt families (cxd56, efm32, f1c100s, pic32mz, py32f0, same7x) keep the net.
Added as an escape hatch for a silently under-selecting selector, but it cannot fire: `on: pull_request` declares no `types:`, so it defaults to [opened, synchronize, reopened] - adding a label starts no run, and a manual re-run replays the original payload without it. It also reached neither hil-hfp-iar (its own selection step, no needs: set-matrix) nor CircleCI (no label access at all), so even when the workflow did run the promise in the comment was false for two of the three consumers. Nothing else referenced it - no labeler.yml entry, no doc, no test, no spec row. A broken escape hatch is worse than none; reverting a bad selector commit gets the same outcome.
Neither token can match. hw/bsp/kinetis_kl sets FAMILY_MCUS KINETIS_KL and compiles OPT_MCU_KINETIS_KL; hw/bsp/samd5x_e5x sets FAMILY_MCUS SAMD51 SAME54 and compiles OPT_MCU_SAMD51. MKL25ZXX and SAME5X are still valid OPT_MCU_ names in src/tusb_option.h, which is why they grep clean and read as live - what changed is the BSP side (the kinetis rework, 1fc203b). They gate nothing today: every one of the 13 examples carrying them is already built on the very board the line meant to exclude, verified per example with skip_example(). So removal is a no-op for what CI compiles, confirmed by building frdm_kl25z x {msc_dual_lun, cdc_msc_freertos, audio_test_freertos} and metro_m4_express x {audio_test, cdc_uac2, audio_test_multi_rate, uac2_headset, audio_4_channel_mic, uac2_speaker_fb} - all green. Dropped rather than re-pointed at the current spellings: re-pointing would remove build coverage that works. If the original intent was runtime rather than compile-time (samd51 iso-IN capture, say), that exclusion belongs in the roster where metro_m4_express already skips device/audio_test_freertos. TestMcuTokensResolve's allowlist shrinks to match, so it stays a list that only ever gets shorter.
get_family_boards' `if preferred_list and examples is None: return [preferred_list[0]]` cannot be reached: with examples None, buildable() is True for every board, so the loop above returns on its first iteration. Verified the picks are unchanged - lpc54 unfiltered lpcxpresso54114, filtered lpcxpresso54608, samd5x metro_m4_express. docs/reference/hil_boards.md is generated from the roster by tools/gen_doc.py and was not regenerated when metro_m4_express's build.args became a variant, so its Variants cell was empty where the generator now emits `metro_m4_express` - exactly as raspberry_pi_pico already shows. Nothing in pre-commit or the workflows checks generated-doc freshness, so the drift would have surfaced as an unexplained hunk in someone else's PR. The spec records the bth ruling: a class no example config enables selects nothing on both axes. Worth stating because the exposure changed - GHA used to rebuild everything for such a PR by accident, through the empty-families bug, so with that fixed both providers now correctly build nothing.
A reader landing in ci_select.py could see `# rule 6` markers but had to open the design spec to learn what rule 6 IS. The table now lives in the docstring as a carbon copy of the spec's - all 23 rows, cell for cell. Both are maintained by hand, so TestRuleTableIsCarbonOfTheSpec pins them: it parses each table and compares row ids and every cell, so editing one without the other fails. Verified by renaming a rule id in the docstring - both assertions fire. A second test pins the table against the CODE: every documented rule id must appear as a `# rule N` marker on a branch. That turned up five rows whose branch was marked by name rather than number (metadata, metrics, the empty-port case, lib, get_deps), so a row could have been documented with no branch, or a branch renumbered, without anything noticing. The markers are numeric now. Unpadded pipes rather than an aligned table: the spec's row 16 path cell alone is 309 characters, and padding five columns to it would make the docstring unreadable. The test compares stripped cells, so the two render differently and stay identical in content.
A reviewer's mutation pass broke five expressions one at a time and the whole
suite stayed green each time. All five silently NARROW the selection, which is the
direction that merges a regression rather than wasting a runner:
build.py defines = () metro_m4_express's MAX3421 firmware, the rig's
only one, stops being built
build.py buildable() -> True lpc54 picks a board that skips the whole -e set,
so the leg compiles nothing
ci_select no _CLS_STEM_RE a midi2 change stops selecting the one example
that compiles it - the e13eff8 fix, unpinned
ci_select drop + ('dual',) a dcd/hcd change stops selecting dual examples
ci_select drop ci=True selector answers differently on a laptop than on
a runner
Each new test was checked against its own mutant: five mutants, one failure each,
zero on the restored tree.
The sixth was vacuous rather than missing. test_class_source_selecting_nothing_
selects_nothing asserted on src/class/vendor/vendor_host.c, which a57f857
deleted - so any made-up path reached the same branch and it could not fail. It
now uses src/class/bth/bth_device.c, asserts the file exists, and checks the
reason names the class.
Four hot spots, each measured before and after. No selection changes: a 260-path differential over every rule shape reports 0 divergences against the previous commit, and the whole run is 1.8x faster (26.0s -> 14.6s). _prune_buildable was 0.63s of 0.64s. It materialised every family's full buildable list to answer two yes/no questions, so it probed 46 examples x every board of the family whatever the diff had selected. Ask skip_example per example instead and both questions short-circuit: 6,883 calls -> 1,889 on one cdc_device.c diff. Its one subtlety is preserved and now pinned by a test. A family whose selection is already everything it can build carries no -e list, because build.py applies the same skip_example the pruner just did and would yield that set anyway. Dropping that check compiled exactly the same firmware and only inflated the payload, which is why 216 tests stayed green through it. reasons are a human diagnostic printed to stderr, and no consumer reads them back - but they rode in the JSON as ~97% of it (453 KB -> 12 KB on a whole-tree diff), through ci_set_matrix, hil_ci_set_matrix, an inline python and three jq calls. The in-process dicts still carry them for the tests. tusb_config.h is re-read once per class path per axis: 4,240 reads of the same 46 files for a diff touching all of src/class. Caching the text (not _config_enables, whose macros argument is a list) takes that to 46, 0.48s -> 0.13s. lib_examples globbed '**/*' per example to keep two filenames - 489 entries to use 107 against a clean tree, and unbounded once `make BOARD=... all` has written _build/, which is where /pre-pr runs.
The upload step passed $EX_ARGS, which never scoped its targets - examples-membrowse-upload is not `all`, so resolve_example_target_groups passes it through as the aggregate - but did move the board, because --one-first picks one that can build the -e set. Dropping it keeps the size history keyed on each family's preferred board whatever a PR touches. The cost, accepted deliberately: where that board is not the one the Build step compiled, there is no elf and every example uploads --identical, so the leg contributes no real data. Eleven families are in that position (imxrt lpc11 lpc18 lpc54 mcx rp2040 rx samd11 stm32l0 stm32l4 tm4c) and the rest are unaffected. The test that pinned the old contract is inverted rather than deleted, and a second one derives the diverging set from build.py and pins it, so the cost stays measured - it already caught an undercount of four.
hathach
force-pushed
the
claude/ci-empty-families
branch
from
August 25, 2026 02:04
6db69b6 to
94fa573
Compare
hathach
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A PR whose build axis legitimately selects nothing was rebuilding everything.
Both merged-since PRs show it:
README.rst,.gitignoretest/hil/**+ docsThe classifier was right both times. From #3842's own
set-matrixlog:The bug
.build.familiesis read twice in the build-extras block: as a|-joined regex for themetrics artifact pattern, and implicitly as "is anything selected". An empty list and
one rejected by the charset guard both leave the regex empty and mean opposite
things — but the branch tested only
-z "$FAMILY_REGEX":MATRIX_JSONwas already right —ci_set_matrix --select-filehad returned the all-emptymatrix. The fall-open branch replaced it with the unscoped one, so every toolchain got its
full family list.
Two things made this hard to see from the outside: the HIL axis never passes through this
block, so it stayed correct (rig jobs skipped on #3842, full rig on #3840 per rule 2); and
no
ci_set_matrix: UNSCOPEDmarker appears anywhere, becauseci_set_matrixbehavedperfectly and the override happens in the shell afterwards. It looked like a
classification failure when it was not.
Introduced in the #3841 follow-up that made
FAMILY_REGEX/EXAMPLE_MAP/BUILD_FILTEREDdrop together — correct for the rejected case, wrong for the empty one.
Branching on which case it is:
build_filteredfamilies: []— nothing selectedfalse''families: [stm32f4, rp2040]truestm32f4|rp2040families: [stm32f4.*]— rejectedfalse''An empty list keeps the all-empty matrix so every leg skips, and sets
build_filtered=falsebecause nothing was built and there is no baseline to compareagainst. A rejected list falls open exactly as before.
FAM_*is renamed toFAMILY_*along the way:
FAMmeanthw/bspboard family, and the abbreviation was ambiguous in afile that also says "family" for the per-family example map and for
get_deps' tokens.This block had no test at all, which is how the bug shipped through two merges and a
max-effort review.
test_ci_metrics.pynow extracts it frombuild.ymland executes itfor real against all three shapes. Verified against master it fails with
129 != 0on theempty case and
'' != 'stm32f4|rp2040'on the scoped one. Writing it surfaced two trapsworth recording: the block prints
::warning::to stdout (so the probe needs asentinel), and
|cannot be the field separator because the regex contains one.Also in here
Follow-ups to the same machinery, each a self-contained commit.
ci_select: fail-open on an odd checkout path. Fiveglob.globsites interpolatedthe repo root raw, so a checkout under a path containing
[,*or?matched nothingand the selector failed closed — silently selecting less rather than more. An
_rg()helper applies
glob.escapeto the root at all five.The
ci-fulllabel is removed. It never worked: the label is read in a job that hasalready computed the matrix from the selection, so setting it changed nothing. Rather than
repair an escape hatch nobody has needed, drop it — a PR that wants the full matrix can
touch any rule-17 path, and the selector falls open on every exception already.
Dead skip tokens. 13
mcu:MKL25ZXX/mcu:SAME5Xtokens across 12skip.txtfilesname MCUs no board reports, so they excluded nothing. They read as deliberate coverage
gaps and are not.
The rule table now lives in the module docstring as a carbon copy of the spec's 23
rows, guarded by a test that fails when the two drift. Contributors editing
ci_select.pysee the binding table without leaving the file.
Six selection behaviours had no test — a mutation pass broke each one and the whole
suite stayed green. Every one is a small expression whose removal silently narrows the
selection, which is the direction that merges a regression rather than wasting a runner.
Performance
Four hot spots, measured before and after. A 260-path differential over every rule shape
reports 0 divergences, and the whole run is 1.8× faster (26.0s → 14.6s).
_prune_buildableasksskip_exampleper example, short-circuitingreasonsout of the JSON — stderr only, no consumer reads them backtusb_config.htext cached (diff touching all ofsrc/class)lib_examplesglobs the two filenames, not**/*_prune_buildablealone was 0.63s of 0.64s: it materialised every family's full buildablelist to answer two yes/no questions, probing 46 examples × every board whatever the diff
had selected.
Its one subtlety is preserved and now pinned. A family whose selection is already
everything it can build carries no
-elist, becausebuild.pyapplies the sameskip_examplethe pruner just did. Dropping that check compiled identical firmware andonly inflated the payload — which is exactly why 216 tests stayed green through it while I
had it wrong.
Membrowse upload is no longer scoped by the PR filter
The upload step passed
$EX_ARGS, which never scoped its targets —examples-membrowse-uploadis notall, soresolve_example_target_groupspasses itthrough as the aggregate — but it did move the board, because
--one-firstpicks onethat can build the
-eset.Dropping it keeps the size history keyed on each family's preferred board whatever a PR
touches. The accepted cost: where that board is not the one the Build step compiled, there
is no elf and every example uploads
--identical, so the leg contributes no real data.Eleven families are in that position (
imxrt lpc11 lpc18 lpc54 mcx rp2040 rx samd11 stm32l0 stm32l4 tm4c); the rest are unaffected. A test derives that set frombuild.pyand pins it, so the cost stays measured — it already caught an undercount of four.
Validation
pre-commit run --all-filesgreen. Suites:test_ci_select218,test_ci_metrics26,test_hil_bounded118. End-to-end, both PR shapes above now yieldlegs=0; adcd_dwc2.cdiff yields 22 families / 8 boards / 37 legs.