-
Notifications
You must be signed in to change notification settings - Fork 10
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(.github): mismatch between CPU chip and binary name on macOS #401
Conversation
Signed-off-by: KeisukeYamashita <[email protected]>
WalkthroughThe changes in this pull request focus on updating the matrix configuration for the build job in the Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[actionlint] reported by reviewdog 🐶
the runner of "actions-rs/toolchain@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue [action]
- uses: actions-rs/toolchain@v1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[actionlint] reported by reviewdog 🐶
the runner of "actions-rs/toolchain@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue [action]
- uses: actions-rs/toolchain@v1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[actionlint] reported by reviewdog 🐶
the runner of "actions-rs/toolchain@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue [action]
commitlint-rs/.github/workflows/release.yaml
Line 141 in 547eff6
- uses: actions-rs/toolchain@v1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
.github/workflows/release.yaml (1)
29-35
: Consider universal binary support for macOSSince you're building for both Intel and Apple Silicon on macOS, consider creating universal binaries to simplify distribution and improve user experience.
Benefits of universal binaries:
- Single binary distribution for all macOS users
- Automatic architecture selection at runtime
- Simplified release management
This would require:
- Modifying the build process to create universal binaries
- Adjusting the artifact naming convention
- Updating the release documentation
Would you like assistance in implementing universal binary support?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
.github/workflows/release.yaml
(1 hunks)
🔇 Additional comments (1)
.github/workflows/release.yaml (1)
29-35
: Verify runner architecture guarantees
Please verify that these assumptions hold true:
macos-13
consistently provides an Intel-based runnermacos-latest
consistently provides an Apple Silicon runner
Run this script to check the current GitHub Actions documentation for runner specifications:
- os: macos-13 (Intel x86) | ||
rust_target: x86_64-apple-darwin | ||
ext: "" | ||
args: "" | ||
- os: macos-14 # beta (Apple Silicon) | ||
- os: macos-latest (Apple Silicon) | ||
rust_target: aarch64-apple-darwin | ||
ext: "" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Critical: Fix invalid runner syntax in matrix configuration
The current runner specifications contain invalid syntax that will cause workflow failures. GitHub Actions requires exact runner labels without parentheses.
Apply this correction:
- - os: macos-13 (Intel x86)
+ - os: macos-13 # Intel x86
rust_target: x86_64-apple-darwin
ext: ""
args: ""
- - os: macos-latest (Apple Silicon)
+ - os: macos-latest # Apple Silicon
rust_target: aarch64-apple-darwin
ext: ""
args: ""
This change:
- Uses the correct runner labels that GitHub Actions expects
- Maintains architecture information as comments
- Preserves the intended mapping between runners and rust targets
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
- os: macos-13 (Intel x86) | |
rust_target: x86_64-apple-darwin | |
ext: "" | |
args: "" | |
- os: macos-14 # beta (Apple Silicon) | |
- os: macos-latest (Apple Silicon) | |
rust_target: aarch64-apple-darwin | |
ext: "" | |
- os: macos-13 # Intel x86 | |
rust_target: x86_64-apple-darwin | |
ext: "" | |
args: "" | |
- os: macos-latest # Apple Silicon | |
rust_target: aarch64-apple-darwin | |
ext: "" |
Why
The
macos-latest
now points to Apple Silicon so the processor and the binary name (x86_64
) mismatches.To fix it, I pinned the
x86_64
binary name with the runner that runs the Intel chip.Ref
Summary by CodeRabbit