ci(release): stop running the nightly submodule resolver on every trigger, and name the build legs again - #1458
Conversation
`nightly-submodules` ran on every trigger, including PRs and release tags, purely so that its dependents' `needs:` would not short-circuit. On a pinned build it checks out the repo, hits its own guard, writes latest=false and exits, so every ordinary run queued a runner and made four build jobs wait on a job that had nothing to do. Gate the job on the same condition its guard step already uses, and let the dependents survive a skipped dependency with `!cancelled()`. Their conditions and `name:` expressions already treat empty outputs as pinned mode, so a skipped resolver reads exactly like the latest=false it used to write. Nightly and `submodules=latest` behaviour is unchanged. A failed resolver in latest mode still cannot pull the build jobs in: their remaining condition requires latest == 'true' && moved == 'true', which empty outputs never satisfy.
Since #1448 gave build-qt and build-tauri an explicit `name:` for the `[latest submodules]` suffix, GitHub stopped appending the matrix values it adds only to unnamed jobs. Every leg rendered identically: four "Build Qt artifacts" and six "Build Tauri artifacts", so a red run did not say which platform failed without opening jobs one by one. Name the OS explicitly, plus the research flag, reusing the idiom the artifact upload steps already use. os and research fully disambiguate: python_version, node_version and the skip flags are single-valued in both matrices.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if: >- | ||
| github.event_name == 'push' || github.event_name == 'pull_request' || github.event_name == 'workflow_dispatch' | ||
| || (needs.nightly-submodules.outputs.latest == 'true' && needs.nightly-submodules.outputs.moved == 'true') | ||
| !cancelled() |
There was a problem hiding this comment.
When a manual run requests submodules=latest and the resolver fails, !cancelled() remains true and the workflow_dispatch branch still allows the dependent jobs to run. Because the failed resolver leaves latest empty, those jobs skip the latest-SHA checkout and build the pinned tree instead. This wastes the full build matrix and presents pinned-tree results for a run intended to validate the latest submodule tips.
There was a problem hiding this comment.
Correct, and it contradicts a claim I made in the PR description. I wrote that a failed resolver still could not pull the build jobs in because their remaining condition needs latest == 'true' && moved == 'true'. That reasoning only holds for the schedule path: on a workflow_dispatch run the github.event_name == 'workflow_dispatch' disjunct short-circuits before the latest/moved check ever gets evaluated, so a failed resolver would have let the whole matrix build the pinned tree and report success for a run whose only purpose was to validate the latest tips.
Fixed in 5414cff by gating the dependents on the resolver's result as well:
&& contains(fromJSON('["success", "skipped"]'), needs.nightly-submodules.result)A skipped resolver still passes through, which is what this PR is for; a failed or cancelled one blocks the dependents exactly as it did before the PR. Applied to all four dependents.
There was a problem hiding this comment.
You're right, and the distinction is important: on workflow_dispatch, the event-name disjunct bypassed the resolver-result check, so a failed latest-submodule resolution could have triggered a pinned-tree build. The additional needs.nightly-submodules.result gate fixes that while preserving the intended behavior: skipped proceeds for ordinary pinned runs, whereas failure and cancelled block all four dependents. No further concern with this fix.
|
|
||
| build-qt: | ||
| name: Build Qt artifacts${{ needs.nightly-submodules.outputs.latest == 'true' && ' [latest submodules]' || '' }} | ||
| name: Build Qt artifacts (${{ matrix.os }}${{ matrix.research && ', research' || '' }})${{ needs.nightly-submodules.outputs.latest == 'true' && ' [latest submodules]' || '' }} |
There was a problem hiding this comment.
The new name uses only matrix.research, but manually dispatched research builds and *-research tag builds enable Research Edition through AW_RESEARCH_EDITION while leaving that matrix value false. Those jobs are therefore labeled as ordinary builds, making it harder to distinguish edition-specific release artifacts. The label should use the effective edition expression rather than only the matrix value.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Agreed, fixed in 5414cff. The label now mirrors each job's own AW_RESEARCH_EDITION expression rather than the matrix value alone, so *-research tag pushes and edition=research dispatches are labelled as research builds:
name: Build Qt artifacts (${{ matrix.os }}${{ (matrix.research || (github.event_name == 'workflow_dispatch' && inputs.edition == 'research') || endsWith(github.ref_name, '-research')) && ', research' || '' }})...Also applied to the manylinux_2_28 job, which you did not flag but which builds the research edition on the same two triggers (its AW_RESEARCH_EDITION omits matrix.research since it has no research matrix leg, and the label omits it to match).
The expression is duplicated rather than hoisted because jobs.<id>.name cannot read the env context, and computing it in an upstream job would reintroduce exactly the always-running prerequisite this PR removes.
🤖 AI code reviewThe release workflow now limits the nightly-submodules job to the nightly cron (0 3 * * *) or workflow_dispatch with submodules=latest, and updates the four dependent jobs to run when the resolver is skipped. The build-qt and build-tauri job names now include matrix.os and research-edition state, and build-qt-manylinux-2-28 includes research-edition state. Safe to merge — no P0/P1 findingsConfidence 5/5
✅ No findings. The diff looks correct to me on this pass. Files changed (1) — the diff as I read it
Previous review passes
Reviewed Maintainer commands
|
… edition Greptile P1: with only !cancelled(), a workflow_dispatch run with submodules=latest whose resolver FAILED would still satisfy the workflow_dispatch disjunct, so the full matrix would build the pinned tree and report success for a run whose whole point was to validate the latest tips. The PR description claimed otherwise; it was wrong, because that disjunct short-circuits before the latest/moved check. Gate the dependents on the resolver's result being success or skipped, which restores the old failure semantics while keeping the new skip behaviour. Greptile P2: research tag pushes and edition=research dispatches enable Research Edition through AW_RESEARCH_EDITION while matrix.research stays false, so those legs were labelled as ordinary builds. Label the effective edition, mirroring each job's own AW_RESEARCH_EDITION expression. Applied to the manylinux job too, which Greptile did not flag but which builds the research edition on the same triggers.
|
All review threads are conversationally closed (fixes in 5414cff, confirmed by the reviewer). I attempted to mark the two Greptile threads resolved but lack permission to do so as a non-author — leaving them for you. CI is fully green, including the per-platform build-leg names and the skipped resolver on non-nightly triggers. Ready for your merge decision. |
2 participants
Two pieces of fallout from #1448, both visible in the v0.14.0b8 release run.
1.
nightly-submodulesran on every triggerThe job ran on PRs, branch pushes and release tags alike, purely so its dependents'
needs:would not short-circuit. On a pinned build it checks out the repo, hits its own guard, writeslatest=falseand exits. So every ordinary run queued a runner to do nothing, and four build jobs (build-qt,build-qt-manylinux,build-tauri,research-edition-checks) waited on it before starting. It also put a job called "Resolve latest submodule tips (nightly)" in the graph of every release tag, which reads as if something nightly is happening during a release.Now the job carries the same condition its guard step already evaluates:
and the four dependents gained
!cancelled() && (...)so a skipped dependency does not cascade. Their conditions andname:expressions already treat empty outputs as pinned mode, so a skipped resolver reads exactly like thelatest=falseit used to write.Behaviour that does not change:
submodules=latestdispatches are untouched.0 12 * * 4) still flows through preflight/create-tag as before; it never depended on the resolver's outputs.latest == 'true' && moved == 'true', which empty outputs never satisfy.2. All the matrix legs had the same name
Giving
build-qtandbuild-taurian explicitname:for the[latest submodules]suffix stopped GitHub appending the matrix values it adds only to unnamed jobs. Every leg rendered identically: four "Build Qt artifacts" and six "Build Tauri artifacts". A red run did not say whether Windows, Intel macOS or arm Linux failed without opening jobs one by one.The names now carry the OS and the research flag, reusing the idiom the artifact-upload steps already use.
osandresearchfully disambiguate, sincepython_version,node_versionand the skip flags are single-valued in both matrices.Happy to split this into two PRs if preferred; they are separate commits and touch the same two
name:lines, so reviewing them together seemed kinder than rebasing one on the other.Testing
actionlintis clean apart from seven pre-existing shellcheck style warnings, identical in count and location to master. The real proof is the run this PR triggers:nightly-submodulesshould be skipped, the build jobs should start immediately, and the legs should be named per platform.