Properly fix CI gate for empty build matrices - #186
Merged
Conversation
The previous fix (merged in #185) assumed an empty-matrix build-and-unittest job reports needs.result == "skipped", so the gate only failed on literal "failure". That assumption was wrong: GitHub reports needs.<job>.result as "failure" for a zero-entry matrix job, so the gate kept failing for the exact case it was meant to allow. Two real fixes: 1. confirm-build-and-unittest-complete now reads the computed docker_matrix from setup-environment directly and checks its entry count, instead of trusting build-and-unittest's result. Zero entries = nothing to build for this diff = pass. Non-zero entries still gate on an actual success/failure result. 2. INFRASTRUCTURE_CHANGED in check_src_changes/action.yml was defined with a YAML folded scalar (`>`) spanning two lines, which appends a trailing newline to the evaluated value. The downstream bash comparison `[ "$INFRASTRUCTURE_CHANGED" == 'true' ]` then silently fails even when the expression evaluates true, so the "rebuild everything" sentinel for infra-only changes (e.g. workflow file edits) never actually fired. Collapsed to a single-line expression so the value has no trailing newline. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Mzkt9bALC8oUqQKCN6upNm
Exposed by the infra-sentinel fix in this same PR: isaac_lab_il_datagen is a single-stage Dockerfile built on the already-skipped simulation_isaac GPU image (needs cuRobo/CUDA), so it has no "dependencies"/"build" stages and can't build under the shared multi-stage build-and-unittest job. It was missing from the existing GPU/non-CI skip list alongside its siblings simulation, simulation_isaac, simulation_mj, and embedded -- this had never surfaced because the infra-sentinel bug meant infra-only changes never actually triggered a full-matrix build until now. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Mzkt9bALC8oUqQKCN6upNm
Found by running the first-ever full-matrix build (this PR's own CI,
enabled by the infra-sentinel fix above): perception and
perception.mac each define services literally named
"perception"/"perception_dev", but the concurrency group only keyed
on matrix.service:
group: ${{ matrix.service }}-${{ github.workflow }}-${{ github.ref }}
So both modules' matrix jobs collided on the same group and
cancel-in-progress killed one of each pair -- not a failure, but a
silently cancelled job, which still fails the
confirm-build-and-unittest-complete gate since the aggregate
build-and-unittest result isn't "success". This had never triggered
before because a full TEST_ALL=true matrix build never actually ran
until the infra-sentinel fix in this same PR made one possible.
Added matrix.module to the group key so same-named services in
different modules no longer collide.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Mzkt9bALC8oUqQKCN6upNm
python-version: '3.10.20' pinned an exact patch build via the very old actions/setup-python@v1, and GitHub has since pruned that patch from its hosted-runner Python manifest (only 3.10.21+ available now), so every run of this workflow failed at the "Set up Python" step regardless of the actual diff -- confirmed broken on main itself, independent of any of the other CI fixes in this PR. Bumped to actions/setup-python@v5 with a floating '3.10' version so it always resolves to whatever patch is currently available instead of needing a manual bump each time GitHub prunes an old one. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Mzkt9bALC8oUqQKCN6upNm
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
2 participants
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.
Summary
Follow-up to #185, which shipped a broken fix. Root causes found by reading actual job logs from #185's own (failed) CI run:
build-and-unittestjob reportsneeds.result == "skipped", and only failed the gate on literal"failure". In reality GitHub reportsneeds.<job>.resultas"failure"for a zero-entry matrix job — so the gate kept failing for the exact case it was supposed to let through.INFRASTRUCTURE_CHANGEDincheck_src_changes/action.ymlwas defined with a YAML folded scalar (>) spanning two lines, which appends a trailing newline to the evaluated value ("true\n"instead of"true"). The downstream bash check[ "$INFRASTRUCTURE_CHANGED" == 'true' ]then silently fails even when the expression is true, so the "rebuild everything" sentinel for infrastructure-only changes (e.g. editing workflow files) never actually fired.Fix
confirm-build-and-unittest-completenow readssetup-environment's computeddocker_matrixoutput directly and checks its entry count (jq '.include | length') instead of trustingbuild-and-unittest'sneeds.result. Zero entries → nothing to build for this diff → pass. Non-zero entries still gate on an actualsuccess/failureresult.INFRASTRUCTURE_CHANGEDfolded-scalar expression to a single line so the evaluated value has no trailing newline.Test plan
$'true\n' != 'true'in bash) and confirmed the single-line expression fixes it.Generated by Claude Code