-
Notifications
You must be signed in to change notification settings - Fork 2.3k
chore: makefile rust targets macos #6457
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
Conversation
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.
Caution
Changes requested ❌
Reviewed everything up to 1db67ea in 1 minute and 46 seconds. Click for details.
- Reviewed
93lines of code in3files - Skipped
0files when reviewing. - Skipped posting
2draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. .github/workflows/template-tauri-build-macos-external.yml:89
- Draft comment:
Removed the explicit 'rustup target add x86_64-apple-darwin' call. Ensure that relying on the Makefile’s 'install-rust-targets' target provides the needed Rust targets consistently. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% This comment is asking for verification that the build will still work, which violates our rules. It's asking the author to "ensure" something works, which is a form of asking for confirmation. The build system will naturally fail if the required rust targets aren't available, so this doesn't need manual verification. Maybe the comment is trying to highlight a potential build reliability issue that could affect other developers? Even if there is a potential issue, the CI build itself will fail if the targets aren't properly installed. We don't need a comment asking for verification. Delete this comment as it's asking for verification of something that would be caught by the build system anyway.
2. .github/workflows/template-tauri-build-macos.yml:45
- Draft comment:
The manual steps to download, extract, and combine 'bun' and 'uv' universal binaries have been removed. Confirm that an alternative mechanism (e.g. prebuilt binaries) is in place so that required assets are still available. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_7BeouwcH9gFg9FbI
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
| yarn build:extensions && yarn build:extensions-web | ||
|
|
||
| # Install required Rust targets for macOS universal builds | ||
| install-rust-targets: |
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.
New 'install-rust-targets' target has been added and is now a dependency for both 'build' and 'build-and-publish'. As a best practice, consider declaring these non-file targets (e.g. install-rust-targets, build, build-and-publish) as .PHONY to avoid conflicts if a file with the same name exists.
Barecheck - Code coverage reportTotal: 29.37%Your code coverage diff: -0.01% ▾ ✅ All code changes are covered |
This pull request refactors the macOS build process for the Tauri app, moving platform-specific Rust target installation from the GitHub workflow files into the
Makefilefor improved maintainability and cross-platform compatibility. It also removes manual steps for creating universal binaries forbunanduvfrom the workflow, streamlining the build pipeline.Build process improvements:
install-rust-targetstarget to theMakefileto install bothx86_64-apple-darwinandaarch64-apple-darwinRust targets on macOS, with conditional logic for non-macOS systems.buildandbuild-and-publishtargets in theMakefileto depend oninstall-rust-targets, ensuring required Rust targets are always installed before building.Workflow simplification:
rustup target add x86_64-apple-darwinstep from both.github/workflows/template-tauri-build-macos.ymland.github/workflows/template-tauri-build-macos-external.yml, delegating this responsibility to theMakefile. [1] [2]bunanduvuniversal binaries from the macOS workflow, likely in favor of a more automated or prebuilt approach.Important
Refactor macOS build process by moving Rust target installation to
Makefileand simplifying workflows for Tauri app.install-rust-targetstarget inMakefilefor macOS Rust targetsx86_64-apple-darwinandaarch64-apple-darwin.buildandbuild-and-publishtargets inMakefileto depend oninstall-rust-targets.rustup target add x86_64-apple-darwinfrom.github/workflows/template-tauri-build-macos.ymland.github/workflows/template-tauri-build-macos-external.yml.bunanduvuniversal binaries in.github/workflows/template-tauri-build-macos.yml.This description was created by
for 1db67ea. You can customize this summary. It will automatically update as commits are pushed.