Build and test the SILK NEON kernel in CI - #255
Closed
hashirmuzaffar wants to merge 1 commit into
Closed
hashirmuzaffar wants to merge 1 commit into
hashirmuzaffar wants to merge 1 commit into
Conversation
The NEON FIR kernel is guarded by arm64 && go1.27 && !purego, because the assembler only learned those instructions in Go 1.27. The shared matrix in test.yaml tracks the supported release list, which is behind that, so nothing in CI assembles the file: the macOS runner is arm64 but on an older Go, and the jobs on a newer Go are amd64. The parity test added in pion#243 therefore never runs. This adds a repo-local job on an arm64 runner with Go 1.27. It is kept out of test.yaml because that file is synced from pion/.goassets, so a change there would be overwritten, and raising the shared matrix would move every pion repository onto a newer Go for one kernel in this one. The job first asserts that iir_fir_arm64.s is genuinely in the build for this configuration. Without that check a build tag which stopped matching would leave the job green while compiling nothing, which is the failure this issue is about. It then runs the parity test, and runs the package again under purego so the generic path stays covered. Fixes pion#240
Member
|
We manage the ci in one repo in goassets, soon we'll change our test matrix to include 1.27 in the next ci update |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #240.
The parity test from #243 exists, but nothing in CI ever assembles the kernel it covers. The build tag is
arm64 && go1.27 && !purego; the matrix intest.yamlis on the supported release list, sotest-macosis arm64 on an older Go and the newer-Go jobs are amd64. The file is never built and the test never runs.I did not change
test.yaml. It carries aDO NOT EDITheader and is synced frompion/.goassets, so an edit here would be reverted, and raising the shared matrix would move every pion repository onto a newer Go to cover one kernel in this one.conformance.yml,encoder-quality.ymlandplc-quality.ymlare already repo-local, so this follows that pattern.The job does three things:
go listoutput foriir_fir_arm64.sand fails with an error annotation if it is absent.-race.-tags purego, so the generic path stays covered on the same runner.Verified locally on arm64 with Go 1.27:
The guard behaves as intended under
purego: the kernel is correctly absent, which is what would fail the job if the tag ever stopped matching in the NEON configuration.If you would rather take the matrix change discussed in the issue thread,
1.24/1.27ontest-macos, that belongs in.goassetsand I am happy to send it there instead.