Skip to content

bump-hrx: treat tests/CMakeLists.txt as an HRX path, so AMD's test rename cannot conflict - #630

Merged
bong-water-water-bong merged 3 commits into
mainfrom
1bit/bump-hrx-tests-cmakelists
Oct 29, 2026
Merged

bong-water-water-bong merged 3 commits into
mainfrom
1bit/bump-hrx-tests-cmakelists

Conversation

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator

Fixes #628.

What was failing

Scheduled run 40692914102, step Sync the fork and merge AMD's pin into our line:

::error::taking AMD's c1e1bcbf6839 changes outside the HRX backend conflicts in: tests/CMakeLists.txt  ; resolve those files by hand on 1bit/hrx-vulkan-patched

Why

config/hrx-system-hold.config names 82804197bcbe, so bump-hrx-sync.sh takes the held path: it builds AMD's pin tree with HRX_PATHS reverted to the merge base and merges that into 1bit/hrx-vulkan-patched. AMD's pin c1e1bcbf6839 renames the four HRX tests (test-hrx-buffer -> hrx-buffer-test, test-hrx-ops -> hrx-ops-test, ...) and rewrites their registration in tests/CMakeLists.txt.

HRX_PATHS covered the test sources with tests/*hrx*, but not tests/CMakeLists.txt, where the targets are registered — so AMD's rename was carried into our line and collided with our own registration (we have ~14 extra test-hrx-* targets AMD does not).

The whole pin delta is HRX backend work:

$ git diff --name-only 43b0ec2c c1e1bcbf
ggml/src/ggml-hrx/...            (11 files)
tests/CMakeLists.txt
tests/hrx-{backend,buffer,loom-jit,ops}-test.cpp
$ git diff --name-only 43b0ec2c c1e1bcbf | grep -vE '^ggml/src/ggml-hrx/|^ggml/include/ggml-hrx\.h$|^tests/[^/]*hrx[^/]*$|^tests/CMakeLists\.txt$'
(none)

so tests/CMakeLists.txt belongs with the HRX test targets it registers.

Before and after, same command

$ DRY_RUN=1 AMD_LLAMA=c1e1bcbf6839... OURS_LLAMA=e7e5cd08f9bb... OURS_HRX=82804197bcbe... scripts/bump-hrx-sync.sh llama

before (origin/main): ::error::taking AMD's c1e1bcbf6839 changes outside the HRX backend conflicts in: tests/CMakeLists.txt, exit 1
after (this branch):

::notice::hrx-system is held (config/hrx-system-hold.config): AMD's c1e1bcbf6839 changes only HRX backend files (16 since 43b0ec2c41cc), which are written against AMD's hrx-system; 1bit/hrx-vulkan-patched stays at 40c4ccb76e48
DRY_RUN: git push fork c1e1bcbf68395a4c436342378938489fb569a8ca:refs/heads/1bit/hrx-vulkan
output: rebased=40c4ccb76e48aafd44e024b0d2bb27b38bf54015

exit 0. The hrx-system half was skipped in the failed run, so I dry-ran it too: scripts/bump-hrx-sync.sh hrx -> held at 82804197bcbe, exit 0.

With this, the nightly run reaches Open the bump PR, finds bump-hrx/40c4ccb76e48-82804197bcbe already exists (that is #610) and exits 0 with a notice. It goes properly quiet once #610 lands.

Second, non-blocking finding

The run also logs, every time:

gh: refusing to allow a Personal Access Token to create or update workflow `.github/workflows/behavior-check.yml` without `workflow` scope (HTTP 422)
##[warning]could not refresh 1bit-MONSTER/llama.cpp master; continuing with the HRX pin sync

HRX_BUMP_TOKEN has lost the Workflows: read/write permission the workflow header asks for, so the fork's master is never refreshed. Not fatal for the pin sync. The second commit carries that reason into the annotation instead of swallowing it — the token itself needs an owner to re-scope it.

Verification

  • bash -n scripts/bump-hrx-sync.sh
  • both dry runs above against the live fork and AMD's live pin
  • the warning path exercised with a stub gh that fails with the real 422 text: prints ::warning::could not refresh 1bit-MONSTER/llama.cpp master: gh: refusing ... (HTTP 422); continuing with the HRX pin sync, exits 0

Nothing else touched; DRY_RUN=1 pushes nothing.

…name cannot conflict

AMD's 2026-10-28 pin c1e1bcbf6839 renames the four HRX tests (test-hrx-buffer ->
hrx-buffer-test, test-hrx-ops -> hrx-ops-test, ...) and rewrites how they are registered in
tests/CMakeLists.txt. HRX_PATHS covers the .cpp files with 'tests/*hrx*', but the registration
block lives in one shared file that was not covered, so while hrx-system is held (the path that
replays only AMD's changes outside the HRX backend) the rename was merged into our line and the
run died on the conflict:

  ::error::taking AMD's c1e1bcbf6839 changes outside the HRX backend conflicts in: tests/CMakeLists.txt

That is the failure in run 40692914102 (engine#628). All 16 files in the pin delta
43b0ec2c..c1e1bcbf are HRX backend files, so with the fix the held path reports "changes only
HRX backend files (16 since 43b0ec2c)" and leaves 1bit/hrx-vulkan-patched at 40c4ccb76e48 while
moving the mirror 1bit/hrx-vulkan to AMD's pin -- exactly what the hold intends.

Also carry the reason into the warning when the fork's master cannot be refreshed: every run logs
"without `workflow` scope (HTTP 422)", and the previous message hid that the fix is a token
permission and not a fork problem. The message contains no secret.
@github-actions

github-actions Bot commented Oct 29, 2026 •

Copy link
Copy Markdown

PR Reviewer Guide 🔍

(Review updated until commit 95ebd5e)

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶

628 - Partially compliant

Compliant requirements:

  • The PR correctly identifies and fixes the merge conflict by including tests/CMakeLists.txt in HRX_PATHS
  • The PR updates the bump-hrx-sync.sh script to include tests/CMakeLists.txt in the HRX paths
  • The PR adds a clear explanation of why this change is necessary

Non-compliant requirements:

  • The PR does not include updated documentation or wiki changes to reflect the new HRX path handling

Requires further human verification:

  • The fix should be verified by running the actual sync workflow to confirm the conflict is resolved
  • The updated tests/CMakeLists.txt should be checked for correctness after the sync

610 - Partially compliant

Compliant requirements:

  • The PR addresses the conflict in tests/CMakeLists.txt which was preventing the merge
  • The PR ensures that the HRX backend paths are correctly handled during the sync

Non-compliant requirements:

  • The PR does not include validation steps or tests for the HRX backend as mentioned in the ticket
  • The PR does not update documentation or README files to reflect the changes

Requires further human verification:

  • The updated HRX backend should be tested on Strix Halo with the long-prompt correctness probe
  • The test-backend-ops -b HRX0 test should be run to validate the changes
⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Missing Documentation Update

The PR fixes a merge conflict in tests/CMakeLists.txt by including it in the HRX paths, but does not update the documentation or README files to reflect this change. According to the instructions, docs must be updated in the same PR as the behavior they describe.

HRX_PATHS=(ggml/src/ggml-hrx ggml/include/ggml-hrx.h 'tests/*hrx*' tests/CMakeLists.txt)
Potential Test Registration Conflict

The PR adds tests/CMakeLists.txt to the HRX paths, which means that when the hold is lifted and a full merge occurs, AMD's test registration block will be merged back in. This could potentially conflict with the existing test registrations if not carefully managed, especially if the test names or registration logic changes in a way that's not compatible.

HRX_PATHS=(ggml/src/ggml-hrx ggml/include/ggml-hrx.h 'tests/*hrx*' tests/CMakeLists.txt)

@github-actions

Copy link
Copy Markdown

Persistent review updated to latest commit 2b8372d

@github-actions

Copy link
Copy Markdown

Persistent review updated to latest commit 95ebd5e

@bong-water-water-bong
bong-water-water-bong merged commit 5b4e7a4 into main Oct 29, 2026
8 checks passed
@bong-water-water-bong
bong-water-water-bong deleted the 1bit/bump-hrx-tests-cmakelists branch October 29, 2026 01:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bump-hrx: the scheduled HRX pin sync is failing

1 participant