Add CI check that no submodule pin regresses relative to main#3331
Add CI check that no submodule pin regresses relative to main#3331jgmelber wants to merge 3 commits into
Conversation
jgmelber
requested review from
abisca,
andrej,
erwei-xilinx,
fifield,
hunhoffe,
jackl-xilinx and
stephenneuendorffer
as code owners
There was a problem hiding this comment.
Pull request overview
Adds a CI guard to prevent third_party/aie_api (which should track Xilinx/aie_api) from silently regressing to a stale/divergent submodule commit during merges/rebases.
Changes:
- Introduces
utils/check_aie_api_submodule.pyto verify the pinned submodule commit is an ancestor of upstream’s default branch. - Adds a new
aie-api-submodulejob to the existing “Lint and Format” workflow to run that check in CI.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
utils/check_aie_api_submodule.py |
New Python check that fetches upstream refs and validates the submodule pin is on upstream history. |
.github/workflows/lintAndFormat.yml |
Adds a CI job to run the new submodule-pin reachability check. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
jgmelber
changed the title
|
Addressed the two Copilot review comments (both resolved) — see inline replies for details:
Also fixed the The one remaining red check, |
third_party/aie_api tracks stock upstream Xilinx/aie_api directly (no fork, see Xilinx#3262), after the local ADF-header patch was replaced by a compile-time define. The pin has since been silently reverted to a stale, divergent pre-Xilinx#3262 commit twice by a merge/rebase from a branch based before the fix landed (Xilinx#3292, Xilinx#3330's regression from Xilinx#3296). Add a CI job that fetches the pin and upstream's default branch tip directly from the submodule's own remote and fails if the pin is not an ancestor of upstream main, catching a third recurrence at PR time instead of after merge. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Replace the aie_api-specific "must be an ancestor of upstream main" check with a generic ratchet: for every submodule in .gitmodules, HEAD's pin must be unchanged or a descendant of origin/main's pin. This needs no per-submodule knowledge of which branch/tag it tracks (cmake/modulesXilinx, platforms/boards, third_party/aie-rt, and third_party/bootgen each track a different one), so it generalizes cleanly across all five submodules instead of just the one that has regressed twice so far. A submodule whose .gitmodules url changes in the same diff is skipped, since a deliberate remote switch (see Xilinx#3262) can legitimately be a non-fast-forward change, and the url edit is itself a visible, reviewed part of the diff. Verified locally against a synthetic submodule+superproject: forward bumps pass, url switches are skipped, and a reverted pin is caught with exit 1. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Two issues Copilot flagged on the earlier aie_api-only version of this check still applied to the generalized version: - A submodule whose .gitmodules url changed was unconditionally skipped, meaning a bad merge that regresses both the pin and the url together (reverting back to an old fork, say) would silently pass. Now the ancestry check runs against the *new* url instead of being skipped; it only passes if the new pin is provably a descendant of the old one there. A genuine non-fast-forward remote switch still fails and must be landed explicitly (e.g. an admin merge), rather than passing silently just because "the url changed". - git fetch failures (e.g. a pin that's unreachable from any ref on the remote) surfaced as an unhandled CalledProcessError traceback instead of a clear ::error:: message. Fetches now go through a non-raising helper and a failed fetch is reported as "not reachable at all". Also pass --filter=blob:none --no-tags on the ancestry-check fetches, since they only need commit/tree objects. Verified against three new synthetic sandbox cases: url switch to a provably unrelated remote now fails instead of silently passing, and a genuine fast-forward across a url switch (the Xilinx#3262 pattern) still passes with a note. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
jgmelber
force-pushed
the
ci/check-aie-api-submodule-pin
branch
from
9c5695f to
48583c6
Compare
andrej
left a comment
andrej
left a comment
There was a problem hiding this comment.
Good addition. The only question I have is if there is a way to circumvent this if we for some reason ever realize we need to roll back a submodule? I guess we could just accept the red X once and bypass the check which would be fine since that seems like a rare occurence.
That's one of my hesitations for including this...is it really all that necessary or helpful? |
Summary
Stacked on top of #3330 — this branch is based on
fix/aie-api-submodule-pointer, so its CI runs against the corrected pin. It'll show as behindmainuntil #3330 merges; merge order: #3330 first, then this.third_party/aie_api's pin has been silently reverted to a stale, divergent pre-#3262 commit twice by a merge/rebase from a branch based before a prior fix landed — first fixed by #3292, then it regressed again via #3296 (fixed by #3330). Nothing stops this from happening to any submodule, not just aie_api.This adds a
Lint and Formatjob,submodule-regressions, generalized across every submodule in.gitmodules(cmake/modulesXilinx,platforms/boards,third_party/aie-rt,third_party/bootgen,third_party/aie_api):origin/main's pinned commit.origin/main's pin — i.e. the change must be a fast-forward. This needs no knowledge of which branch/tag each submodule is meant to track (they each track a different one), so it's fully generic..gitmodulesurlalso changed in the same diff is skipped — a deliberate remote switch (like [aie_api] Drop jgmelber fork; track stock Xilinx/aie_api #3262's de-fork) can legitimately be non-fast-forward, and the url edit itself is a visible, reviewed part of the diff.::error::pointing at Recover accidentally dropped submodule update: [aie_api] Drop jgmelber fork; track stock Xilinx/aie_api (#3262) #3292/Fix aie_api submodule pointer accidentally reverted by #3296 #3330 as the precedent.Test plan
utils/check_submodule_regressions.pyagainst the live repo (before Fix aie_api submodule pointer accidentally reverted by #3296 #3330 merges): correctly flagsthird_party/aie_api's still-live regression, correctly reports the other four submodules unchanged.gitmodulesurl changed alongside a non-ff pin change → skippedlintAndFormat.ymlparses as YAML🤖 Generated with Claude Code