Repository navigation
ci: run the macOS write watch and register context tests under Rosetta - #3962
Conversation
|
Looks good. The three new jobs ( AI-assisted: yes. Written with AI assistance (Claude Code); I reviewed it. |
|
Tested on macOS 27.0.1 (Apple silicon, Rosetta 2), AppleClang 21.0.0, CMake 4.4.4, with this PR merged into Same commands as the three new jobs (default generator,
To look for Rosetta-only flakiness, each build also ran This adds a macOS 27 result to the macOS 26.0.1 runs in the description and the AI-assisted: yes (Claude Code) |
…ite watch and register context tests under Rosetta (@ahir061) Taken from the open upstream pull request so the macOS integration branch carries every piece of the port that is already under review, instead of re-implementing it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
fb15383 to
1c3116c
Compare
…context tests under Rosetta (@ahir061) This branch already carried an earlier head of this PR; the author has pushed newer commits since, and this merge brings the branch up to them. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
MarcZX10
left a comment
There was a problem hiding this comment.
The Rosetta matrix changes are supported by the passing macOS x86_64 jobs and repeated author-reported runs. I manually compared the shared workflow hunks with #6044: its current changes relocate path-filter entries around lines 70–85, while this PR adds the matrix entries and commands near lines 358–429, so I found no same-hunk dependency.
I’m requesting one contribution-template correction before approval. The current .github/pull_request_template.md includes Fixes:, Unblocks:, and GPUs tested: checklist lines, but this PR body omits all three. Please complete them (none, none, and n/a, respectively) so the description follows the repository’s current template. The conventions workflow passed, but it did not flag these missing fields.
|
Small, verifiable change: two macOS x86_64 matrix rows added with One suggestion before it merges: both new matrix entries still run inside jobs with Envelope: 4,631 commits behind base |
Tested on Apple M1 Pro, macOS 27.0.1 (native arm64, and x86_64 under Rosetta 2)AppleClang 21.0.0, CMake 4.4.4. Head
All runs used Two notes on the PR text, neither of which blocks it:
Not checked here: the AI-assisted: yes (Claude Code applied the PR, ran the tests and wrote this report). |
THEMANnt
left a comment
There was a problem hiding this comment.
Reviewed 5c3e283416dcec80839058b85dfedf5904397165 against author merge base 4866297fcfaa56b53d0ff5b21e4909bed1d89ffd.
Approve. No actionable findings in the CI matrix patch.
Reviewed the complete workflow changes: existing native matrix combinations remain, while macos-15 x86_64 Debug/Release register-context and x86_64 write-watch combinations are added. Each new combination installs Rosetta, configures the x86_64 architecture explicitly and receives the longer 15-minute timeout. Empty architecture values retain the existing native configuration path.
Validation: parsed the exact YAML and inspected matrix entries. Hosted Build 40095103764 successfully executed all three added jobs: write-watch 120875607837, register-context Debug 120875607845 and Release 120875607865; their logs show actual passing CTest runs on the PR merge checkout. Conventions and whitespace passed. This older registration CLI rejects --scripts, so that check is not reported as passing. No local macOS execution or full application/game test was performed.
|
Windows run of this head
AI-assisted: yes (Claude Code) |
|
Static review plus local checks on the changed test project (CachyOS Linux, GCC 16.2.1, Ninja, Release). No findings. What I checked
Local smoke check (this is the changed build path' template — same CMake project, not the Rosetta invocation itself): 3/3 passed (0.01s total), Not covered The Rosetta-instruction path ( |
|
Correction: The previous review comment (Oct 24) suggested raising the timeout for the new Rosetta matrix rows. Commit Verdict: should-fix Findings:
Freshness envelope:
Cross-PR ledger:
|
MarcZX10
left a comment
There was a problem hiding this comment.
Re-checked — changes requested
The Rosetta matrix has passing hosted runs, but the PR description still misses required checklist fields and the branch is far behind current main.
What I ran
Rechecked exact PR head 5c3e283416dcec80839058b85dfedf5904397165, base 3fdd1f52e968c57d2b6890b11863f96aee0c11ce, and current upstream main 9624d8c6bb9e96b7aa92b815f55690f69b0a88d5. The current-main merge preview is clean. Environment: Linux x86-64, Git and GitHub CLI; I did not build locally.
git diff --check 9624d8c6bb9e96b7aa92b815f55690f69b0a88d5...refs/review/pr-3962-head
git merge-tree --write-tree 9624d8c6bb9e96b7aa92b815f55690f69b0a88d5 refs/review/pr-3962-head
git rev-list --count 3fdd1f52e968c57d2b6890b11863f96aee0c11ce..9624d8c6bb9e96b7aa92b815f55690f69b0a88d5Results
| Check | Result |
|---|---|
| Patch whitespace | Pass |
Merge preview on current main |
Clean |
| Base freshness | Current main is 10,781 commits ahead of the PR base |
| Hosted Rosetta register-context Debug/Release and write-watch x86_64 jobs | Pass on this exact head; the existing matrix jobs also pass |
| Hosted conventions, eligibility and labels | Pass on this exact head |
| Hosted Linux and Windows product jobs | Skipped |
| Shared workflow overlap with #12741 | The overlap bot could not compare the shared file because #12741 conflicts with its base. I manually compared the hunks: this PR changes the register-context/write-watch matrix around lines 358–429; #12741 changes eligibility and adds edge-watch elsewhere. I found no same-hunk overlap. Recheck after syncing current main. |
| PR template checklist | Fixes, Unblocks, and GPUs tested lines are absent from the description |
| Local build/tests | Not run; the exact-head hosted matrix already exercises the added jobs |
Unedited local command output
diffcheck-exit=0
0c9c8fc05e1a6fdb6d8464fc4f8bc7ad6d5b245b
merge-tree-exit=0
10781
Findings
Blocking
- The PR description omits the required checklist entries
Fixes,Unblocks, andGPUs tested. Please addnone,none, andn/arespectively, as applicable to this CI-only change. - The PR base is
3fdd1f52e968, while currentmainis9624d8c6bb9e. Although the merge preview is clean, the repository requires branches to be based on and kept up to date with currentmain; rebase or merge currentmain, then rerun the overlap/convention checks.
Non-blocking
- The overlap bot could not compare
.github/workflows/build.ymlagainst #12741 because that PR conflicts with its base. My manual hunk comparison found separate workflow sections and no duplicate change; the overlap check should be refreshed after the branch is updated.
Sources
- Branch and checklist requirements at current main
- Pull request template at current main
- Build workflow at current main
Not verified
I did not independently run a macOS/Rosetta host; the successful hosted matrix results and contributor runs are separate from local testing. The Linux and Windows product workflows were skipped.
To get this to a good state
Add the three checklist lines, update the branch onto current main, and rerun the overlap and convention checks. I will recheck the resulting head and the new hosted statuses.
|
Ran this on Apple M1, macOS 27.0.1 (26A434), AppleClang 21.0.0, CMake 4.4.5. Merged locally onto f911371 (merge clean); test binaries verified as x86-64 under Rosetta. Commands follow the workflow, with --repeat until-fail:3. register-context Debug and Release: 3/3 passed in every repeat, on main and with the PR. write-watch (Release, x86_64): configure fails on main and with the PR, with the same error: The diff looks correct as a workflow-only change. One detail: the description says these jobs run on every PR, but they are gated by changed files. AI-assisted (Codex). |
|
@ahir061 Thanks for keeping this going. #13220 has now merged, so the duplicate write-watch registration is gone from this PR's way, and we've queued a fresh CI run on your current head to see the Rosetta cells on today's One small thing that would help it land: the PR template's |
|
Other open pull requests touch the same code. Merging one will break the other, or both implement the same thing. If this one builds on them, list them in Depends on.
Some comparisons could not be completed because a pull request conflicts with the base branch. Review these shared files manually before concluding that there is no overlap.
|
5c3e283 to
69eed07
Compare
What
Relinked titles run on Apple silicon as x86-64 code under Rosetta (
--macos --to-rosetta, USAGE.md), but CI runs the write watch tests there only as arm64 (macos-15) and the register context tests only onmacos-15-inteland Linux. This adds x86_64 builds of both onmacos-15, using the-DCMAKE_OSX_ARCHITECTURES=x86_64configuration BUILD.md documents for local runs, and installs Rosetta first in case the runner image lacks it.New jobs:
register-context (macos-15, Debug, x86_64),register-context (macos-15, Release, x86_64)andwrite-watch (macos-15, x86_64). The existing jobs are unchanged:CMAKE_OSX_ARCHITECTURESis empty for them, which keeps the host architecture.Tested
filereportsMach-O 64-bit executable x86_64), with the same commands as the new jobs and--repeat until-fail:3:tools/check_conventions.py --base mainpasses.macos-15image.Checklist
main; no other open PR implements the same functionsNotImplemented_nid_no_patch); silent stubs are listed in TechnicalDebt.mdfiles or images added to the repository