fix(mex): unblock multi-platform auto-PR pipeline (2 bugs)#93
Merged
Conversation
Bug 1: aggregator never opens the auto-PR refresh-mex-binaries.yml's `actions/upload-artifact` trims the LCA of the supplied paths to `libs/`, so each artifact's internal root is `FastSense/...` not `libs/FastSense/...`. With `merge-multiple: true` and no `path:` set, files extract under workspace root — outside `libs/`. The aggregator's `find libs` step finds nothing new, `peter-evans/create-pull-request` sees no diff, exits silently. The auto-PR has never actually opened — PR #65's binaries were committed by hand. Fix: add `path: libs` to the download-artifact step so the trimmed-LCA contents reassemble under libs/. Bug 2: Windows MATLAB silently emits no .mexw64 needs_build()'s probe 2 (bare `binary_search_mex.mex`) was meant as an Octave fallback but on MATLAB it falsely matches stale bare-.mex files left over from before the octave-<tag>/ subdir convention. Probe 2 trips → core_ok=true → stamp matches → needs_build returns false → first_run skipped → no current-platform binary produced → upload-artifact errors with "no files found". Linux/macOS MATLAB happened to dodge this in CI run 25051632929 by stamp-drift luck; Windows MATLAB hit it cleanly. Fix: guard probe 2 with an isOctave check so the bare-.mex fallback only fires when the current runtime is Octave. Together these two fixes make the multi-platform refresh truly automatic: trigger refresh-mex-binaries.yml, all 7 platforms compile, the aggregator opens an auto-PR with the full binary set, merging that PR makes the next release archive ship multi-platform binaries without manual intervention. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark 'FastSense Performance'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.10.
| Benchmark suite | Current: ae8bdf7 | Previous: 5c45199 | Ratio |
|---|---|---|---|
Downsample mean (1M) |
1.753 ms |
1.178 ms |
1.49 |
Downsample mean std(1M) |
0.013 ms |
0.009 ms |
1.44 |
Render mean std(1M) |
1.251 ms |
0.697 ms |
1.79 |
Downsample mean (5M) |
10.635 ms |
7.417 ms |
1.43 |
Downsample mean std(5M) |
0.299 ms |
0.029 ms |
10.31 |
Instantiation mean std(5M) |
1.857 ms |
0.73 ms |
2.54 |
Render mean std(5M) |
2.54 ms |
1.228 ms |
2.07 |
Downsample mean (10M) |
23.812 ms |
14.754 ms |
1.61 |
Downsample mean std10M) |
0.293 ms |
0.112 ms |
2.62 |
Instantiation mean (10M) |
216.713 ms |
189.831 ms |
1.14 |
Instantiation mean std10M) |
1.321 ms |
0.933 ms |
1.42 |
Render mean std10M) |
1.372 ms |
0.66 ms |
2.08 |
Zoom cycle mean std10M) |
0.823 ms |
0.596 ms |
1.38 |
Downsample mean (50M) |
122.353 ms |
77.077 ms |
1.59 |
Downsample mean std50M) |
0.861 ms |
0.525 ms |
1.64 |
Instantiation mean (50M) |
1590.668 ms |
1237.805 ms |
1.29 |
Instantiation mean std50M) |
28.043 ms |
0.205 ms |
136.80 |
Downsample mean (100M) |
238.53 ms |
153.074 ms |
1.56 |
Downsample mean ( std00M) |
2.257 ms |
0.874 ms |
2.58 |
Instantiation mean (100M) |
3341.705 ms |
2602.888 ms |
1.28 |
Instantiation mean ( std00M) |
234.666 ms |
210.907 ms |
1.11 |
Render mean ( std00M) |
5.921 ms |
2.866 ms |
2.07 |
Downsample mean (500M) |
1282.428 ms |
770.747 ms |
1.66 |
Downsample mean ( std00M) |
2.993 ms |
0.874 ms |
3.42 |
Instantiation mean (500M) |
25502.063 ms |
22241.049 ms |
1.15 |
Instantiation mean ( std00M) |
301.114 ms |
210.907 ms |
1.43 |
Zoom cycle mean ( std00M) |
0.736 ms |
0.468 ms |
1.57 |
This comment was automatically generated by workflow using github-action-benchmark.
CC: @HanSur94
3 tasks
HanSur94
added a commit
that referenced
this pull request
Apr 28, 2026
Round 2 on the multi-platform refresh pipeline (after #93). Two remaining issues from validation run 25055144678: 1. Windows MATLAB still emitted no .mexw64 despite the install.m probe-2 guard. The Compile step ran for 6s with zero MATLAB output — too short for actual compilation, suggesting install() exited before first_run() ran (possibly a path resolution issue with `fileparts(mfilename('fullpath'))` inside run-matlab-command's temp cwd, but also generally undesirable in CI). install()'s needs_build cache is a user-facing convenience; CI should compile unconditionally. Replace `install();` with explicit `addpath('libs/FastSense'); build_mex();` plus a diagnostic header (cwd, mexext, arch, build_mex path) that prints regardless. 2. The aggregator's create-pull-request `add-paths` filter (with nested-glob patterns like `libs/FastSense/private/octave-*/**`) silently failed to stage despite git status showing all the modifications. peter-evans/create-pull-request then errored with "no changes added to commit" → "Unexpected error". Remove the filter; the aggregator's working tree is exclusively the committed repo plus extracted MEX artifacts plus the regenerated stamp, so staging all changes is correct here. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
1 participant
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.
Summary
Two coordinated fixes that make the multi-platform MEX refresh pipeline actually work end-to-end. After this lands,
refresh-mex-binaries.yml(manual or paths-triggered) will produce a complete 7-platform binary set and open an auto-PR back to main without manual artifact-juggling.Bug 1 — aggregator never opens the auto-PR
actions/upload-artifacttrims the least-common-ancestor of the supplied paths tolibs/, so each artifact's internal root isFastSense/...notlibs/FastSense/.... Withmerge-multiple: trueand nopath:configured, files extract under the workspace root — outsidelibs/. The aggregator'sfind libsstep finds nothing new,peter-evans/create-pull-requestsees no diff, exits silently. The auto-PR has never actually opened — PR #65's binaries were committed by hand, and v2.1.1's binaries (PR #92) were committed by hand too.Fix: add
path: libsto the download-artifact step so the trimmed-LCA contents reassemble underlibs/.Bug 2 — Windows MATLAB silently emits no .mexw64
install.m'sneeds_build()has a probe 2 that checks for a barebinary_search_mex.mexfile as a "current-platform MEX present" signal. Probe 2 was meant as an Octave fallback, but on MATLAB it falsely matches stale bare-.mexfiles sitting atlibs/FastSense/private/binary_search_mex.mex(cruft from before theoctave-<tag>/subdir convention). When probe 2 trips on a MATLAB platform with no current-platform binary committed:core_ok = trueneeds_buildreturnsfalsefirst_runskipped → no compile → no.mexw64upload-artifacterrors with "no files found"Linux/macOS MATLAB happened to dodge this in CI run 25051632929 by stamp-drift luck; Windows MATLAB hit it cleanly.
Fix: guard probe 2 (and the
octave-<tag>/probe 3) behind anisOctavecheck so the bare-.mexfallback only fires when the current runtime is actually Octave.Test plan
refresh-mex-binaries.ymland verify:peter-evans/create-pull-requestopens an auto-PR named "chore: refresh prebuilt MEX binaries" with the full 7-platform diffOut of scope (not in this PR)
.mexfiles inlibs/FastSense/private/,libs/FastPlot/private/, etc. — they're harmless once probe 2 is guarded; cleanup can be a separate housekeeping PR.🤖 Generated with Claude Code