Repository navigation
fix(ci): skip the libraries baseline gate instead of failing it - #8161
Conversation
✅ All Checks Passed — Ready for Review
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
|
🎉 All checks passed! This PR is ready for review. |
The `libraries_baseline_ready` job added in #8069 exists only so bump_automation.py can tell whether a run's reused stages are good enough to hand out as a `baseline_run_id`. #8069 expected it to be inert -- "nothing else in Multi-Arch CI depends on it, so a bug in it ... cannot block or skip any other job in the run" -- but that does not hold. The job lives inside a reusable workflow, so failing it fails the calling `build_multi_arch_stages` job, and the test matrix is guarded by `if: !failure()` and never starts. That hits any caller that narrows the build graph with `build_stages`: the stages it deliberately excluded come back as `skipped`, the gate fails on them, and the run goes red with no tests. Observed on the rocm-systems host-asan presubmit, where both requested stages built cleanly and every test was skipped anyway: https://github.com/ROCm/rocm-systems/actions/runs/34608811109 Dropping `if: always()` and the failing step restores the default `needs:` semantics, which already express the intent: the job runs only when every reused stage succeeded, and is skipped otherwise. The bump automation keys on `conclusion == "success"`, so a skipped gate rejects the baseline exactly as a failed one did -- without turning the run red. Co-authored-by: Cursor <cursoragent@cursor.com>
fb6c70d to
1b3f25f
Compare
tony-davis
left a comment
There was a problem hiding this comment.
Hit the same bug from the other direction today: ROCm/rocm-libraries#11952 (a bump-bot PR using prebuilt_stages/baseline_run_id reuse) went red on both the Linux and Windows Libraries Baseline Ready jobs for exactly this reason, on the same commit that introduced the gate (#8069). Good catch that it also breaks build_stages-narrowed runs like the host-asan presubmit; I hadn't considered that case.
I like this fix better than the per-stage contains(inputs.prebuilt_stages, ...) check I was about to write. Letting the default needs: semantics do the work is simpler and doesn't need updating every time the reused-stage list changes.
Checked out the branch and ran it locally:
pytest build_tools/github_actions/tests/bump_automation_test.py -q→ 63 passedpre-commit run --files .github/workflows/multi_arch_build_portable_linux.yml .github/workflows/multi_arch_build_windows.yml build_tools/github_actions/tests/bump_automation_test.py→ all green, including actionlint
LGTM, approving.
Motivation
GitHub issue: #7202
The
libraries_baseline_readyjob added in #8069 exists for one purpose: to letbump_automation.pydecide whether a completed run's reused stages are good enough to hand out as abaseline_run_id. #8069 assessed it as inert:That does not hold in practice. The job lives inside the reusable workflows
multi_arch_build_portable_linux.yml/multi_arch_build_windows.yml, so when it fails, the callingbuild_multi_arch_stagesjob is marked failed.test_artifacts_per_familyis guarded byif: ${{ !failure() && !cancelled() }}and therefore never starts — the test matrix does not even expand.This bites any caller that narrows the build graph with
build_stages. The stages it deliberately excluded reportskipped, the gate treatsskippedas not-success and fails, and the run goes red having run no tests. Observed on the rocm-systems host-asan presubmit in https://github.com/ROCm/rocm-systems/actions/runs/34608811109, where both requested stages built cleanly:compiler-runtime / Stage - Compiler Runtimeruntime-tests / Stage - Runtime Testsbuild_stages)Libraries Baseline ReadyTest ${{ matrix.family_info.amdgpu_family }}Worth noting the gate fired in a context nothing will ever read:
get_baseline_run_id_from_merged_pr()only scans TheRock's ownMulti-Arch CIruns after a merge, never an external repo's run.Technical Details
Dropped
if: always()and the conditional failing step. GitHub's defaultneeds:semantics already express exactly the intent — a job whoseneeds:did not all succeed is skipped — so the gate now runs only when every reused stage succeeded, and is skipped otherwise.The rejection signal is unchanged from the bump automation's point of view.
_baseline_gate_jobs_succeeded()keys onconclusion == "success", and skipped jobs are still returned by the Actions jobs API withconclusion: "skipped", so a skipped gate rejects the baseline exactly as a failed one did. All three cases still land correctly:The remaining step now just echoes
toJSON(needs)for diagnosis when the gate does run.Test Plan
python -m pytest build_tools/github_actions/tests/bump_automation_test.py -qpre-commit run --files .github/workflows/multi_arch_build_portable_linux.yml .github/workflows/multi_arch_build_windows.yml build_tools/github_actions/tests/bump_automation_test.pyTest Result
63 passed. Targeted pre-commit passed, including
actionlinton both workflow files.Risk Assessment
Risk 2. CI wiring on the libraries/systems bump path. The change strictly reduces what the gate can break: it can no longer fail, so it can no longer propagate failure to a calling job. The baseline-rejection behavior the gate was added for is preserved, and is now covered by a test asserting a
skippedgate job is rejected.Related
build_stagesnarrowingDevice / Architecture Coverage
No device code. CI workflow and unit-test change only.
Testing Summary
Added
test_false_when_the_gate_job_was_skipped, covering the path that is now the primary rejection route: a gate job skipped because a reused stage did not succeed must not be handed out as a baseline. Existing coverage for succeeded / failed / missing / multi-platform gate jobs is unchanged and still passes.Testing Checklist
python -m pytest build_tools/github_actions/tests/bump_automation_test.py -q- Status: Passed (63)Flags / Guardrails
None.
Adjacent Tests Considered
GetBaselineRunIdFromMergedPrTestexercises the lookup end to end against mocked job lists and needed no change, since it already drives_baseline_gate_jobs_succeededthrough non-success conclusions.Risk Acceptance / Waivers
None.
Technical Changes
if: always()and the conditionalexit 1step fromlibraries_baseline_readyin both the Linux and Windows multi-arch build workflows, so the job is skipped rather than failed when a reused stage did not succeed.Submission Checklist